Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: Remove "experimental" designation #60884

Closed

Conversation

fullofcaffeine
Copy link
Member

@fullofcaffeine fullofcaffeine commented Apr 19, 2024

Note

Closed in favor of splitting commits into individual PRs. See the issue for a complete list.

Solves: #59418.
Follow-up to the first draft: #60837.

What?

Remove __experimental prefix from all "experimental" components, effectively promoting them to regular stable components. See the related issue for more context.

Why?

The strategy of prefixing exports with __experimental has become deprecated after the introduction of private APIs. Read more in the related issue.

How?

For each __experimental component:

  1. Exports it from components without the __experimental prefix;
  2. Keep the old __experimental alias export for backwards compatibility;
  3. Add a deprecation annotation above the old alias;
  4. Change all the code snippets in docs to reference the new non-experimental import (changing the actual imports across the Gutenberg app will be done in other follow-up(s));
  5. Add the component id to the PREVIOUSLY_EXPERIMENTAL_COMPONENTS const array in manager-head.html so that old experimental story paths are redirected to the new one.

TODO:

  • Discuss code smells 👃🏻
  • Add changelog entry(ies) about the removal the __experimental prefix. Perhaps Components: Remove "experimental" designation for <component> for each component would be the best changelog entry, semantically?

Testing Instructions

  • All checks should pass;
  • Unit tests and E2Es should pass;

@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Apr 19, 2024

There are a lot of files changed here, but they mostly follow the same pattern. I didn't want to create separate PRs for each component because:

  1. It'd spam the repo with 30 similar changesets;
  2. After each merge, we'd need to rebase the others and potentially fix conflicts each time.

I think it's better to have all of these very similar changes in a single PR. To make it easier to review, I compartmentalized each component changeset in a single commit (check the commit list).

This is still a WIP, and I must do another pass for each commit. Due to the repetitive nature of the changes, it's possible I might have missed something and there may be some silly copy-pasta mistakes 🙈

NavigatorBackButton as __experimentalNavigatorBackButton,
/**
* @deprecated Import `NavigatorToParentButton` instead.
*/
NavigatorToParentButton as __experimentalNavigatorToParentButton,
Copy link
Member Author

@fullofcaffeine fullofcaffeine Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mirka, Based on your comment here https://github.com/WordPress/gutenberg/pull/60837/files/ce4f6386c86c9f4b3a119888aa1ad1326f373ef4..51d389b4cb73811b2eed88aab931abf75f5486ed#diff-bef15a411502a2e04eb09ba2277b51079c8883c8cafd650fa7d1bb8873d03fb9, I'm wondering how we should approach this. It seems we should only deprecate the components and leave any helper functions as is? Does that mean I should have left the hook aliased to its __experimental alias? Should I revert this altogether?

By the way, I think this might call for moving those components as subcomponents (e.g Navigator.Provider, Navigator.Screen, Navigator.BackButton, etc)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we should only deprecate the components and leave any helper functions as is?

Yes, I think so. Hope we don't find anymore helper functions though 😅 use* hooks should be fine.

By the way, I think this might call for moving those components as subcomponents (e.g Navigator.Provider, Navigator.Screen, Navigator.BackButton, etc)?

Good point, this would be a great opportunity to do that 😱 It will be a little bit more work for our consumers to update their code from the __experimental, but we do indeed want to move to dot notation namespacing (Navigator.Provider) over "flat" namespacing (NavigatorProvider). Could you try it for one of the components so we can all check that there are no implications we're failing to consider?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we do indeed want to move to dot notation namespacing (Navigator.Provider) over "flat" namespacing (NavigatorProvider). Could you try it for one of the components so we can all check that there are no implications we're failing to consider?

Sure! Wil do 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started experimenting with it here; it'd allow consumers to import only the root component (the provider), and then the rest would come built-in as properties of the function component. I think this makes sense, as these components are building blocks for the same thing; they can't be used in isolation, by themselves (though some might be omitted, I guess, like one of the buttons; see the comment about tree-shaking below). I still need to solve some TS errors, though.

Assuming TS errors are solved, there are a few other things to discuss, like the naming of the root component and if doing this could negatively affect tree shaking the way it's currently setup (I don't know enough about the webpack setup to judge that, yet).

