🇳🇱Netherlands @balintbrews

The issue summary already talks about nodes only, but I just wanted to reinforce that at this stage we're only targeting node bundles, no other entity types are in scope.

Edited the issue summary to note that only content types (node bundles) are in scope at this stage.

hooroomoo → credited balintbrews → .

Crediting Jesse for spotting a gap in the issue description.

Because of the data integrity issues, I wonder if we could prioritize the simplest possible change before we settle on the final solution, including soft-deletion etc.

I think it would be the following:

When users try to delete or make a component internal (i.e. remove from components), we currently do that check to see if the component is being used in the current page's component tree. We can now do that properly thanks to 📌 Calculate field and component dependencies on save and store them in an easy to retrieve format Active . I don't see an HTTP API for the audit info, so unless I'm mistaken, that would need to happen before we update the check on the UI.

Do we already account for first-party code component imports (JavaScriptComponent dependencies) in the audit logic?

balintbrews → created an issue.

balintbrews → created an issue. See original summary → .

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

The discussion here became outdated. Closing in favor of 🌱 [META] Library organization with folders Active .

balintbrews → created an issue.

I added the uiLibrary property in the issue summary which will allow us to track which part of the XB UI the folder belongs to. Note that we also need to be able to have folders under Code, but Code is not something that \Drupal\experience_builder\Entity\Component::computeUiLibrary is aware of.

#9 — No, we can avoid that, see #3538775-4: Handle data dependencies in CLI commands → .

I was thinking about backwards compatibility with the 0.x branch, and I was ready to go ahead and start different versions for the CLI to address that, but @justafish had a better idea:

We could see if in our upload command we receive a 422 error complaining about the dataDependencies property not being recognized — which would happen with the 0.x branch. In that case, we could retry the request while removing dataDependencies from the payload.

I might have been wrong in #6, and we may need to backport anyway and start branching with the CLI tool versions.

Important note: While this is targeting the 1.x branch, due to 📌 Handle data dependencies in CLI commands Active , it needs to be tested with an XB module installation from the 0.x branch. Backporting code of the CLI tool has no practical benefit since it's a separate release process, but this is a bit inconvenient until we fix 📌 Handle data dependencies in CLI commands Active .

Adding credit for reporting the issue and investigating the problem.

balintbrews → created an issue.

I'm picking this up.

balintbrews → created an issue.

Adding credit for the original bug report.

balintbrews → made their first commit to this issue’s fork.

griffynh → credited balintbrews → .

Unassigning as I'm not working on this right now, in case anyone would pick it up.

balintbrews → created an issue.

balintbrews → created an issue.

The backport tag probably only applies to the first MR that's already in.

Didn't mean to remove those tags.

Pushed it to 0.x.

wim leers → credited balintbrews → .

https://www.npmjs.com/package/@drupal/xb-cli is live. 🎉

balintbrews → changed the visibility of the branch 3537135-npm-audit-fix to hidden.

Updates to vulnerable packages have been applied. This also included updating Vite to 6. It was done in an initial merge request, which is now merged, so in case upgrading to Vite 7 (and Storybook 9, which we need in order to use Vite 7) takes longer, we're not holding up those updates.

My initial testing showed that with Vite 7 there are problems in dev mode when importing the WASM files for @swc/wasm-web and tailwindcss-in-browser. We've been using optimizeDeps.exclude for those, which previously worked.

balintbrews → created an issue.

Updated the issue summary based on what we know so far. I think what I said in #7 should be ignored based on the subsequent comments and a quick testing I did.

The MR previously received +1s from @larowlan and @wim leers. I'll open a follow-up for an additional unit test. @mglaman helped me verify the changes with manual testing.

Failing end-to-end tests are not related. We have a few in 0.x right now that have been failing all day, so we'll need to address those.

balintbrews → made their first commit to this issue’s fork.

Look what we have! 🤩 https://project.pages.drupalcode.org/experience_builder

Fantastic work, @justafish!

I think auto-detection should be provided as a convenience layer instead of our default way of deciding when to unwrap: a best effort safety net in case the code component author doesn't explicitly express that the code component is using client-side code (i.e. by adding the 'use client' directive in the code or setting a config entity property), and we're able to catch when the component should be treated as such. I say best effort because of two reasons:

