🇳🇱Netherlands @balintbrews

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

The current changes in the MR fix the problem, but only without React.StrictMode (can be removed in src/main.tsx). React.StrictMode renders every component twice, and that must be causing a race condition. This will only affect the app in development mode, but it's still a bug we need to fix.

balintbrews → created an issue.

wim leers → credited balintbrews → .

We will expose simple functions for code component authors to retrieve data from drupalSettings.xbData, so they don't need to deal with the Drupalism that is window.drupalSettings. Our initial discussion concluded that for title and breadcrumb, we'll most likely have a getPageData() function, and getSiteData() for base URL and branding. The issue(s) to make these happen are yet to be created. Nevertheless, good call to have some explanation added in the starter component template. Which is getting really long, so we'll want to link to online docs, but that's another issue that we'll need to create and work on. 🙂

Wow. What an amazing idea! 🙇

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

#6: Love that, let's go with #5.1. It should be straightforward to adjust the MR to that direction.

#7: I think it depends on how much they understand about what next/image does. What's nice about next/image is that it's not very necessary to fully understand, it mostly just works. Either way, I'm more than okay to let this thought go, and I'm happy with #5.1, a.k.a. #4.

#4: That's a great idea! It's in line with how we chose to provide @drupal-api-client/json-api-client and SWR instead of a custom wrapper. More idiomatic, as you said. For both we add minimal customization and convenience, but we still keep their original names in the module specifiers. So I think we can do the same if we choose to provide next-image-standalone directly, as in, we would still have the room to add customizations, such as defaults to the config prop, as you point out.

The only potential downside is that a loader function is optional with next/image: it optimizes images using its own backend by default. I wonder if it could be misleading that we provide something called next-image-standalone, which may make it sound like a loader function is optional, just as it is with next/image. Whereas it is required for next-image-standalone, which is written in its README.

So the way I see it, we need to choose how we would like to communicate what is it that we offer:

1. "Here is a component that's almost identical to next/image, but you must provide a loader function. For that we also got you covered, here is a loader function that works with our backend."
2. "Here is a component you can use to output images. All you need is to pass these simple props. For more control, the component also accepts most props next/image does."

I created ✨ Image component for code components Active for the frontend counterpart and drafted the initial implementation based on our discussion yesterday.

balintbrews → created an issue.

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

balintbrews → created an issue.

I just realized that I meant to open the issue for api.config.get, that's where I originally discovered the problem. I haven't tested api.config.auto-save.get, so let's be sure to check that, too.

balintbrews → created an issue.

#7: The payload expects the page data values to be keyed by strings. So do this instead:

store.dispatch(setPageData({ 'title[0][value]': 'Newer title' }));

Unfortunately, I don't think atomic updates like this will work without modifying the action setPageData action creator. In our existing code we update the entire form state this way, see formStateToStore() in inputBehaviors.tsx.

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

#10: I don't think that MR fixes this issue. Also, there is nothing broken currently, it's just the way we include CSS in the code editor preview is not optimal, because we include CSS for every code component that exist. I re-titled the issue and updated the summary to reflect what's left here.

I created 📌 Support designating a code component slot as the React component's `children` prop to allow JSX nesting Active .

balintbrews → created an issue.

#3: We need the page title in drupalSettings.xbData regardless of that. 🐛 Handle adding page title to content Active will enable the page title block to work, but we would like to support building a dynamic code component where users can access the raw page title and add custom markup around it. It was amongst our original goals for ✨ Code Components as Block Overrides, step 1 Active , but we dropped that due to the challenges with page title block. Or maybe what you're saying is that we need 🐛 Handle adding page title to content Active even to be able to populate it in drupalSettings.xbData? 🙂

I added more details and what I propose as the data shape.

Our timeline to deliver the initial version of the CLI didn't allow for setting up the required automation and CI for integration tests. I hope we can prioritize that soon. Until then, what will definitely improve the situation is that we're planning to start sharing code between ui and cli once we get to ✨ CLI command to build Tailwind CSS for code components Postponed , which will mean shared TypeScript type definitions for how the component payloads should look like. That will catch a lot of problems already by static analysis.

Does the word "structured" in the issue title refer to adding content to fields of the xb_page entity type other than XB's own field (the components base field on the xb_page entity type) which we sometimes refer to as the canvas?

From that response it seems like that title would go in the title field of the entity. For that and any other fields on the entity (other than components) — see what's available in \Drupal\experience_builder\Entity\Page —, you can use the setPageData action creator from src/features/pageData/pageDataSlice.ts.

What about body? If the goal is to fill in fields other than components, then I think you should provide the information to the agent about what kind of fields are available to generate content for. There is no body field, but content could be generated for other fields in a similar fashion. If that content is meant to be for components/canvas, then you would need to tell the agent about the available components (SDCs, JavaScript components, and blocks) that can be used, and have it generate the component tree.

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

Placeholder already added in cli/src/utils/build.ts.

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

We'll cover this in ✨ CLI command to upload code components Postponed .

balintbrews → created an issue.

griffynh → credited balintbrews → .

