🇺🇸United States @effulgentsia

Account created on 2 September 2006, almost 19 years ago
  • Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇺🇸United States effulgentsia

Postponing this on Unwrap astro-islands and astro-slots Active , because that issue will make this one more complicated, because once we remove the astro wrappers, it's less clear how to then target the component instance and re-render it with updated props. It's definitely solvable, just trickier.

Related, it doesn't strictly need to block a beta despite #5. Though I still stand by #5 as a strong nice-to-have, so instead of removing the "beta blocker" tag entirely, this is now our first issue that I'm tagging as a "beta target". And now that we have our first one, we'll likely start using this tag for some other issues as well :)

🇺🇸United States effulgentsia

This is currently a several second latency, just a single time and then browser cached, and only for people editing code components. It doesn't need to block a beta. It's possible we'll manage to squeeze it in before beta anyway, but we don't need to track it for that.

🇺🇸United States effulgentsia

I think this is close to landing, so whether or not it's tagged as a "beta blocker" is probably moot, but I'm untagging it per #44.

🇺🇸United States effulgentsia

I moved the "beta blocker" tag to just 📌 Create UI for selective publishing of changes Active and tagged the other children as "stable blocker".

🇺🇸United States effulgentsia

Organized the issue summary by beta blockers vs stable blockers.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

This doesn't need to block a beta since the Page entity type doesn't have an image field (only a media field), but ideally, a stable release of XB 1.0 should work with all core field widgets.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

transforms aren't applied until after the field value validates so we might need to revisit that.

Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

Should we open a separate issue to fix this, or do it as part of this issue, since this issue is the first to surface the problem?

🇺🇸United States effulgentsia

