-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Components: Remove "experimental" designation #60884
Conversation
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:
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, |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍🏻
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to de__experimetal
ize 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Closed in favor of splitting commits into individual PRs. |
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:__experimental
prefix;__experimental
alias export for backwards compatibility;PREVIOUSLY_EXPERIMENTAL_COMPONENTS
const array inmanager-head.html
so that oldexperimental
story paths are redirected to the new one.TODO:
__experimental
prefix. PerhapsComponents: Remove "experimental" designation for <component>
for each component would be the best changelog entry, semantically?Testing Instructions