Components: Remove "experimental" designation#60837
Conversation
|
Size Change: +4.7 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
fullofcaffeine
changed the title
AlignmentMatrixControl
fullofcaffeine
force-pushed
the
remove/wp-components-experimental-designation
branch
2 times, most recently
from
850589e to
08e43d1
Compare
…xport the legacy adapter as the classic one (WIP)
fullofcaffeine
force-pushed
the
remove/wp-components-experimental-designation
branch
from
08e43d1 to
752a07f
Compare
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Thanks for working on it @fullofcaffeine 👍
I know you're still working on it, but sending some early high-level feedback, in case it's helpful.
| // Components. | ||
| export { default as __experimentalAlignmentMatrixControl } from './alignment-matrix-control'; | ||
| export { | ||
| default as __experimentalAlignmentMatrixControl, |
There was a problem hiding this comment.
While I appreciate that we're keeping the __experimental exports for backward compatibility reasons, I wonder if we should deprecate __experimental imports now that the components are stable. That will nudge 3rd party developers to migrate to stable imports, otherwise they might be stuck with the __experimental ones forever
There was a problem hiding this comment.
There are so many "shades" of deprecation — hard, soft, TS-only, TSDoc only... 😂 For these I think a TSDoc @deprecated Import would be appropriate. Is that what you had in mind?AlignmentMatrixControl instead
There was a problem hiding this comment.
Yup, it's odd that deprecation can be so complex 😅
Anyway, imagine I'm a plugin dev that uses some experimental components in my plugin. How will a TSDoc @deprecated help me find out that I'm using a deprecated import and I have to replace it with the canonical stable import? Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?
There was a problem hiding this comment.
Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?
I managed to make the @deprecate annotation work when it's added just before the actual export symbol, inside the export block:
export {
/**
* @deprecated Since version X.X. Will be removed in version X.Y. Use `ConfirmDialog` instead.
*/
ConfirmDialog as __experimentalConfirmDialog,
ConfirmDialog,
} from './confirm-dialog';I can confirm it doesn't affect the second export there:
And of the @deprecated declaration, it will render like this in vscode (notice the strikethrough in the import and the warning in the IntelliSense popup):
 |
 |
So I assume all editors/IDEs that support tsdoc will warn users about it.
If that's a good way to prevent developers from continuing to use it, that is another story. It might be good to do the deprecation gradually? First soft-deprecate using a tsdoc, announce in the changelog and perhaps a Make post, then in another (major?) version, completely remove the __experimental exports.
There was a problem hiding this comment.
Nice 👍
And yeah, gradual deprecation sounds good to me. We should likely document the plan for that in the related issue, including some releases where we aim to further/hard deprecate.
There was a problem hiding this comment.
Anyway, imagine I'm a plugin dev that uses some experimental components in my plugin. How will a TSDoc
@deprecatedhelp me find out that I'm using a deprecated import and I have to replace it with the canonical stable import? Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?
My current stance on this is,
- Do we plan to hard deprecate it?
- Does switching to the new thing provide the consumer with actual benefits over the old thing?
If the answer is "no" to both questions, we shouldn't log a console warning and we shouldn't block the TS from compiling. We shouldn't demand a dev's attention for what is essentially a housekeeping detail. If they care about housekeeping details, they'll do it.
And to be clear, personally I don't think the __experimental exports are worth hard deprecating. It's virtually free to maintain this BC, compared to the cost on the ecosystem if we yank them.
There was a problem hiding this comment.
Oh, the complex deprecation discussion again 😅
I think we have far too many cases that are cheap/free to maintain, but because we have too many, it adds so much noise. I wish we had a clear deprecation policy that actually allows us to get rid of deprecated features at some point.
If I come back to planet Earth though and draw a line on the tradeoffs, you're right and it should be just fine to continue soft-deprecating as suggested.
| Button, | ||
| __experimentalConfirmDialog as ConfirmDialog, | ||
| } from '@wordpress/components'; | ||
| import { Button, ConfirmDialog } from '@wordpress/components'; |
There was a problem hiding this comment.
I think updating all of Gutenberg to use the stable component imports can be its own PR. There are quite a few of them that this PR isn't covering just yet, and splitting to smaller PRs could help with reviewing.
There was a problem hiding this comment.
Doing it in a separate PR would also help to check that we didn't remove any __experimental exports by mistake.
There was a problem hiding this comment.
Sounds good! I'll rework that 👍🏻
mirka
added
the
Needs Dev Note
bph
added
[Feature] Component System
mirka
added
[Package] Components
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
I reviewed the commits for the first two components, AlignmentMatrixControl and BorderBoxControl.
The checklist of what needs to be done for each component looks good, pending the decision about if/how to flag as deprecated. One more thing we need to do is to remove these callouts in the readmes:
On another note, I'm sure this de-experimentalizing process will reveal some smells, where we'll need to make case-by-case decisions. One example I already saw is the assortment of utility functions being exported in relation to BorderBoxControl. Feel free to flag if you notice anything fishy like that, for example components that aren't being used anywhere in Gutenberg.
So given that this process does require relatively careful review, I'm thinking we can chunk PRs into several components each, so we can better detect abnormalities. Does that sound about right?
| hasSplitBorders, | ||
| isDefinedBorder, | ||
| isEmptyBorder, |
There was a problem hiding this comment.
Ok, these three exports are sounding major alarm bells for me 😱 Not sure what's going on there. Let's keep these experimental for now so we can investigate and figure out a better way.
There was a problem hiding this comment.
Let's not touch the *.native.js stuff for now, they are completely separate.
|
Closed in favor of #60884, where I focused on removing the I'll follow up later with a PR to replace the imports and another to work on replacing the exported implementation of |
5 participants
Solves: #59418.
What?
Remove
__experimentalprefix from all "experimental" components, effectively promoting them to regular stable components. See the related issue for more context.Note: Some more complex conversions could be done in a separate PR, like the one for
CustomSelectControl,which also requires migrating to a new adapter component for current consumers.Why?
The strategy of prefixing exports with
__experimentalhas become deprecated after the introduction of private APIs. Read more in the related issue.How?
For each
__experimentalcomponent:__experimentalprefix;__experimentalexport for backwards compatibility;__experimentalin GB and components to the one without the prefix (including in storybook stories). Also update the docs to refer to the new unprefixed component;PREVIOUSLY_EXPERIMENTAL_COMPONENTSconst array inmanager-head.htmlso that oldexperimentalstory paths are redirected to the new one.TODO:
__experimentalprefix. PerhapsComponents: Remove "experimental" designation for <component>for each component would be the best changelog entry, semantically?Testing Instructions