I like the main concept of this MR, which is to have "dynamic image styles", which I see as embodying three things:

  • The "image style" part of the name implying that they behave just like regular image styles with respect to when, how, and where the image derivatives are generated, stored, and flushed (see further down in this comment about flushing which isn't implemented in the MR yet).
  • The "dynamic" part of the name implying that each variation (e.g., different widths) does not get saved as a separate config entity, so as not to clutter the config management and image style listing and selection UIs. The MR currently doesn't store any config entity for these dynamic image styles, but further in this comment I propose saving one for a group of variations, just not one per variation.
  • It's not implemented in this MR yet, but I think another desired meaning of "dynamic" here is that all variations within a group produce the same itok, thereby allowing the front-end to pick the variation by just varying a part of the URL without needing to also worry about getting a different itok for each one. This broadening of itok from giving access to a single derivative of a single image to giving access to a set of derivatives of a single image would make this easier to integrate into common front-end image components like the ones in Next or Nuxt. However, to prevent DDOS attacks, this means the set of variations within a group (e.g., the set of widths) needs to be reasonably bounded. This MR bounds to 8 hard-coded widths matching the default deviceSizes in Next.js. We might want this to be configurable in some way, whether we add that configurability here or in a follow-up.

With the above preamble out of the way, some thoughts about the details...

These URLs are very long.

I agree, and I don't think we need the full genericism of being able to specify any combination of effects within the image style name. I think our use-case for now is just about widths. It's conceivable we'll want additional things to be dynamic in the future (format, quality, focal point, etc.), but I don't think we need to solve for that now. I think it's equally possible that those settings don't actually need to be dynamic in the same way that width does.

In other words, I think XB's use case could be addressed with short image style names like xb--WIDTH (e.g., xb--640) or whatever other delimiter we want to pick if we think there's something preferable over --.

The dynamic image styles are cool - but if the device sizes are hard-coded - do we need them - can't we just ship N image styles in the modules config/install?

That is indeed one option, but I don't think it's optimal, because it would add clutter to image style listing and selection UIs, and image styles don't implement locked or no_ui, so we'd need to add code to prevent them from being edited or else let them be edited but figure out how to deal with them getting out of sync (e.g., their name not matching the width setting). Plus we'd still need to implement itok sharing, so they'd be in this in-between concept of sort of dynamic and sort of static. Overall, I think there's more downside than upside to have a stored config entity for each width.

This is dynamically creating the image style, but never saving it. How will it handle image style flushes when one stops being used?
Less important than flushes but also relevant here, how will it handle garbage collection when source images are deleted from disk?

One option would be for XB to implement hook_cache_flush(), hook_file_move(), and hook_file_predelete(). However, that wouldn't exactly provide parity with regular image styles, because regular image styles are not flushed by a regular cache clear and can be flushed on their own via the UI or a Drush command. So if we want parity with that, I propose that we create an image style that represents the group. For example, we ship with an xb image style, which then exists as a stored config entity, that xb--* image styles are "derived" from (but not individually stored). Then we can implement hook_image_style_flush() to flush all the --* ones whenever the base one is flushed.

"target_id": {
  "title": "file id",
  "type": "integer"
}

I don't like this being added to the json schema definition of the Image prop type. Prop types should be independent from Drupal's data model. What I recommend instead of this is adding a srcsetTemplate property that's of type uri-template. So its value for an image foo.jpg would be /sites/default/files/styles/xb--{width}/public/2025-06/foo.jpg.webp?itok=.... Client code could then generate a srcsset from this by replacing {width}. Or, if integrating with a Next or Nuxt image component, you'd configure a loader/provider function that does that replacement.

Ensure that the module can function with security tokens (i.e. itok)

Per the top of this comment, we need the same itok for each width. This MR already implements a param converter that instantiates the image style even though it's a dynamic image style that doesn't exist in config storage. I think the only addition needed for this is to instead of instantiating it as an ImageStyle to instantiate a subclass (e.g., DynamicImageStyle), where the subclass overrides getPathToken() from hashing $this->id() to instead hashing just the part of $this->id() before --.

🇺🇸United States effulgentsia

We're going to have some early XB adopters at Acquia (and maybe elsewhere, but I only know about the ones at Acquia) running XB beta1 (which is great for helping us find problems ahead of tagging a stable release) who won't be able to upgrade to Drupal 11.2 right away, so we're not quite ready yet to constrain XB to 11.2 only.

🇺🇸United States effulgentsia

I haven't looked at the XB codebase related to this deep enough to comment on the code-level implementation suggestions of #8, but I'm +1 to the high level premise if I understand it correctly, which is:

If an auto-saved entry exists, and there is 1 or more fields for which:

  • The value is different than what the published entity has, and
  • The current user doesn't have edit permission for it

Then the user should not be allowed to make any changes at all on the Page Data tab. I think ideally the Page Data tab would still be visible, but be read-only, with a message at the top saying that it's read-only because there exist unpublished changes to it that were made by someone with different permissions and must be published or reverted by someone with those permissions before it can be editable by others.

Then we can punt any further scope (i.e., allowing edits of the permissible fields and merging those into the auto-save) to a post-beta, and maybe even post-stable, follow-up.

🇺🇸United States effulgentsia

Any downside to doing a version check?

'sqlite_type' => version_compare(\Drupal::VERSION, '11.2', '>=') ? 'json' : 'text'

I'm not clear if any upgrade path would actually be needed or if the above would just automatically make things better once people upgrade to 11.2.

🇺🇸United States effulgentsia

With all of the other changes that have landed in the last few weeks, is this issue still relevant? If so, what are the steps to create a broken auto-save state?

Tagging as a beta blocker to answer this question and evaluate. Depending on the answer, we might decide that actually fixing it doesn't necessarily have to block a beta.

🇺🇸United States effulgentsia

It's possible this is already at a state that's good enough for beta, but in case it isn't I'm tagging this as a beta blocker to make sure we evaluate it before releasing a beta.

🇺🇸United States effulgentsia

Possibly related to #16, if Component config entities have the config schema type of versioned_config_entity, what happens if an old version has a dependency but the active version doesn't have that dependency? Does the dependencies of the Component config entity include only the dependencies for the active version, and if so, is there anywhere that tracks the dependencies of old versions (for which there might still be stored json blobs)?

🇺🇸United States effulgentsia

I'm not quite following the last two paragraphs of #15. What plugins do the JSON blobs for StaticPropSources depend on that aren't dependencies of the Component config entities?

🇺🇸United States effulgentsia

This MR doesn't need the Drupal\Driver\Database\sqlite classes, only the Drupal\experience_builder\... classes. The former was only for Drupal 9 and maybe early minor releases of 10. https://www.drupal.org/project/mysql57 and https://www.drupal.org/project/sqlite337 can be used for reference for Drupal 11.

drush site-install doesn't work if I have the namespace

Yeah, I've run into this too with https://www.drupal.org/project/sqlite337 when installing without an already existing settings.php. What I found is I need to use the browser installer to write out settings.php the first time, and then drush site-install works if I keep that settings.php around.

Stepping back a bit though, I'm curious why XB needs to leap ahead of 🐛 Cannot delete a field which uses JSON type Active . Does SQLite itself actually have a json type? I thought it only had text and jsonb, and you can migrate from text to jsonb both incrementally and at anytime, so why does XB need to do it before core's driver supports it?

🇺🇸United States effulgentsia

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 exiting 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. Which means we don't need to execute the wasm code until the user starts typing something into the code editor. At which point, could we put a message in the preview area saying "Generating Preview..." or something like that if the wasm files haven't fully downloaded yet?

🇺🇸United States effulgentsia

The code in the MR looks good, so I approved it (before seeing Wim's comments), but it seems questionable to me to be preloading 30MB of stuff (even with low fetch priority) before we even know the person wants to edit a code component. I wonder if we should delay this until there's some further indication of that, such as:

  • Clicking "Add new component" (after which there's a bit of time for the user to enter a name for it)
  • There already existing at least one code component (either as a real config entity or in auto-save)

React-dom has a preload function by the way, if we want to initiate this based on some client-side determined condition.

Since this MR only preloads for people with the "administer code component" permission, I'd be okay with this being merged as-is (once Wim's okay with it) and us discussing the above in a follow-up.

🇺🇸United States effulgentsia

Although I don't think this needs to be figured out before beta, I think before tagging a stable release we should come to a decision on whether a slot should be thought of as a black box, or if it's legitimate for SDCs to treat a slot as an array of components and be able to decorate them in some way.

🇺🇸United States effulgentsia

Tagging this as a beta blocker, because we want early adopters of the beta able to run XB in production, including under potentially heavy server load.

🇺🇸United States effulgentsia

Upgrading this to a beta blocker, because we want early adopters of the beta able to run XB in production without having their CSS break when we eventually add SSR.

🇺🇸United States effulgentsia

Editing the JS code of code components requires a restrict access permission, so using non-sandboxed iframes for the various previews isn't a vulnerability, but sandboxing them would help add extra defense against some privilege escalation vectors, so switching the tag from Security to "Security improvements".

However, I'm still tagging this as a beta blocker as well, because we want early adopters able to run the beta in production, and this would help provide extra confidence for doing so.

🇺🇸United States effulgentsia

Yay about #5 that Views across exposed slots is not a necessary capability.

Another concern I thought of: if we do one-field-per-exposed-slot, would that prevent us from being able to use Workspaces and wse_config to stage changes to content templates if such a change involves adding or deleting exposed slots? XB doesn't yet integrate with Workspaces and wse_config, but it's something we'd like to add eventually, even if after 1.0.

🇺🇸United States effulgentsia

I think the main drawback of one field per exposed slot is with respect to future Views integration. Suppose in the future we want to be able to support Views (or even just EntityQuer(ies)) for finding all instance of a Heading component whose element prop is equal to h2 (or likewise get lists of component instance matching whatever other condition). If we want such a View to be able to query across all exposed slots, I think that gets more complicated (maybe not impossible) when the data for each exposed slot is in a different field.

The main benefit of one field per exposed slot is being able to leverage Drupal's existing infrastructure for deleting fields (and the corresponding batched purging, etc.) when exposed slots get deleted. Although that is a benefit, it would be limited to just the case of deleting exposed slots. It wouldn't, for example, solve deletion of non-exposed slots (e.g., a code update to an SDC that removes a slot), or deletion of components. However, @catch argues in #3519352-57: Content templates, part 3b: store exposed slot subtrees on individual entities that the exposed slot is the more important use case for which to ensure robust data integrity, because those can get deleted through normal UI interaction, whereas the other cases are only triggered by code deployments, for which there's other options available for managing those (for example, the ability to run a longer running process to perform the update or simply choosing to not deploy code that breaks things you don't want to break).

If we decide against the field-per-exposed-slot idea, then another way to manage deletion/purging could be:

  • Add an orphaned_reasons (or other name TBD) column. Increment it whenever the component instance is orphaned by a slot deletion (whether an exposed slot or a non-exposed slot) or a component deletion affecting itself or an ancestor. Decrement it whenever the slot or component that got deleted gets restored (if we decide to allow such restorations).
  • When orphaned_reasons goes from 0 to 1, set the deleted column to 1. When orphaned_reasons goes from 1 to 0, set the deleted column to 0.
  • Leverage existing or add new logic for not loading and for purging field items whose deleted=1.
🇺🇸United States effulgentsia

I wonder if we could solve this use case by adding a |wrapChildren filter. E.g., the example in the issue summary could then be:

<div class="example">
  {{ items|wrapChildren(item => '<div class="example__item">' ~ item ~ '</div>') }}
</div>

We could implement the wrapChildren filter to act on each item before applying the item's #prefix and #suffix, and the above ensures that items itself is rendered and not just iterated.

@luke.leber: Would something like this meet the use cases you're aware of, or is there more complex logic than what Twig allows inside an arrow function that you'd need?

🇺🇸United States effulgentsia

@luke.leber: I'm surprised that the dirty workaround in the issue summary works. Are you sure that it does? As in, can you drag and drop into that slot? I think there are two potential issues here...

  1. The slot itself (not just its children) needs to be rendered somewhere. In other words, the issue summary's workaround would make sense to me if it ended with {% endfor %}{{ items }}</div> instead of {{ item }}{% endfor %}</div>.
  2. There are markers (HTML comments) in each item's #prefix and #suffix and those need to be direct children of the slot in the rendered output. For this, the dirty workaround in the issue summary addresses that by putting the desired wrapping element "inside" the existing #prefix and #suffix rather that "outside" of it.
🇺🇸United States effulgentsia

I agree with #13 and #14. Also, I think we can do Add way to "intern" large field item values to reduce database size by 10x to 100x for sites with many entity revisions and/or languages Active in a way that's upgrade path friendly. For example, adding the interned column but keeping the old column around until all old records have been updated.

🇺🇸United States effulgentsia

Keeping the same set of nodeTypes but adding more metadata was proposed by @effulgentsia.

+1 to using existing components and slots and just adding flags!

Although that was my original proposal earlier this week, I changed my mind since then, and @wim leers, @penyaskito, and I met earlier today and converged on adding two new nodeTypes: switch and case to supplement the 3 that XB already has (region, component, and slot) for a total of 5. The reasoning for this is that the concept of "show only one of these depending on some logic" isn't limited to just how the resulting page gets rendered, but rather this switch/case concept needs to be integrated into the whole XB UI:

  • The layers panel needs to show only one of the variants at a time.
  • A chip needs to be added to the overlay for the active variant whenever a descendant is selected.
  • Clicking on that chip needs to open an entire React-driven (not Drupal-driven) UI in the right sidebar for configuring all of the variants and the mapping from the personalization segment to the variant.
  • From that right sidebar, you can click "Edit" on a variant that's not the one that should be shown to your currently selected audience segment, and doing so switches the canvas to a focus-mode editing experience, similar to editing global regions, where you can only work with that variant's subtree and nothing else that's on the page.

All of that combined, but especially those last two, is a lot of logic that's specific to the concept of "here are multiple subtrees, optimize the UI for working with only one at a time, and managing which one should be active when". Having the React code implementing all of that logic based on nodeType == 'component' && type == 'personalization.wrapper' feels like breaking the simplicity of the React app basing its logic on the nodeType and leaving the details of the component "type" to the back-end.

The idea of nodeType == 'component' && mutuallyExclusiveSlots has some appeal as the "flag" to the React app to do all the "optimize the UI for working with only one subtree at a time" things, but the downside with this is it implies that on the Drupal side, this will be managed as a single component with multiple (dynamic based on how many variants got created in the UI) slots. That might or might not be the desired back-end implementation. In fact, so far we're leaning towards a component-per-variant plus a component-for-the-group approach in order to avoid the concept of dynamic slots. Whether or not we stick with that or end up reconsidering the dynamic slots idea ideally should not require changing the client-side structure to have to match it.

And that all leads us to the idea of adding two new nodeTypes to be explicit about the new functionality being added to the UI without being coupled to how that ends up getting stored on the server.

With that, #5 becomes:

  "nodeType": "switch",
  "id": "UUID-of-the-Personalization-Wrapper",
  "type": "personalization",   // 💡 In the future, could add other types, such as perhaps "breakpoints".
  "mapping: {
    // 💡 Show nothing to these segments.
    "us": null,
    "uk": null,

    // 💡 The default variant. Shown to all segments not otherwise included in this mapping.
    "*": "UUID-of-a-Variant",

    // 💡 Show a different variant to these segments.
    "finland": "UUID-of-another-Variant",
    "norway": "UUID-of-another-Variant"
  },
  "cases": [
    {
      "nodeType": "case",
      "id": "UUID-of-a-Variant",
      "components": [
        // One or more components to show for this variant.
        ...
      ]
    },
    {
      "nodeType": "case",
      "id": "UUID-of-another-Variant",
      "components": [
        // One or more components to show for this variant.
        ...
      ]
    }
  ]
}
I have spotted a potential scenario, though, that means that perhaps mutuallyExclusiveSlots is not the correct name for the flag! In the above example an audience member in the UK might expect to see both variants at the same time and thus those are not mutallyExclusiveSlots but rather conditionalSlots.

We clarified with @lauriii that this isn't what's desired in terms of design. Any given segment should only map to one variant (or to None), but multiple segments can map to the same variant. That's why in the above I moved from segments on the variant/case nodes to mapping on the wrapper/switch node.

However, I do think there is a need for the mutallyExclusiveSlots and that is when the purpose of the variant is A/B testing. Where a site author wants 50% of their audience to see one variant and the other 50% to see the other. So perhaps both flags are needed?

A/B testing is out of scope for the initial version of this. But in the future, we can expand mapping to accommodate it. There's probably multiple options for how to model that, but just as one naive example:

"mapping: {
    "us": null,
    "uk": null,
    "*": "UUID-of-a-Variant",
    "finland": "UUID-of-another-Variant",
    "norway": {
      "UUID-of-a-Variant": 0.5,
      "UUID-of-another-Variant": 0.5
    }
  },
🇺🇸United States effulgentsia

I agree with the conclusion here that any further work on this can wait until after 1.0. However, a few minor points I want to capture here while I'm thinking of them:

[#22.1] seems something we might do but it also seems like a new API to specify attributes outside of the schema.

I interpret this as meaning that it would not be part of the SDC's YAML, and instead be something "owned" by XB. For example, it might be extra info on the Component config entity. In other words, SDCs just say "here are some examples" and it's not their concern whether those are treated as initial content, placeholder content, fallback content, etc. People administering XB would be the ones deciding what behavior they want for what gets done with the examples. For example, someone using XB to create a demo site for a DrupalCon presentation might want examples to be the initial value, and stored with the content, as per the current behavior. But someone making a corporate site for production might want SDCs' example values to help give some guidance to the content creators, but not get accidentally leaked to the published site.

implement case 4 later, and use SDC's default for that. That would mean that if the SDC changes fromdefault: 'foo' to default: 'bar', that all existing instances of that SDC rendered using XB would start showing "bar" (unless the user entered a different value, then the user's value continues to show). That'd be achieved by comparing the value in the StaticPropSource with the default: … in the SDC, and if they match, we would not store it in the component tree.

