Plan how to evolve code component overrides

balintbrews → credited aaronmchale → .

balintbrews → credited gábor hojtsy → .

balintbrews → credited lauriii → .

balintbrews → credited longwave → .

balintbrews → credited wim leers → .

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

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.

Reviewing at last 😬

Sorry for the long wait. Based on DDD discussions, I expected this to be much bigger than it turns out to be, I should've reviewed this sooner 🙈

@effulgentsia's remarks in #10 are squarely on the front-end implementation side of this new component source plugin, which I have no strong opinions about.

I reviewed this from a XB data model + component source architecture + reliability + ecosystem reach POV.

The ADR simultaneously does not go into detail about the challenges of the most common use case (menu blocks) and also doesn't really explain how we'd make other use cases (think: arbitrary other block plugins such as the branding block). We discussed much of this at length in person at DDD Leuven.

I think this ADR is a great start, but I think there's a few crucial things that are too open-ended for this ADR to be mergeable. I'm happy to be overruled if all @balintbrews, @longwave, @effulgentsia and @lauriii all think this is sufficiently precise. But I would like to see more detail, because at least to me it's not clear how this proposes to implement "block overrides" for e.g. the breadcrumbs or site branding blocks. 😇🙏

• ✅ Always passing the entire menu tree to the component is not good enough: different menus want to show different slices or subtrees of a menu — for example only the top level, only the first 2 levels, starting at the 3rd level etc.

  The obvious solution to support parametrization: a HTTP API to fetch menu links, for which Drupal core has the → endpoint.

  👉 The ADR covers this: +

• ❌ The ADR does not explain the code component developer/author DX for knowing the shape of the data they would receive.
• ❌ The ADR does not explain how the breadcrumbs block would work in option 2. Drupal has no HTTP API to fetch the results of a service (the breadcrumb service in the server-side container). Is building such a HTTP API part of the envisioned scope?
• ❌ The ADR does not explain how the site branding block would work in option 2. There's no HTTP API to fetch (simple) config (the site name + slogan). Core's JSON:API and REST modules both do not support that. Is building such a HTTP API part of the envisioned scope?
• 🤔 The ADR does not state whether this would be a new component source plugin, or whether it's just the existing "code component" source but with additional APIs (for data fetching) available. If it's the same component source plugin, how will we ensure that the data-fetching code components end up in the UI library? (See \Drupal\experience_builder\Entity\Component::computeUiLibrary().)

  Can you confirm that the intent in all 3 options is to implement a new "block override" ComponentSource plugin, which just happens to share a lot of infrastructure with the existing "code component" ComponentSource plugin, but is about data fetching (and hence ) instead of providing props/slots?

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

Productive meeting with @balintbrews just now 😄 He'll add great clarifications on the ADR 😊👍

Merging the ADR is a good first step. But also, we should remove all traces of code component overrides now that we have a better solution. And best to do that before beta to not have to deal with an upgrade path for it.

#15: totally! Happy to do that as a secondary MR on this issue. So just did that 👍

Yay! Looks like all PHPUnit tests are passing, except for the one I know needs updating (it's become kinda tightly coupled to a particular block_override code component — I need to untangle that later).

For some reason, Cypress unit & e2e tests are refusing to run though? 😅 That seems to be an npm infra problem though, because installing Cypress still isn't done after >10 minutes 🤪

Working on the test

Pushed a version that works, we will replace it with a real component that uses drupalSettings in the future, its not relevant for this test.

Thanks, very kind — this wasn't urgent, but it's nice to see this 99% green now 🥳🙏 (Only Playwright expectations still need updating.)

FYI: I opened 📌 Build Breadcrumb and SiteBranding code components Needs work as a child issue of this one, and re-parented 📌 Build a navigation code component Active to be a child issue of this one as well.

Thanks! Still missing an issue for a "page title" code component, but @balintbrews thinks that should be done all in one go, so in 📌 Build Breadcrumb and SiteBranding code components Needs work .

#23: Good catch! I adjusted 📌 Build Breadcrumb and SiteBranding code components Needs work to cover that.

📌 Build a navigation code component Active landed, so now we're down to just 📌 Build Breadcrumb and SiteBranding code components Needs work !

📌 Build Breadcrumb and SiteBranding code components Needs work is in, rebasing…

Tested the removal on the UI and in the CLI. Everything works as expected. Leaving the issue as NW to finish up the ADR, unless you prefer otherwise, Wim.

The ADR isn't beta blocking, so untagging.

Leaving the issue as NW to finish up the ADR, unless you prefer otherwise, Wim.

👍