🇳🇱Netherlands @balintbrews

Wim and I will meet tomorrow to talk things through, then I will make the necessary adjustments.

Sorry for the confusion. Everything for JS components is happening in 📌 Leap ahead of #3493070 in core: SDC `enum` props should have human-readable labels: use `meta:enum` Active .

We should reduce the code comments over time in the default component code by improving the UI. So we need to design this experience, potentially getting inspiration from how Drupal already solves this with machine names.

Perhaps as an interim step, we could simply display the calculated variable name for props.

Yes, that issue didn't affect what's being reported here. So this issue is still relevant.

How relevant? Our starter code component code explicitly says "Do not include @import 'tailwindcss'", and it also demonstrates how Tailwind CSS just works without that, so I recommend we change the priority to minor. 🙂

Lauri, please bump it back in case you disagree.

📌 Do not inline script for code editor preview Active was our immediate response, but it didn't fix everything. E.g. we still use the blob URI scheme, which still may not be great when it comes to strict CSP configurations.

@attilatilman, what is the current config you're using?

#16: The global asset library is not in play here if we follow what I suggested in #11.

So, let's hit reset on this issue. 🙂 📌 Compile Tailwind CSS globally for code components Active landed with a lot of improvements we can build upon to fix this problem. I originally thought I'd do this as part of the Tailwind issue, but I ended up only adding the necessary UI states to the code editor Redux slices.

Updating the issue summary, crediting @callumharrod for the toaster and overlay design.

FTR, confirmed with @mayur-sose that the issue is now fixed.

Thanks for spotting this, @penyaskito!

Original MR was added in the hotfix release of 0.2.1-alpha10. Test was added as a new commit, as it already uses a component that was added through changes that are not part of the hotfix.

RTBC, but let's hold off with merging, this might go into a hotfix relase first.

Lee, if you have time, a review would be super helpful.

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

I think our end-to-end test coverage should be focused on the end user's experience. React's <StrictMode> is wonderful for development, and you're right that if it makes our app behave differently, that's most likely a bug. The kind of problems <StrictMode> potentially surfaces during development, we should fix and verify at the unit testing layer using Vitest, which we recently introduced in 📌 Compile Tailwind CSS globally for code components Active .

I did some investigation, turns out we were missing some needed client-side invalidation — see MR —, but that is not fixing all the issues. The preview thumbnail can still show stale data. I tried it on top of 🐛 Components sourced from JsComponent are missing source component cacheable metadata Active , and that didn't help. I recorded a video, I'll post it shortly.

#9 shows 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active — now that we landed 📌 Compile Tailwind CSS globally for code components Active . More context: We switched from saving the compiled Tailwind CSS in individual code components to saving it in the global asset library. This is the first time we're actually using the global asset library for assets that are needed for rendering, so it surfaced the need for 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active .

→ Let's not use Tailwind CSS for testing this issue to identify clear boundaries of what we're trying to fix.

Here is an example code component for testing:

JavaScript:

export default function Card() {
  return <div className="card">Am I updating? 🕵️</div>;
}

CSS — NOT global CSS:

.card {
  padding: 2rem;
  font-size: 1.5rem;
  font-weight: 500;
  background-color: teal;
  color: white;
}

Note that we'll need 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active for a full experience. Thanks to Wim for pointing this out.

This just became very relevant for a good experience after 📌 Compile Tailwind CSS globally for code components Active lands.

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

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

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

This is ready for a first round of review. I haven't done the "Fix auto-save UX issues" part from #7, but I added an isCompiling and isSaving status to the Redux slice. I'll move the rest of that work back to 🐛 Auto-save changes from code editor get lost if you navigate out too quickly Active , this MR is already enormous.

I recommend starting the review from useCodeEditor.ts and move towards the hooks used in there. To understand what's happening, there is a flowchart in the MR. I refactored codeEditorSlice.ts, which produced changes in many files: I added comments to all of those with a 🟢 emoji to help skipping over those.

The MR also introduces Vitest for component/unit tests. I really wanted to have these lower level tests for the updated functionality, and Cypress components tests are not suitable for mocking/stubbing ES modules.

Thanks, @larowlan, for the sanity check on my tests, and pointing out inefficiencies with the new CI job.

I got started with this, because I needed it for efficiently mocking in tests I'm writing for 📌 Compile Tailwind CSS globally for code components Active . I'm already using this code there in ui/src/features/code-editor/Preview.tsx, but everything else still need to be refactored.

I know I originally suggested to create React hooks, but I was wrong. 🙂 No need for React. This can live as a simple utility module.

This is the expected behavior. Component authors need to use appropriate selectors. Or use the built-in Tailwind support, which fully avoids scoping issues.

In the (probably distant) future, we can add support for CSS Modules, but even then we won't get scoping out-of-the-box — the components will need to import the CSS and reference class names that way.

hooroomoo → credited balintbrews → .

hooroomoo → credited balintbrews → .

balintbrews → created an issue.

wim leers → credited balintbrews → .

#5: We merged a simple MR for a stop-gap solution in 🐛 Code editor preview doesn't include CSS for dependencies of dependencies Active , then left the issue open for an improved fix to land later. I'm leaning towards marking this issue as fixed, but I could go both ways. Thoughts, @effulgentsia?

I think it became clear that the direction of the MR in this issue is not right. I've been trying to patch something already way too fragile. I'll tackle the problem in 📌 Compile Tailwind CSS globally for code components Active where I posted my plan (#7) 📌 Compile Tailwind CSS globally for code components Active .

Leaving this issue open for now, but marking it as postponed.

Changes by this issue are going to significantly effect the code for saving JS component data. I would like to take this opportunity and improve the auto-save logic. Here is my high-level plan.

Make auto-save more robust

Auto-save currently works by subscribing to any changes in the code editor Redux slice. The complexity of this is that there are changes that will occur upon loading the data for the first time, and we need to ensure that those are not triggering an auto-save.

The way this is currently implemented is fragile: The implementation makes the assumption that changes that occur first are from data loading, and whatever changes come after are good to auto-save. Certain race conditions can easily break this — and they have in test environments.

I would like to update that logic in a way where the actions dispatched to update the code editor can express whether auto-saving is desired. So instead of the subscriber logic trying to figure out when auto-save is appropriate, we let the individual actions tell when it is not. By default we should assume auto-save is desired, as that is the more common case.

Fix auto-save UX issues

The current auto-save implementation has a big problem. Data can be lost if users navigate away from the code editor route before saving the data has been completed. This can sometimes take up to seconds, especially in slower environments. 🐛 Auto-save changes from code editor get lost if you navigate out too quickly Active tracks this problem.

To mitigate this, we can delay navigation using the useBlocker hook of React Router in the following cases:

1. Auto-save is pending;
2. JS/CSS compilation is in progress.

Whenever users try to navigate away, we should

1. display a toaster informing the user that saving/compilation is happening;
2. disable code editor input;
3. disable editing props and slots.

Once the JS/CSS compilation is finished, and auto-save is done, we'll automatically proceed with the navigation.

An important change that is required for this, which will also help making auto-save more robust in general, is that auto-save should not be subscribed to changes in the compiled JS or CSS. Instead, finishing the compilation should dispatch the auto-save action. This way we ensure that an auto-save always explicitly follows a compilation. Navigating away can be sufficiently delayed, and compiling the preview for the first time will not trigger an auto-save. (Preview is always compiled, never read from the saved data.)

Reviewed the code and tested manually, including switching between 0.x and the MR's branch to see the effects, and the problem is gone.

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.