I think yes to using the default keyword in the json schema for this for when we want to implement it, and to not implementing this until later, probably not until after XB 1.0. However, I don't think the logic/UI should be "does the value in the form happen to match the default", but rather I think we'll want to implement something more explicit like wrapping/extending the widget to give the content editor a choice to either fill in the decorated/base widget or choose a "Use the default" option, or something like that.

🇺🇸United States effulgentsia

By the way, Layout Builder doesn't let you add the page title block either, with an @todo pointing to 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Needs work .

Personally, I don't think we need to support the page title block until that issue is fixed. Instead, I think we can:

  • Add the page title into drupalSettings. E.g., perhaps via a hook_preprocess_html()? Then code components have access to it easily, so you could just implement a code component that does <h1>{drupalSettings.pageTitle}</h1> or similar.
  • Later, when we add (UI) support for dynamic prop sources, we could make the page title available to SDCs, such as to a regular Heading component, as the value of whatever string prop gets linked to the page title.

Then, when 🐛 PageTitle block is non-functional when not handled directly by \Drupal\block\Plugin\DisplayVariant\BlockPageVariant Needs work is resolved, we can make the page title block available for people who really want to do it as a block, though if the above two options are available, what's the point of the block?

Just my 2 cents.

🇺🇸United States effulgentsia

