[META] Plan to allow editing props and slots for in-use code components

Crediting @tedbow for #3500043-3: Publishing code components.

Related: #3500074: Usage info for code components with unpublished changes. Because … it'd be 100% safe to change props and slots whenever as long as there are no instances yet. And thanks to #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation, that's now actually possible.

IOW: the conversation here needs to be forked along two paths:

1. 0 instances exist: anything is allowed
2. >=1 instances exist: care is needed

Note: a new required prop would actually be trivial to support. Because XB requires required props to have an example value!

Especially since #3518336: When a field instance used by a `ContenTemplate` is deleted, or a field-type providing module is uninstalled, replace affected `inputs` with default `StaticPropSource` just added ::getDefaultExplicitInput(), this would be easy to support.

Conceptually speaking, the needed changes would be:

1. ComponentTreeHydrated would have to change from
   

   $source->renderComponent($stored_explicit_input)
   

   to

   $source->renderComponent($stored_explicit_input + $source->getDefaultExplicitInput())
   

2. EITHER ValidComponentTreeConstraintValidator would need to be updated to relax from
   

   $component_source->validateComponentInput(
             inputValues: $stored_explicit_input,
             component_instance_uuid: $component_instance_uuid,
             entity: $host_entity,
           ),
   

   to also do + $source->getDefaultExplicitInput()

   OR

   \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer() should do + $source->getDefaultExplicitInput(), which would ensure that any edit to any component tree would automatically add new required props' default values.

Important for usability of Code Component creators.

Critical for XB's data integrity when we add support for changing props/slots.

Let's discuss and plan how we could improve this.

Initial plan posted! 🏓

@lauriii, please see the proposed resolution.

The scope of this issue is both props and slots but it looks like the issue summary is only talking about props. Adding the 'Needs issue summary update' tag to expand the proposed solution to include slots.

never allowing prop shapes to be changed, unless there's zero instances

I think we should allow this to be done. Prop data will be either converted to the new prop type (when possible) or deleted (when conversion is not possible) when component with the changed prop type is loaded.

Added child issues and updated the issue summary to reference them

Prop data will be either converted to the new prop type (when possible)

You can state that if you want, but that's not a trivial undertaking. Different field types might support the same prop shape, but might store data completely differently.

A number of field types for example use computed properties, and it's only the computed properties that can populate a certain prop shape … the stored data may not even look like it.

And that's just when the prop shape remains the same! You're talking about changing the prop shape, which is an even more difficult transition.

So -1 for including that in the scope here. That's #3463996: [META] [PP-1] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade territory, and would be a significant scope expansion even there.

or deleted (when conversion is not possible) when component with the changed prop type is loaded.

Huh?! 🤯 How is that not data loss? Can you please elaborate?

EDIT: and the current issue summary still also indicates that we won't support this:

• …
• 🔴 changing the shape of existing props (with instances) #3557271: [PP-2] BE: Support changing the data-type of a code component prop should always be disallowed; if not, existing component instances (possible to determine since #3457504: XB field type: calculate all dependencies, store them, surface in new Component "Audit" operation) would have invalid inputs, resulting in fallback markup informing the user/visitor about the breakage (since #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log)). We'd have to build a fairly complex UX to allow recovery from this; avoiding data loss would be hard technically and/or confusing in terms of usability.
• …

So perhaps I'm just misreading what @lauriii wrote here? 😅

I think it's fine to punt that feature to a follow-up issue which we may or may not prioritize depending on feedback. I don't think it's the most critical aspect of this issue. The reason I think we should consider doing that is to avoid data loss like you mention. However, we still should allow changing the prop type by first deleting the prop and then adding a new prop with the same name. It includes a delete action which makes it clear that data loss is involved.

.

.

.

Organizing list of issues in summary into sections.

.

.

.

#3556337: BE: Ensure we support adding a new optional prop to an in-use code component merged.

That MR that landed, wow … \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\JsComponentEvolutionTest is magnificent! Almost 1000 LoC, but that's necessary for maintaining data integrity and ensuring all of this is possible via both PHP an Canvas' internal HTTP API 👏

Beautiful work, @larowlan & @penyaskito!

#27 reverted (I think unintentionally) my issue summary organization from #26, so I reverted back to the issue summary from #26.

Removed the last section of the proposed resolution, some of which is outdated, and the rest a repeat of what's above it.

.

Still not clear if removing a required prop is allowed. By my tests on #3567413: When editing a Canvas entity with a component tree (e.g., page, template, or region), automatically upgrade component instance versions for cases where there's no impact on existing data, it's not completely covered. Maybe we missed an issue here? Or this is deliberately not supported? What should happen then if that happens in an SDC? I might be able to answer my question looking at \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\ComponentInputsEvolutionTest and \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\JsComponentEvolutionTest, which I didn't yet.

Still not clear if removing a required prop is allowed.

This should be allowed. From UX perspective it should be handled the same way as a deletion of an optional prop.

#3557272: BE: Support removing a slot from a code component landed, now just needs follow-ups created.

What remains to be done here? #3463996: [META] [PP-1] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade seems obsolete, but simultaneously weirdly is still open yet the issue summary marks it with 🟢?

Related: #3524751-10: [later phase] Component Source plugins: generalized support for schema changes of explicit inputs — that is still necessary for e.g. the Block component source.

Updated IS for #3463996: [META] [PP-1] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade. It's marked with 🟢 because the required changes for this META were done in the sub-issues (aside of changing shapes).

What's left here is 🔴 changing the shape of existing props, which still needs to be discussed., as implies potential data loss.

I'm tempted to mark both METAs as fixed, even if both have still open child issues.

Per #3463996-45: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade and #3463996-51: [META] When the field type, storage/instance settings, widget, expression or requiredness for an SDC/code component prop changes, the Content Creator must be able to upgrade, changing #3463996 from 🟢 to 🟡. The needle is also moving forward for changing the shape of existing props, see that first comment.