griffynh → credited balintbrews → .

I also think removing it for now is the good call. Approved the MR, assigning back to @hooroomoo to resolve the conflicts, then this can be merged.

Thanks, @penyaskito, for highlighting this issue for me. After our discussions, I'm now unblocked with regards to the _user_is_logged_in access checker and other permissions — see my summary here: #3525572-13: Implement OAuth2 authentication for API endpoints related to code components → .

Thanks to @mglaman, I now understand that I was having problems with the _user_is_logged_in access checker because of how my OAuth2 consumer was configured while testing. The client credentials grant type allows configuring a user to be used "as the author of all actions made". \Drupal\simple_oauth\Authentication\TokenAuthUser decorates the user account defined here, so when the _user_is_logged_in access checker calls isAuthenticated, it was checking that user. So this is expected, and won't be a blocker for this implementation, I just need to make sure (and document) that the anonymous user is not configured.

The administer themes permission will be handled in 🐛 Content authors cannot see components Active , and the access administration pages might also be going away based on some discussions.

So I'm good to proceed with the current permissions. I may rethink the scopes a bit.

tim.plunkett → credited balintbrews → .

#10: It is the limitation of the Simple OAuth module → , yes, but it doesn't change the fact that the permissions could use some love. We can always use umbrella scopes, so my ask is not necessarily to express everything with a single permission.

Let's focus on how we could:

1. improve the situation with administer themes and access administration pages — it doesn't feel right to map these to OAuth scopes in the context of those API operations;
2. remove the use of the _user_is_logged_in route requirement, and perhaps use a permission instead.

While the first one is more about semantics, the second one is potentially a hard blocker for this issue.

I still would like to add tests that make actual HTTP requests.

Besides that, we need to overcome some challenges with permissions. I naïvely thought when I created the issue that administer code components will cover everything we need. That's close, but not quite the full picture. Here is an overview from the perspective of API endpoints:

1. The biggest blocker is the _user_is_logged_in route requirement for the experience_builder.api.config.list route. That will break things in an OAuth-context. Maybe there is a way to remove it dynamically for those requests, but I wonder if we could handle that requirement differently.
2. Then there's the administer themes permission for the INDEX operation which not only is not the most intuitive choice, but it also returns all the data that can otherwise be accessed individually with a GET request, but for those the administer code components permission is needed.
3. The access administration pages permission also may not be intuitive either.

The key to potentially rethinking our permissions is that when using them in OAuth2 scopes, we need to have 1:1 relationships. A scope can't be associated with multiple permissions. We can have umbrella scopes holding other scopes, though.

I would love to simplify this and make it nicer for the scopes we provide in config. Any thoughts on how much we could easily update our permissions?

I completely get the appeal of avoiding the complexity, but we will need to provide OAuth2 to follow industry standards, and to account for the use case when someone wants to manage components on multiple sites.

We also have these two awesome POCs:

1. Pinto theme objects (Drupal Slack thread)
2. Layout Builder layouts (Drupal Slack thread)

I'm surprised to see this piggyback on top of SDC though

I suggested to start prototyping like that, because if that works, there's a clear path towards a new component source plugin, which is exactly how we started experimenting with code components. 🙂

Crediting people who participated in the conversation that resulted in this issue.

balintbrews → created an issue.

I agree with #12 and #13 ✨ Preload WASM files for compiling code component previews Active : An action on the UI would be better to use as a trigger for preloading the assets.

How close are we to introduce granular permissions to code components? If we are, I also agree that the current MR could go in, since it only impacts the "administer code component" permission.

#13 ✨ Preload WASM files for compiling code component previews Active :

Also, we don't need these when the user first enters the code editor, do we? Since either it's a new code component, so we're using the sample code, or it's an existing code component, so we're using whatever was previously saved, so in both cases, we already have previously compiled stuff we can show in the preview.

This sounds very sensible, but at the time of implementing that, I thought it was a good idea to always re-compile the first preview for these components to ensure consistency with the current compilation config. Now it seems unnecessary, however, we do additional compilation for the code editor preview only: we compile the slot examples into Preact components. (dangerouslySetInnerHTML is not supported on Fragment, so we compile the example HTML code for slots into proper Preact components.) If we were to switch to using the compiled JS for the code editor preview, we'd need to start storing that as well.

My recommendation for the (future) UI changes would be to use the toast and overlay that was introduced in 🐛 Auto-save changes from code editor get lost if you navigate out too quickly Active when the code editor opens, and the WASM assets are not available. If and when we decide to start using the compiled JS even for the first preview, we can still rely on this UI pattern, but not right when the code editor opens.

bnjmnm → credited balintbrews → .

hooroomoo → credited balintbrews → .

penyaskito → credited balintbrews → .

FTR, we now have 🌱 [Meta] CLI tool for code components Active .

It works! 🙂 Assigning to Ted for code review.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

balintbrews → created an issue.

Now that 🐛 Canvas and hover preview out of sync after drag-drop and publish Active is straightforward to land, let's do that first, so we don't get confused by the problem that issue fixes.