#32 shows an example that only includes scalar values, but it would work for objects/arrays as well. For example, if you have an input that's an array of links, you could still remove the superfluous value key. I.e., instead of:

{
  "links": {
    "value": [
      {
        "uri": "https://example.com/page1",
        "options": []
      },
      {
        "uri": "https://example.com/page2",
        "options": []
      }
    ]
  }
}

you could have:

{
  "links": [
    {
      "uri": "https://example.com/page1",
      "options": []
    },
    {
      "uri": "https://example.com/page2",
       "options": []
    }
  ]
}
🇺🇸United States effulgentsia

Comment #32 is based on the assumption that we're versioning the union of the inputs, not each input individually, and therefore the "version" (or "field union ish" config entity reference) would be pulled out into its own column, but please correct me if that's an incorrect assumption.

🇺🇸United States effulgentsia

Now that 📌 [PP-1] Consider not storing the ComponentTreeStructure data type as a JSON blob Postponed is in, it would be helpful if the before and after examples in this issue summary were updated to be based on the new schema that landed in that issue.

AFAICT, the "after" in this issue will result in the inputs column being reduced to containing only the value key for each input, is that correct? For example, for an instance of a heading component with 3 props, the inputs column would look like:

{
    "text": {
        "value": "Example heading",
    },
    "style": {
        "value": "primary",
    },
    "element": {
        "value": "h1",
    }
}

If I'm understanding that correctly, then would it be reasonable for the scope of this issue to also include removing the value key since it would become superfluous? In other words, could we simplify the above to:

{
    "text": "Example heading",
    "style": "primary",
    "element": "h1"
}
🇺🇸United States effulgentsia

I'm glad I found this issue; there's some great info in here. We have a use case for this in Experience Builder where we're exploring how to make a nice data fetching API available to JS Code Components.

I'm curious if https://www.drupal.org/project/openapi combined with https://openapi-ts.dev/ has been considered for this. If it hasn't been evaluated yet, should it be? If it already has been, what were its shortcomings? Is #11 a better way to go than that?

🇺🇸United States effulgentsia

I really like the option 2 proposal that's in the ADR. My one nit though is I don't think we should do it as a custom hook, but instead as a plain async function that's passed to React 19's use() API. In other words, instead of:

import { useMenuData } from '@/lib/menu';

const NavigationMenu = ({ menuId, level }) => {
  const menuData = useMenuData(menuId, { level });
  // ...
}

to do:

import { use } from 'react';
import { fetchMenuData } from '@/lib/menu';

const NavigationMenu = ({ menuId, level }) => {
  const menuData = use(fetchMenuData(menuId, { level }));
  // ...
}

This would then also work for other UI frameworks when we add support for them in the future, which we might do soon as part of the CLI work. For example, in Svelte:

