🇳🇱Netherlands @balintbrews

balintbrews → created an issue.

I don't have the exact steps to reproduce, but I may have an understanding of why this error occurs.

react-mosaic uses react-dnd for its drag & drop functionality. The Mosaic component wraps its logic in the DndProvider from react-dnd. I believe the problem we're seeing is that when we switch between routes, that context provider sometimes can't fully unmount itself before the new provider starts mounting itself on the new route. The solution is to decouple DndProvider from where we use the Mosaic component. Luckily, react-mosaic exports MosaicWithoutDragDropContext for us to do that.

So I'm adding react-dnd, rdndmb-html5-to-touch, and react-dnd-multi-backend as explicit dependencies — they were already indirectly added by react-mosaic — and wrapping everything in the context provider in App.tsx, so we can replace our use of the Mosaic component with MosaicWithoutDragDropContext. The provider then remains stable while switching between routes.

This is all just theory, as I don't have a way to reproduce the error. Let's thoroughly test that he new context provider doesn't cause issues with our other drag & drop library, dnd kit. Yes, ideally we would only use one, but this was so far an implementation detail of react-mosaic, and we already heavily based our content preview canvas interactions heavily on dnd kit.

For future consideration, I opened ✨ Move away from react-mosaic for the code editor UI Postponed .

If and when we decide to address this, please look at all the changes that were added in 🐛 Cannot have two MultiBackends at the same time Active , so all those can be undone. Specifically, we won't only need to uninstall react-mosaic-component, but also react-dnd, react-dnd-multi-backend, and rdndmb-html5-to-touch.

balintbrews → created an issue.

Note for reviews: before 📌 Support props that can use wysiwyg widgets Active is fixed, the props form of instantiated components will have an empty dropdown for selecting a text format.

#7: Would the new controller take care of returning the CSS for the dependencies (and their dependencies, recursively) of the import names we pass? I assume yes, otherwise we'd be back to why I chose the stop-gap solution where we would need to parse all dependencies recursively to be able to pass all component names.

Stop-gap solution done. 🚢

Assigning for review to land the stop-gap solution, then we'll leave the issue open for the proper one.

balintbrews → created an issue.

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

balintbrews → created an issue. See original summary → .

#3: Yes, that sounds great. I also discussed this with @lauriii, and we'll go with Formatted text to match the core field type's name. We also descoped displaying CKEditor in the code editor from this issue.

We are missing one smaller piece here. Here is how an import map currently looks like:

{
  "scopes": {
    "/sites/default/files/astro-island/z2TNPL8SBbhdCntH5L8-nUxBDIJKHG-QMqYOC8xazRY.js": {
      "preact": "/modules/experience_builder/ui/lib/astro-hydration/dist/preact.module.js",
      "preact/hooks": "/modules/experience_builder/ui/lib/astro-hydration/dist/hooks.module.js",
      "react/jsx-runtime": "/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js",
      "react": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
      "react-dom": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
      "react-dom/client": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
      "clsx": "/modules/experience_builder/ui/lib/astro-hydration/dist/clsx.js",
      "class-variance-authority": "/modules/experience_builder/ui/lib/astro-hydration/dist/class-variance-authority.js",
      "tailwind-merge": "/modules/experience_builder/ui/lib/astro-hydration/dist/tailwind-merge.js",
      "@/lib/utils": "/modules/experience_builder/ui/lib/astro-hydration/dist/utils.js",
      "@/components/card": "/sites/default/files/astro-island/e7KF9ds1KTO6BgcK5ocVIBs0AwihvLBL6dq9fxvnwxI.js"
    }
  }
}

@/components/card is brought in as a dependency of another component. So far so good! 👍🏻

Now, because everything in the import map is scoped under the individual components, when the code from @/components/card is imported, the browser won't know how to map react, react/jsx-runtime etc. To solve that, we need to move all those to the global scope:

