adobe / aem-react-editable-components

SPA React Editable Components for Adobe Experience Manager
Apache License 2.0
61 stars 28 forks source link

Change in Container props structure for ChildComponent #129

Closed iamdavidjackson closed 2 years ago

iamdavidjackson commented 2 years ago

In version 2.0.0 the Container component was updated to pass props to child components using the "model" prop. This is quite inconvenient because no components that worked with version 1.1.10 can't be reused with version 2.0.0 without changing the props that they are expecting.

This is what the logic looked like in version 1.1.10 where you can see that itemProps are being spread into the ChildComponent.

https://github.com/adobe/aem-react-editable-components/blob/1f98fdf2bd63f8523fa2a724fe04e22c0e85a93b/src/components/Container.tsx#L84

return <ChildComponent {...itemProps}
   key={itemPath}
   cqPath={itemPath}
   isInEditor={this.props.isInEditor}
   containerProps={this.getItemComponentProps(itemProps, itemKey, itemPath)}/>;

This is what the logic looks like in version 2.0.0 where you can see that the itemProps are passed in as model.

https://github.com/adobe/aem-react-editable-components/blob/master/src/components/Container.tsx#L61

<ItemComponent
    key={itemPath}
    model={itemProps}
    cqPath={itemPath}
    className={itemClassNames}
    removeDefaultStyles={props.removeDefaultStyles}
  />

Is there a benefit to doing this that would justify having to update every components props with a new nested model prop? Seems like a huge amount of rework that could be resolved by just spreading the itemProps like the previous version.

iamdavidjackson commented 2 years ago

Can anyone please comment on this? This seems like a significant breaking change that could be easily eliminated. I'm actively working on an implementation of this library for a client and need some feedback on this.

iamdavidjackson commented 2 years ago

Ok I did some more research and I see that components are now required to be wrapped in an EditableComponent to be able to be used with the new version of the library. In my implementation the components are shared between NextJS and the AEM authoring instance so I would need to wrap all my components with the EditableComponent in AEM but not in NextJS. In the previous version the Container component took care of all this editable wrapping which I think made a lot more sense.

This will also cause some problems when Typescript users take this approach:

https://github.com/adobe/aem-guides-wknd-spa/blob/React/2.0.0/ui.frontend/src/components/Text/Text.js#L60

This example shows the same props being passed into the EditableComponent and the Text component but the types will definitely not match as Text is looking for a flat prop structure and EditableComponent is looking for the text props to be on the model parameter.

mitchross commented 2 years ago

I am in the same boat....

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 2.0.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: