plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
446 stars 606 forks source link

Refactor: Change content types to interfaces to make them extendable #6191

Closed tomschall closed 1 month ago

netlify[bot] commented 1 month ago

Deploy Preview for plone-components canceled.

Name Link
Latest commit 708fd5d5c0a9bd0f0553432be4ed970c2d0c336d
Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66a3a42fb042120008cde30a
JeffersonBledsoe commented 1 month ago

@tomschall Can we not already achieve extending through the use of union types and intersections? E.g. type ExtendedImageScale = ImageScale & { custom_field: string };

ichim-david commented 1 month ago

@tomschall I think that @JeffersonBledsoe makes a good point and I suspect that @sneridagh will say the same thing. One article that ranks high is this one where the author prefers type in order to make it more obvious when you have something "extended" https://www.totaltypescript.com/type-vs-interface-which-should-you-use.

I think this kind of pull request is a good candidate to be an issue first maybe with the discussion label and only after some feedback should it be followed through by a pull request.

This avoids putting in the work that might not be merged due to the current solution being preferred.

sneridagh commented 1 month ago

@JeffersonBledsoe @ichim-david I've asked Thomas to make this change.

The gist goes like this: You can't extend type declarations, but you can extend interfaces.

In general, all items subject to be customizable/extendable in the backend should be interfaces, so you can extend the types properly in your project. There are some that need to be amended in @plone/types.

This is an example from one of my projects:

// We extend the block types with the custom ones
declare module '@plone/types' {
  export interface Content {
    background_color?: Color;
  }

  export interface SettingsConfig {
    backgroundColors: Color[];
    blockWidths: Array<StyleDefinition>;
  }

  export interface BlockConfigBase {
    colors?: Color[];
    allowedBlocks?: string[];
    presets?: Array<Preset>;
    containerToolbar?: React.ComponentType<any>;
    presetComponent?: React.ComponentType<any>;
    editBlockWrapper?: React.ComponentType<any>;
  }

  export interface BlocksConfigData {
    landingPageTitle: BlockConfigBase;
    separator: BlockConfigBase;
    heading: BlockConfigBase;
    masonry: BlockConfigBase;
  }
  export interface BlocksFormData {
    variation?: string;
    headline?: string;
    placeholder?: string;
  }

  export interface WidgetsConfigByWidget {
    BackgroundColorWidget: React.ComponentType<any>;
    blockWidth: React.ComponentType<any>;
  }
}

This is called "declaration merging' I think, being this, the way to go. I haven't found any other way to proceed or extend types in TS.

Regarding using types/interfaces discussion is one that leads to nothing and nonesense to me, althogh Matt Pockock is my guy of reference in all things TS.

Once this is said, ContainedItem and RelatedItem should be extendable. I think that ImageScale and Image should not.

@tomschall can you make this amendment, so we only make interfaces ContainedItem and RelatedItem?

Thanks!

tomschall commented 1 month ago

@sneridagh @JeffersonBledsoe @ichim-david Hey guys, yeah sure, for me is everything good you decide. I can't say what's best for this project. I just know that this Declaration Merging possibility in typescript exists and that you can do that with interfaces. We did this already with other WidgetsConfigByWidget, so i thought that's the recommended way you guys do this in general in volto. Thanks anyway.

tomschall commented 1 month ago

@sneridagh Ah yeah sure i do the amendment.

ichim-david commented 1 month ago

@JeffersonBledsoe @ichim-david I've asked Thomas to make this change.

@sneridagh lol that information would have been nice to have from the start so that there was no doubt on what you think of this change and hence me asking your opinion.

@tomschall I hope you understood my comment, I think you did, and why I added it. In general, if we want to change something from how it works it's best to get some feedback on our idea before producing the change so that we don't end up with a working solution that would still not be merged due to perhaps different requirements that need to be kept in place.

Since you talked with Victor and he told you to add these changes then this was just fine as he was already aware and thinking it was a good decision.

Also thank you for your contribution, one at a time each of us helps this stack be more friendly and useful for all of us with every contribution given.