{
  "imports": {
    "preact": "/modules/experience_builder/ui/lib/astro-hydration/dist/preact.module.js",
    "preact/hooks": "/modules/experience_builder/ui/lib/astro-hydration/dist/hooks.module.js",
    "react/jsx-runtime": "/modules/experience_builder/ui/lib/astro-hydration/dist/jsxRuntime.module.js",
    "react": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
    "react-dom": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
    "react-dom/client": "/modules/experience_builder/ui/lib/astro-hydration/dist/compat.module.js",
    "clsx": "/modules/experience_builder/ui/lib/astro-hydration/dist/clsx.js",
    "class-variance-authority": "/modules/experience_builder/ui/lib/astro-hydration/dist/class-variance-authority.js",
    "tailwind-merge": "/modules/experience_builder/ui/lib/astro-hydration/dist/tailwind-merge.js",
    "@/lib/utils": "/modules/experience_builder/ui/lib/astro-hydration/dist/utils.js",
  },
  "scopes": {
    "/sites/default/files/astro-island/z2TNPL8SBbhdCntH5L8-nUxBDIJKHG-QMqYOC8xazRY.js": {
      "@/components/card": "/sites/default/files/astro-island/e7KF9ds1KTO6BgcK5ocVIBs0AwihvLBL6dq9fxvnwxI.js"
    }
  }
}

I made slight changes to the UI, and adjusted the list of code components to exclude code overrides and the currently edited component. Also addressed @jessebaker's comment on making the list alphabetically sorted based on the name of the components.

borisson_ → credited balintbrews → .

I went ahead and merged the MR — after thorough manual testing and reviewing the code. Keeping the issue open to add the necessary tests.

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

Crediting people who were part of the discussion that led to the ADR at Drupal Dev Days in Leuven.

balintbrews → created an issue.

Yes, let's close this. 🙂🎉
All must-haves happened, I'll extract everything else into new meta issues.

balintbrews → created an issue.

#16: That's right, that's what's being implemented in this issue.

#12: That's amazing, thank you! 👏🏻

When there is no auto-saved version, that endpoint does a 307 redirect to the static JS file. I wonder if the browser will like that. But we'll find out soon! Or maybe you already know. 🙂

While on a call with @hooroomoo and @tedbow, it became clear to me that one piece is missing in the plan. We need to make sure the code editor preview can also understand these imports. Here is an idea.

1. Extend the code editor preview's import map by adding every imported component's compiled JS code in a blob URL — something like this:

   diff --git a/ui/src/features/code-editor/Preview.tsx b/ui/src/features/code-editor/Preview.tsx
   index 9f433faf..6a18e787 100644
   --- a/ui/src/features/code-editor/Preview.tsx
   +++ b/ui/src/features/code-editor/Preview.tsx
   @@ -79,6 +79,22 @@ const importMap = {
        'class-variance-authority': 'https://esm.sh/class-variance-authority',
        'tailwind-merge': 'https://esm.sh/tailwind-merge',
        '@/lib/utils': `${XB_MODULE_UI_PATH}/lib/astro-hydration/dist/utils.js`,
   +    '@/components/button': URL.createObjectURL(
   +      new Blob(
   +        [
   +          `
   +          import { jsx as _jsx } from "react/jsx-runtime";
   +          const Button = ({ children = "Button" }) => {
   +            return /*#__PURE__*/ _jsx("button", {
   +              children: children
   +            });
   +          };
   +          export default Button;
   +          `,
   +        ],
   +        { type: 'text/javascript' },
   +      ),
   +    ),
      },
    };
    

   As part of this issue, we will already know what other components are imported, and we can fetch their compiled JS code. The trick is to grab the auto-saved version if it exists. Use code from ui/src/features/code-editor/hooks/useCodeComponentData.ts as example.

2. Also add the CSS from the imported code components to the preview iFrame, similar to how we add CSS now in ui/src/features/code-editor/Preview.tsx. This will result in a lot of duplicated CSS, but we'll rectify that in 📌 Compile Tailwind CSS globally for code components Active .