ToggleGroupControlOption as __experimentalToggleGroupControlOption,
/**
* @deprecated Import `ToggleGroupControlOptionIcon` instead.
*/
ToggleGroupControlOptionIcon as __experimentalToggleGroupControlOptionIcon,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToolsPanel as __experimentalToolsPanel,
/**
* @deprecated Import `ToolsPanelItem` instead.
*/
ToolsPanelItem as __experimentalToolsPanelItem,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TreeGridRow as __experimentalTreeGridRow,
/**
* @deprecated Import `TreeGridCell` instead.
*/
TreeGridCell as __experimentalTreeGridCell,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export { default as Icon } from './icon';
export type { IconType } from './icon';
export { default as IconButton } from './button/deprecated';
export {
/**
* @deprecated Import `ItemGroup` instead.
*/
ItemGroup as __experimentalItemGroup,
Item as __experimentalItem,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to de__experimetalize the Item here, though I wonder if I should even do it for subcomponents as per your comment in https://github.com/WordPress/gutenberg/pull/60837/files/ce4f6386c86c9f4b3a119888aa1ad1326f373ef4..51d389b4cb73811b2eed88aab931abf75f5486ed#r1570925958. Also see: https://github.com/WordPress/gutenberg/pull/60884/files#r1571626137. Any insights appreciated :)

BorderBoxControl as __experimentalBorderBoxControl,
hasSplitBorders as __experimentalHasSplitBorders,
isDefinedBorder as __experimentalIsDefinedBorder,
isEmptyBorder as __experimentalIsEmptyBorder,
BorderBoxControl,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tyxla
Copy link
Member

tyxla commented Apr 19, 2024

There are a lot of files changed here, but they mostly follow the same pattern. I didn't want to create separate PRs for each component because:

  1. It'd spam the repo with 30 similar changesets;
  2. After each merge, we'd need to rebase the others and potentially fix conflicts each time.

I think it's better to have all of these very similar changes in a single PR. To make it easier to review, I compartmentalized each component changeset in a single commit (check the commit list).

This is still a WIP, and I must do another pass for each commit. Due to the repetitive nature of the changes, it's possible I might have missed something and there may be some silly copy-pasta mistakes 🙈

I don't think addressing the changes in more digestible pieces will spam the repo. I definitely don't mean to arbitrarily create more work, but I agree with @mirka here that I'd prefer splitting the work into several PRs. It doesn't have to be 30 PRs - we can have one PR manage a few of them, but ideally, they shouldn't be in one big PR, because on top of the changes here, we have to review additional details, like where (and whether) each component is used, the API it exports, whether the documentation is enough, etc. For some of them, we might want to follow up, create additional stories and tests, etc. Some of those components may still have been experimental for a good reason, and we'll have to audit each one of them carefully to ensure that isn't the case. Doing that in a single PR sounds like too much.

@fullofcaffeine
Copy link
Member Author

fullofcaffeine commented Apr 19, 2024

There are a lot of files changed here, but they mostly follow the same pattern. I didn't want to create separate PRs for each component because:

  1. It'd spam the repo with 30 similar changesets;
  2. After each merge, we'd need to rebase the others and potentially fix conflicts each time.

I think it's better to have all of these very similar changes in a single PR. To make it easier to review, I compartmentalized each component changeset in a single commit (check the commit list).
This is still a WIP, and I must do another pass for each commit. Due to the repetitive nature of the changes, it's possible I might have missed something and there may be some silly copy-pasta mistakes 🙈

I don't think addressing the changes in more digestible pieces will spam the repo. I definitely don't mean to arbitrarily create more work, but I agree with @mirka here that I'd prefer splitting the work into several PRs. It doesn't have to be 30 PRs - we can have one PR manage a few of them, but ideally, they shouldn't be in one big PR, because on top of the changes here, we have to review additional details, like where (and whether) each component is used, the API it exports, whether the documentation is enough, etc. For some of them, we might want to follow up, create additional stories and tests, etc. Some of those components may still have been experimental for a good reason, and we'll have to audit each one of them carefully to ensure that isn't the case. Doing that in a single PR sounds like too much.

Fair enough! I'll start moving these changes into separate PRs then. We can keep this as an sort of birdseye view for now.

@fullofcaffeine
Copy link
Member Author

Closed in favor of splitting commits into individual PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants