Not all components have UUIDs, so rename to ID

Not all components have UUIDs

Which \Drupal\experience_builder\Entity\Components do not have UUIDs? 🤔

I think this MR is more broad than it needs to be.

• Components must be instantiated, so they can only be identified using something unique: a UUID.
• Slots (all kinds: SDCs' slots and theme regions, which conceptually are also slots) do not have UUIDs. Each slot has a name.

Components and Slots are two different concepts, why is it not okay for them to heave different identifiers?

(And yes, a slot instance is then identified by its containing component instance's UUID + the slot name. The only exception would be the "top-level slots", i.e. those at the PageTemplatelevel.)

DM'd with @longwave.

IOW, the client side's layout tree contains 3 types of nodes:

1. component nodes are identified by uuid, because they correspond to Components (\Drupal\experience_builder\Entity\ComponentInterface instances) which have UUIDs
2. slot nodes are identified by a [component UUID, slot name] pair, conveyed as a single id value that contains both
3. region nodes (needed for 📌 Add support for global regions Active which this blocks) are identified only by a name

After discussing with Wim I think the above is correct. Unless there is a good reason not to, the front end will have to start considering slot and component nodes separately, which is safer and more correct - we should never be able to end up with a component containing another component or a slot containing another slot. A slot always has a name and belongs to a parent component, which is enough to uniquely identify it. So (ignoring regions for now) I think this can be represented in TypeScript as

export interface ComponentNode {
  nodeType: 'component';
  uuid: UUID;
  type: string;
  slots: SlotNode[];
}

export interface SlotNode {
  nodeType: 'slot';
  name: string;
  components: ComponentNode[];
}

This does complicate functions such as findNodeByUuid() which now has two issues: it can no longer find slots, nor can it recurse easily as it needs to know about child structures. But this is solvable: we likely need to refactor it into findComponent(uuid) and findSlot(uuid, name) - and findSlot() can call findComponent() first and then retrieve the specific slot.

@jessebaker I realise this likely causes significant rework on some of the frontend code and data structures, but what do you think of this proposal overall?

In fact regions are just a special case of slot, they only appear at the top level, maybe we only need this:

export interface RegionNode {
  nodeType: 'region';
  name: string;
  components: ComponentNode[];
}

Regions can contain components, components can contain slots, slots can contain components, but no other combinations are allowed.

I agree with everything in #5, #6 and #7.

This will require a lot of refactoring on the front end but as @longwave says this is solvable and I do think is the correct path forward.

In fact I think there are already assumptions on the front end that components can only contain slots and slots can only contain components already, it's just not explicitly stated/defined in TS or anything. E.g. In ComponentOverlay.tsx I loop over component.children and for each one I render a <SlotOverlay />. In turn the children in SlotOverlay.tsx are all rendered as <ComponentOverlay />s.

I agree with everything in #5, #6 and #7.

This will require a lot of refactoring on the front end but as @longwave says this is solvable and I do think is the correct path forward.

In fact I think there are already assumptions on the front end that components can only contain slots and slots can only contain components already, it's just not explicitly stated/defined in TS or anything. E.g. In ComponentOverlay.tsx I loop over component.children and for each one I render a <SlotOverlay />. In turn the children in SlotOverlay.tsx are all rendered as <ComponentOverlay />s.

longwave → changed the visibility of the branch 3491013-not-all-components to hidden.

I think the title summarises it better. The draft MR!438 makes the minimal server side changes required and will probably fail a lot of tests. I will fix up the server side where it breaks and hand over to @jessebaker to rework the client side.

In going through and trying to implement this I have consistently hit a pain point in working with the data formatted as described in #6 and #7

Without going into huge details about the painpoint I'm hitting, I think @wim leers suggestion in #5 to give each slot an id (changed from uuid) of 'id' => $component_instance_uuid . '/' . $slot_name, will go a long way to fixing it. In simple terms, it will allow me to specify a specific slot without having to always keep track of the slots parent when recursively traversing the tree.

So I think that would be:

export interface RegionNode {
  nodeType: 'region';
  name: string;
  components: ComponentNode[];
}

export interface ComponentNode {
  nodeType: 'component';
  uuid: string; // unique uuid
  type: string; // corresponds to the drupal component id e.g 'sdc.experience_builder.druplicon'
  slots: SlotNode[];
}

export interface SlotNode {
  nodeType: 'slot';
  id: string; // made up of the parent components uuid and the slot's name.
  name: string;
  components: ComponentNode[];
}

From there, potentially one other QoL change for the front end would be to normalise 'uuid' on components and 'id' on slots to both be 'id' in the JSON but that is not critical and perhaps the clarity of giving them the correct names is more important.

@longwave in #6: → you mean what I wrote in #5 I think?

@longwave #7++ ("regions are a special case of slots")

@jessebaker in #8: YAY! Music to my ears! 😄 And the presence of those assumptions in the front end is indeed unavoidable, necessary and sane, which is exactly why I felt #5 would be a sensible step. Very glad to see you agreeing so strongly :)

@jessebaker in #13:

1. In simple terms, it will allow me to specify a specific slot without having to always keep track of the slots parent when recursively traversing the tree.

   Why can't findSlot(uuid, name) use findComponent(uuid), like so:

   export function findSlot(uuid: string, name: string): Node | null {
     const component = findComponent(uuid);
     // missing error handling for invalid name
     return component.name;
   }
   

   ?

2. perhaps the clarity of giving them the correct names is more important.

   That's my current thinking, and my hope 😊🤓

YAY, all tests are passing, and it's now down to 15 TypeScript type checking errors! 🚀

I see @longwave just implemented what @jessebaker proposed in #13. If it helps us land this faster, then I'm in favor! 👍

But I'd like to understand why it's necessary. I bet it's something to do with state tracking? A comment would go a long way 😊🙏

Detailed review posted: 13 remarks with a summary 🏓

I added a

3491013-but-with-ts-muted

branch which (as of rn at least) is the same a the primary MR, but with a bunch of ts-ignores so e2e tests can run and help us identify other places the FE code needs to change. I had hoped to find a few places to fix in the process but aside from fixing a few ts bits (and avoiding a few additional ts-ignores), none of the fixes were particularly obvious (@jessebaker has said as much). Hope the e2e-running branch at least helps things a bit.

Merged 0.x, copied and updated the relevant data-model.md changes from 📌 Add support for global regions Active

I reviewed in-depth and RTBC’d — now this just needs @travis.carden’s approval for the /openapi.yml changes — @travis.carden, IMHO this is a big step in the right direction, although it’s not yet perfect.

But … given that the changes here block several critical issues (see related issues), I decided to not wait for @travis.carden to approve. The changes here are around refactoring the client-side data model, which affects some request & response bodies, and this MR definitely updates /openapi.yml accordingly.

There is this:

+  // TODO refactor of this file should remove this ts-ignore once Slots and Component rendering is correctly separated out.

… and in that MR, we'll be perfectly positioned to split /openapi.yml's LayoutSlot into LayoutRegion and LayoutSlot.

@travis.carden, if/when you approve, please go ahead and merge this too, because per @alex.bronstein, this is blocking a lot of things: https://acquia.slack.com/archives/C06M3JQJG1M/p1733849815358459

All test failures are failures on 0.x too, all happening due to upstream changes in Drupal core 11.1.x. So, went ahead and merged this, bypassing @travis.carden's approval for reasons cited in #21.

Epic work, all of you, but especially @jessebaker! 👏