Consider renaming javascript component ID to 'id' rather than machineName

FYI: \Drupal\experience_builder\Entity\JavaScriptComponent::$machineName was specifically chosen to match SDC's core/assets/schemas/v1/metadata-full.schema.json#properties.machineName

As documented in config schema:

    # See core/assets/schemas/v1/metadata-full.schema.json#properties.machineName
    machineName:
      type: machine_name
      label: 'Machine Name'
      constraints:
        Regex:
          pattern: '/^[a-z]([a-zA-Z0-9_-]*[a-zA-Z0-9])*$/'
          message: 'The %value machine name is not valid.'

I get that we are following SDC, but we also use snake case for all other config entity properties. Not sure what is best here.

I don't feel strongly, I was just explaining the rationale!

If you got confused by it, and it sounds like #2 does not convince you.

So: you decide! 😄 What did you expect? What would've caused the least friction for you? Name it so! 👍

FWIW, I thought not using snake case for the property name was an oversight when I reviewed the code for ✨ HTTP API for code component config entities Active , and saw all the other properties.

I realized I was looking for 📌 JavaScriptComponent config entities should have mutable machineNames Active when I found this issue. Should we close this in favor of 📌 JavaScriptComponent config entities should have mutable machineNames Active and handle the renaming there?

#6: yes, please.

Except … maybe we wanna repurpose this issue to tackle #5?

This is basically all you need for that:

diff --git a/src/Entity/JavaScriptComponent.php b/src/Entity/JavaScriptComponent.php
index 86e66995..062e5f63 100644
--- a/src/Entity/JavaScriptComponent.php
+++ b/src/Entity/JavaScriptComponent.php
@@ -99,10 +99,10 @@ final class JavaScriptComponent extends ConfigEntityBase implements XbHttpApiEli
         'props' => $this->props,
         'required' => $this->required,
         'slots' => $this->slots,
-        'source_code_js' => $this->js['original'] ?? '',
-        'source_code_css' => $this->css['original'] ?? '',
-        'compiled_js' => $this->js['compiled'] ?? '',
-        'compiled_css' => $this->css['compiled'] ?? '',
+        'sourceCodeJs' => $this->js['original'] ?? '',
+        'sourceCodeCss' => $this->css['original'] ?? '',
+        'compiledJs' => $this->js['compiled'] ?? '',
+        'compiledCss' => $this->css['compiled'] ?? '',
       ],
       preview: [
         '#markup' => '@todo Make something 🆒 in https://www.drupal.org/project/experience_builder/issues/3498889',
@@ -128,12 +128,12 @@ final class JavaScriptComponent extends ConfigEntityBase implements XbHttpApiEli
       'props' => $data['props'] ?? [],
       'slots' => $data['slots'] ?? [],
       'js' => [
-        'original' => $data['source_code_js'] ?? '',
-        'compiled' => $data['compiled_js'] ?? '',
+        'original' => $data['sourceCodeJs'] ?? '',
+        'compiled' => $data['compiledJs'] ?? '',
       ],
       'css' => [
-        'original' => $data['source_code_css'] ?? '',
-        'compiled' => $data['compiled_css'] ?? '',
+        'original' => $data['sourceCodeCss'] ?? '',
+        'compiled' => $data['compiledCss'] ?? '',
       ],
     ];
   }

(plus equivalent search+replace in \Drupal\Tests\experience_builder\Functional\XbConfigEntityHttpApiTest::testJavaScriptComponent().)

#7 📌 Consider renaming javascript component ID to 'id' rather than machineName Active : Yes, please, let's 🐪Case! (Just like in ✨ Send entity keys to the editor from the mount controller Active .)

Re-scoping to #7 per @balintbrews in #8.

Hey, I have updated the code in the JavaScriptComponent.php file as per the discussion and created a merge request for the changes. Please review and let me know if anything further is required. Thanks.

Thanks! I'm afraid there's lots of out-of-scope code reformatting, even causing the phpcs CI job to fail 😇

Besides that, the other next step: update the UI :)

Hi @wim leers, could you please clarify what you mean by “update the UI”? Additionally, could you outline the next steps required for this issue so I can proceed accordingly? Thank you!

Hi @libbna, I think I can help here.

I would first take a look at the pipeline for the MR and you'll see a number of PHPCS issues, which is due to the reformatting @wimleers mentioned.

In terms of the UI, it is currently still expecting `source_code_js` to be valid, if you search the code base for `source_code_js` you'll see it is referenced in a number of the UI React files, these will also need updating.

Hopefully that makes sense and you can proceed.

Hi @thoward216 thank you so much for clarifications.

Assigning the issue to myself to continue working on it as mentioned.

Just to confirm, should I follow the same approach for the other entity IDs as well?

I have updated the code. Please review and let me know if anything is missed.

eslint failed

Moving back to needs work as theres a PHP unit test failing.

Gonna continue this, would like this changes in for CLI tool work

Gonna push this to the finish line since its close, would like this changes in for CLI tool work

❤️ — thanks! 🙏

Conflicted (trivially) with #3526297. Resolved those conflicts.

Looks like ✨ CLI command to download code components Postponed already landed, 1 day after @hooroomoo pushed this forward.

Worryingly, the CLI tests are not failing … because they're not running! This means its tests are not running when underlying infra changes; only when CLI code itself changes. Fixing…

Updated logic + test expectations for the following that landed since June 12, thanks to test coverage:

• ✨ Implement OAuth2 authentication for API endpoints related to code components Active
• 🐛 External AI Chatbot Functionality Active

I think this is ready, but the CLI test coverage seems to lack integration/functional tests 😱 Which is why the CLI vitest CI job passed when it clearly cannot work 😅
We clearly shouldn't fix that here, but 🌱 [Meta] CLI tool for code components Active needs a child issue for creating such test coverage.

Removing tag here because I posted this at #3525571-5: [Meta] CLI tool for code components → , which is the more appropriate place. It's really no fallout of this issue.

there's 1 phpunit test failing that i'm stumped on

Thanks for updating all others, I’ll take care of that last straggler! 😄

Glad to see this MR’s work coming to fruition rather than waste 👍

Odd … passes locally? 🤔

Too fiddly to merge today. Enjoying the sun 😎

Automatically closed - issue fixed for 2 weeks with no activity.