We could extract this to another issue, but this one can't work without it anyway, so I'd suggest to include it in the scope here.

Although we could also use a fetchpriority of low and leave the browser to work it out

Something like that is what I had in mind.

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

@lauriii suggested this after investigating a performance bug report.

balintbrews → created an issue.

#10: I'm fine with that change, sure.

#11: I was still working on creating other issues under 🌱 [Plan] First-party code component imports Active , and I wanted to add a reference to make that clear. I updated the issue summary now. That part will need more work, I won't be able to define what's exactly needed there, but I had a call with @tedbow to discuss the high-level requirements, and he was fine with what is being proposed.

balintbrews → created an issue.

#15: That specific part was suggested as a potential improvement on top of what was outlined before — which I think should be considered as a complete solution. So ignoring that additional part, what are your thoughts?

I intentionally left #3503272 out of the scope. As of right now, machine name is the primary key of JavaScriptComponent entities. Once that changes, we'll need to revisit changes in this issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

#6 happened because of 🐛 Component name goes missing when first creating one Active .

I added a fallback icon provided by @reneelund.

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

Also @lauriii prompted us to fill this gap.

Code component to test with:

import React from "react";
import ReactDOM from "react-dom";
import { useState, useEffect, useRef } from "react";

console.log("React:", React);
console.log("ReactDOM:", ReactDOM);
console.log("useState:", useState);
console.log("useEffect:", useEffect);
console.log("useRef:", useRef);

const Test = () => {
  const [count, setCount] = useState(0);
  
  const createdElement = React.createElement("div", null, [
    React.createElement("h1", null, "Import Map Test (createElement)"),
    React.createElement("p", null, `Count: ${count}`),
    React.createElement("button", { 
      onClick: () => setCount(count + 1) 
    }, "Increment")
  ]);
  
  return (
    <div>
      {createdElement}
      <div>
        <h1>Import Map Test (JSX)</h1>
        <p>Count: {count}</p>
        <button onClick={() => setCount(count + 1)}>
          Increment
        </button>
      </div>
      {count > 5 && (
        <div style={{ color: 'green', marginTop: '20px' }}>
          Count is greater than 5!
        </div>
      )}
      <ul>
        {[1, 2, 3].map(item => (
          <li key={item}>Item {item}</li>
        ))}
      </ul>
    </div>
  );
};

export default Test;

@effulgentsia had this idea a while back.

balintbrews → created an issue.

Turns out this needed more work. I would appreciate help with thorough testing of auto-save, typing into the editor, switching between components etc.

#6 identified the cause, #7 had the right solution, I just simplified it and extended to the global CSS, too. Major oversight by me. 🤦🏻‍♂️
Thanks, @mglaman! I added steps to reproduce to the issue summary.

1. 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active can be worked on independently.
2. 📌 Compile Tailwind CSS globally for code components Active is the follow-up issue.

balintbrews → created an issue.

Vision: Lauri. Serious wordsmithing: Gábor.

balintbrews → created an issue.

kristen pol → credited balintbrews → .

Here is a rough outline I've been thinking about. I'll have more time to elaborate after Atlanta.

1. Going with @effulgentsia's example of the Card component including Button, and having the following import statement:

   import Button from '@/components/button'
   

   The code editor UI — and also the soon-to-be-worked-on CLI tool — can parse this, recognize button as the machine name, and translate it to the UUID of the Button component's config entity. (We already use @babel/parser to verify that the code contains a default export, which is currently a requirement.)

2. Now that we have the UUIDs for every dependency in the client, we can send that info along with the code component data to the backend, and it can be stored in a js_dependencies property. (This is where third-party dependencies can also be stored later.) Then the config entity dependencies can be calculated.
3. To make these import statements work, the backend can generate an import map, where e.g. @/components/button can be mapped to the hydrated JS file.

How about renaming code components?