1. We won't be able to examine code brought in via URL-based imports.
2. In the future we would like to support code components created via the CLI tool using custom JS compilation to open the door for writing components in other frameworks than React.

I also left a comment on the regular expression finding all hooks, because I don't think all hooks should prevent us from doing the unwrapping.

I hate to put this back to NW, but I don't feel like we completely cracked this yet. We could also acknowledge that as follow-ups, so feel free to override, especially if #20 means we'll work on an ADR after landing what we currently have.

#16: I think that's the deployed version of the XB app from the early days. 🙂 That will be taken over by the new docs site once the MR is merged. For MRs the docs are published at, e.g., https://project.pages.drupalcode.org/experience_builder/mr-1291.

@justafish: This is looking fantastic, I just left a few final minor comments.

+1 by @larowlan, back to me for adding tests.

Ready for a first-round review to see if we like the approach. I assume we'll want one or more additional tests in tests/src/Functional/CodeComponentDataProviderTest.php.

The new documentation site is looking amazing! 👏 I left a few comments.

Oops, didn't mean to do that.

This looks really great!

I think long term we should consider what both @larowlan and I suggested (in #9 and #4), having Vite/Rollup output filenames with a hash in them. That would allow us to do releases with backend-only changes where we don't force re-downloading unchanged JavaScript assets.

I updated the issue summary based on the comments.

I made a small change in the starter code so we don't set a bad example with prop spreading. Thanks to @effulgentsia for pointing that out. I re-tested the code component functionality, still works great with the changes by this MR. The UI code probably could use a quick review as I wrote all of it, I wouldn't want to sign off on my own code. 🙂

So we have the items in the import map generated in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::renderComponent.

Then we have a lot of JS files included in libraries (i.e. in experience_builder.libraries.yml) where the version gets appended from the library definition, but we never update those, and it's not even feasible for xb-ui as that gets recompiled and updated constantly. How are those usually handled? I think I've seen it before that a hash was added to the filename by the JS bundler, and the filename was read in hook_library_info_alter().

balintbrews → created an issue.

Thank you!

The MR looks good to me, and manual testing shows that it solves the problem. @narendrar, would you mind testing, as well?

We had a debugging session with @narendrar and @bnjmnm where we managed to identify how to reproduce the problem, so updating the issue summary.

#31: Right, I'm aware that Standard provides that, but most projects don't use Standard, or make changes when they do, so it would be easy to run into this problem.

#32: I think that's not a great experience. It's only slightly better than hitting the error we have now. A code component author might put in all the effort to build a polished component only to find out they can't use it.

Updating status based on Wim's latest comment. Besides that we'll need to re-test and validate from the frontend's perspective the actual part where the error was thrown before.

One more thing I wanted to add is what @mglaman and @phenaproxima helped me figured out after I was hitting the following 422 HTTP error:
Experience Builder does not know of a field type/widget to allow populating the <code>video prop, with the shape {\"type\":\"object\",\"$ref\":\"json-schema-definitions://experience_builder.module/video\"}.

The error happens as you're trying to save a JavaScriptComponent entity and you have no media type that supports local videos. (I didn't test what happens in case this prop shape occurs in an SDC — we are probably gracefully handling it by disabling the component, but it's worth checking.)

We discussed conditionally removing the prop type from the code editor when we detect that an appropriate media type is missing. Then there's the use case of a media type being removed after a prop like this was created, but maybe that's lower priority.

@lauriii, this issue could use your input.

@justafish proposed using GitLab Pages, so we'll get something like https://project.pages.drupalcode.org/api_client, which would be really nice. +1 from @lauriii and me, too.

#3: Nice, I like that. I would even remove the use of the cn() utility function, because it's for composition, and what it does is only obvious (without reading docs) for people who are familiar with shadcn/ui.

// Get started: http://drupal.org/docs/experience-builder/code-components

const Component = ({
  text = "Component",
}) => {
  return (
    <div className="text-3xl">
      {text}
    </div>
  );
};

export default Component;

Your proposed template had a drupal.org URL as the link — does that mean you prefer that over a Markdown file in the repo's docs folder?

Assigning to Wim to take care of #25.

I made the necessary changes as part of 🐛 Calling the `setPageData` action creator directly doesn't update values in the page data form Active .

Once critical piece left, see my comment on \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::rewriteExampleUrl.

balintbrews → made their first commit to this issue’s fork.