{#await fetchMenuData(menuId, { level }) then menuData}
...
{/await}

One hiccup with this approach though is that preact/compat doesn't implement the use() function. I opened https://github.com/preactjs/preact/issues/4756, but we can't count on that getting done in time. However, I think we can work around that by implementing our own use() function until such time as Preact implements the proper one, and ours can sidestep the need to work with Suspense, because we can rely on either server-side prefetching or async Astro island hydration to make sure we don't hydrate the component until we've already fetched the data and can return it synchronously by the time use() is called.

🇺🇸United States effulgentsia

and then we'll change all of our SDCs to reference it?

Maybe that won't be necessary since SDCs reference the schema in Drupal core, so we'll be able to update just that central one when the time comes?

🇺🇸United States effulgentsia

x-formatting-context is an example of such workaround, because we are asking the component author to add an annotation related to the UX of a specific display building tool he doesn't have to care about, instead of being focused only on its component's own UI model and logic

My reply to this is tangent to this issue, but I do want to note that I disagree with this characterization. If an SDC defines an HTML-containing prop, and then has Twig code that renders that prop inside a <p> tag or has CSS that assumes that what's in that prop is only inline formatted content, then it's the component's own UI logic that dictates x-formatting-context: inline.

🇺🇸United States effulgentsia

That's why I like meta:enum. Yes, it is not part of the JSON schema specs, however...it is not something we have invented, it is already used in opens source tools

But is that future-compatible considering https://json-schema.org/blog/posts/the-last-breaking-change? When a stable json schema version is released and tools adopt it, are we expecting https://github.com/adobe/jsonschema2md to provide the vocabulary/schema for it, and then we'll change all of our SDCs to reference it?

If we do stick with meta:enum given its prior art, then what about translation context? If that's one that we are inventing here and not copying from other OSS tools, then I think that one needs an x- prefix given JSON schema's move away from unknown keywords not prefixed with that. Perhaps x-translation-context?

🇺🇸United States effulgentsia

@catch: sorry, i can see how my comment of "I'm not aware of any other AI-generated MR in the XB issue queue at the moment" could be read as questioning your "This is the second issue" comment. That's not how I intended it. I'm aware of Add automated image optimization to image component Active , but the MR currently on there is no longer the initial AI-generated one, so my comment just meant that there's no others in the queue right now for me to re-title, as far as I know.

As to your other question, I think the issue summary was written by @grasmash (or him in collaboration with other people), but from a Product Management rather than Engineering perspective, and from a "this is the desired feature-set / capabilities" perspective rather than a "here's what Drupal building blocks already exist, here's the path to get from them to the desired UX" perspective. I agree with the "Needs issue summary update" tag to get closer to the latter, but I find the currently written Product-oriented write-up a necessary and helpful starting point from which to get to the Engineering-oriented one.

🇺🇸United States effulgentsia

Re #6, the Overview tab of the MR says:

🤖 Generated with Claude Code

But yeah, that's easy to miss if you just click the MR link from the issue and then immediately click the Changes tab (which is what I do >90% of the time) without reading the Overview tab.

Therefore, I'm adding the disclaimer to the issue title.

I wonder if we should add more specifics to https://www.drupal.org/docs/develop/issues/issue-procedures-and-etiquett... to standardize on how to disclose this info. Should it be in the issue title? The issue summary? A label on the MR? I think a label on the MR would be great if d.o. added MR labels to the MR link that's at the bottom of the issue summary.

🇺🇸United States effulgentsia

we've had responsive image module in core for about ten years, it relies on the picture element, which Opera Mini does not support

Is this actually buggy in practice? Or does Opera Mini just ignore the picture element and fall back to the child img element?

what do you think the concrete consequences have been of 'supporting' Opera Mini during this time, given it's never tested as part of a browser matrix

Do you mean never tested by people? Or not by automated tests? In either case, is there a specific technical barrier that makes testing with Opera Mini harder than testing with other browsers?

what do you think the consequences would be of dropping support?

Given our track record in reality of not having tested with it, probably functionality on Opera Mini will keep on degrading the same way regardless of whether we say it's supported or not. So in that sense, saying it's not supported would be more honest. Unless we decide to actually support it for real by including it in our test matrix. However, I don't like the messaging of saying we're dropping support due to its usage being below the threshold we care about if in reality the usage is actually above that threshold. Especially if it's the case that the usage is high (if you believe Opera's numbers which I don't know if we should) due to it addressing a real pain point (low bandwidth or expensive bandwidth in certain countries) that other browsers don't address. However, if we have other reasons for dropping Opera Mini: e.g., technical barriers to us being able to test with it, or a level of missing web standards support that makes it impractical for Drupal sites to work well with it, or similar, then I think it's reasonable to drop support. E.g., if our policy in 🌱 [policy, no patch] Define usage heuristics for browser support Active isn't just about usage, but also includes that a requirement that for us to support a browser, the browser must sufficiently implement web standards, and provide a reasonable way to test with it. Or alternatively, if we want to keep it strictly usage based, then I think we need a way to ascertain which data source is most trustworthy when there's an order of magnitude disparity between them.

🇺🇸United States effulgentsia

One idea for fixing #13 is to use the resource timing API. In other words, instead of swapping the iframe when the load event fires, delay the swapping until both the load event fires and also all URLs referenced by rel="modulepreload" links are in the resource timing list and have a responseEnd timestamp.

🇺🇸United States effulgentsia

Check the iframe srcDoc for rel="modulepreload" links. Do browsers delay the iframe's load event until these are all loaded? If not, that would explain why js components might not be rendered when XB swaps the iframes.

From https://html.spec.whatwg.org/multipage/links.html#link-type-modulepreload: A user agent must not delay the load event for this link type.

I think this is very likely the culprit, especially if some of the JS assets are in auto-save storage, meaning their URL needs to do a full Drupal bootstrap and isn't cacheable.

🇺🇸United States effulgentsia

@jessebaker: If you start digging into #11 for JS components, here's a few ideas I can think of that might help:

  • The Astro hydration library ideally should run sooner. I commented on #3509357-10: Working with grids with slots is difficult due to missing Astro CSS about that.
  • Check the iframe srcDoc for rel="modulepreload" links. Do browsers delay the iframe's load event until these are all loaded? If not, that would explain why js components might not be rendered when XB swaps the iframes. But if browsers do delay the load event until those are all preloaded, then the question is: are there any JS files being downloaded that aren't in that preload list and therefore not blocking the load event?
  • Because Astro's hydration uses async functions that await on modules getting imported, even if the iframe's load event is being correctly delayed until those modules are preloaded, I wonder if there's still a timing issue where the iframe's load event is fired before all those async promises are resolved. In which case, would adding a setTimeout(0) between the iframe's load event and the swapping be enough to ensure those promises get resolved first?
🇺🇸United States effulgentsia

I recommend renaming client.css to hydration.css, since it's part of the hydration library, not the client library. Note that the hydration library is independent of UI framework, whereas the client library is specific to Preact.

Also, as part of this issue, can we add header: true to the astro.hydration library, similar to how we do for the xb-ui library? That will ensure that the <astro-*> custom elements begin in an upgraded state, reducing layout shift on the actual site, and possibly also helping with 🐛 Overlay and iFrame flicker on updating props values Active while editing within XB.

🇺🇸United States effulgentsia

All the issues in the issue summary are done, so marking this RTBC.

There is 🐛 Code editor preview doesn't include CSS for dependencies of dependencies Active still left and possibly other related issues not yet listed here or found?

🇺🇸United States effulgentsia

This looks good to me, but there's a failing E2E test that looks like it could be related to this MR, not just a flaky test.

🇺🇸United States effulgentsia

This press release from this month is still making the claim that Opera Mini has 100M users worldwide. Note that's a decrease from claims about 100M users in Africa.

It's possible that they're making a false claim, but if it's true, that's still 2% of the global internet population, and likely a significantly higher percentage in Africa if that's where they have a higher than average concentration.

🇺🇸United States effulgentsia

Assuming ~6 billion people on the internet, 0.05% is 3 million. That's very different from #6's claim of 100 million, and #7 doesn't explain that discrepancy unless we're thinking people are averaging downloading Opera Mini 30 times. https://en.wikipedia.org/wiki/Opera_Mini says that in 2013, it was 300 million active users, though I'm guessing that's worldwide, not just within Africa. It would be nice if we could find some corroboration for either the claim that that has fallen by 100x down to 3 million, or conversely the claim that there's currently 100 million people in Africa using it.

Production build 0.71.5 2024