With this approach, we have all the data in the client (both UI and CLI) to tell people what components need to be updated. I think this is enough as the first step. Then, to further improve this, nothing stops us from automatically carrying out those updates — again, still client-side.

It was barely a UX decision I made, and since the validation (based on the JSON Schema) passed on those values, I forgot to match this how Drupal usually expects these. Let's change in a follow-up.

According to @lauriii, we'll remove all components from XB, so we're not adding this in there either. Opened ✨ Add Drupal logo component Active in the SDDS queue.

I had an MR in the issue I originally opened for XB: ✨ Add Drupal logo SDC Active .

balintbrews → created an issue.

bnjmnm → credited balintbrews → .

I've seen this a lot recently, but didn't manage to find the time to come up with steps to reproduce. I'm very grateful for this report.

kristen pol → credited balintbrews → .

Looks great!

Two minor things:

1. I think we can get rid of all instances of IconType.COMPONENT1, as well as the styling for that type. Unless, see my question below.
2. Can you please also update the stories for the PublishReview component? We don't have a lot of stories in our Storybook, this component does have them. 🙂

Question to @lauriii, is it okay to use the same code icon for the exposed code components? They get a different icon the sidebar in that case, that's why I'm asking.

Also, do we want to update the icon for Global CSS as well while we're here? The API endpoint returns icon: 'cms' for it, so we may want to adjust this on the backend, but it's also easy to look at the entity type in the frontend component.

It's a mistake in the useEffect hook responsible for auto-saving changes: the timeout is cleared in the clean-up function, which means navigating to another route will cancel any pending changes. MR incoming.

There is another mistake there: compiledCss and compiledJs are dependencies of the same useEffect hook. Whenever the source code is updated, it will trigger the useEffect hook and kick off the compilation at the same time. The useEffect hook starts the 1 second time-out for the auto-save. If the compilation takes less than that, it will re-trigger the same useEffect hook, canceling out the already ongoing time-out. This means that triggering an auto-save can take up to 1.99 seconds depending on how fast the compilation is. This better explains to me why you were hitting this problem frequently. I will make sure to address this when I update this logic in the yet-to-be-created follow-up issue for 🐛 Global CSS is applied only after there are changes to a component after Global CSS was modified Active .

That looks very neat, thanks for sharing! 😍

I don't think we want to go this far in Experience Builder. I kind of just needed a quick solution for demos where the logo is in nice resolution, and thought it would come handy for others. The kind of solution you are suggesting should live in design systems.

balintbrews → created an issue.

I took a look, and what happens here is that if we use the contextual menu from the sidebar list, we will send the canonical entity data with the PATCH request when we update the JavaScriptComponent entity to rename it or make it internal/exposed, because that's what we have available in the React component that outputs that list. With a new code component which hasn't been published since it was created, it means that only the auto-saved version has the data — and we lose all of that. 🙈

This isn't a cache invalidation problem, because the endpoint that lists the code components doesn't use auto-saved data. To solve the issue, we need to:

1. Fix 🐛 PATCH request for config entities reset values that are not sent Active (priority increased)
2. Update our PATCH requests to include only the wanted changes:
   • ui/src/features/code-editor/dialogs/AddToComponentsDialog.tsx
   • ui/src/features/code-editor/dialogs/RemoveFromComponentsDialog.tsx
   • ui/src/features/code-editor/dialogs/RenameCodeComponentDialog.tsx

Increasing priority, because need to solve this in order to fix the rather critical 🐛 Adding component to component library results in component code and configuration being lost Active .

I think this is a duplicate of 🐛 No visible error message when trying to save with missing image Active , which may get solved by 📌 Split model values into resolved and raw Active .

I started seeing this problem with regular code components as well. Before updating the issue title and summary, can you please confirm, @lauriii? This seems like a regression after 0.2.0-alpha1.

Removing the required property from under slots. They can't be marked required.

Thank you, this has been annoying.

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

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

I got +1 from @lauriii.