CustomSelectControl: Privatise __experimentalShowSelectedHint using @wordpress/experiments #47229
Conversation
noisysocks
added
[Type] Code Quality
| // 'without a warning. If you ignore this error and depend on unstable features, ' + | ||
| // 'your product will inevitably break on one of the next WordPress releases.' | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
This check doesn't play well with Story Book. There, __dangerousOptInToUnstableAPIsOnlyForCoreModules is called multiple times assumedly due to hot module loading. This causes an error when navigating to a different component.
I'm not sure what to do to get around this. Thoughts @adamziel?
There was a problem hiding this comment.
Could this be unrelated to HMR? I wonder if the storybook is loading the same file twice.
If this is indeed a hot reloading issue:
Ideally __dangerousOptInToUnstableAPIsOnlyForCoreModules wouldn't be called for the second time on hot reload. The module has already opted-in and the global state knows it, there's no need to go through the motions all over again. The easiest solution would be:
try {
__dangerousOptInToUnstableAPIsOnlyForCoreModules()
} catch(e) {
// .. ignore
}However, this would obscure genuine errors.
I have two ideas:
- Do not re-evaluate experiments.js during HMR
- Detect we're inside HMR and bale out
There was a problem hiding this comment.
That check matters mostly in the WordPress build. We can build the Gutenberg plugin without that error:
if ( ! IS_GUTENBERG_PLUGIN ) {
// This check doesn't play well with Hot Module Reloading and isn't included in
// the Gutenberg plugin. It only matters in the WordPress core release.
if ( registeredExperiments.includes( moduleName ) ) {
throw new Error(
`You tried to opt-in to unstable APIs as module "${ moduleName }" which is already registered. ` +
'This feature is only for JavaScript modules shipped with WordPress core. ' +
'Please do not use it in plugins and themes as the unstable APIs will be removed ' +
'without a warning. If you ignore this error and depend on unstable features, ' +
'your product will inevitably break on one of the next WordPress releases.'
);
}
}There was a problem hiding this comment.
Will defer to you on this 😅 I'm not really sure what's going on. I'll update the PR with your ! IS_GUTENBERG_PLUGIN suggestion.
There was a problem hiding this comment.
Oh, that doesn't fix this because IS_GUTENBERG_PLUGIN is not set when running the story book (npm run storybook:dev).
@ciampo: Is it safe to set process.env.IS_GUTENBERG_PLUGIN = true in storybook/main.js?
There was a problem hiding this comment.
It feels overkill to me that there should be a global env variable for this one specific thing. Can we kill the reregistration check altogether? In my mind the only check that matters is the consent string.
There was a problem hiding this comment.
It feels overkill to me that there should be a global env variable for this one specific thing.
To me it's the opposite – removing that check just for this one specific thing feels like overkill.
The consent string is a carrot and this checks is a stick, I think it's important to have both. A global variable is just a feature flag – they are commonly used in similar scenarios. Why would it be overkill? I could prepare the necessary commit if you wish.
There was a problem hiding this comment.
@adamziel I'm coming late to this discussion so I don't have all the context -- but it seems to me that allowing double registration of the same module is just fine. We check if the moduleName is one of the core modules, and if it is, return the { lock, unlock } pair. We can do the same on the second or the 100th call, there's no reason to allow just a single call.
The registeredExperiments array could be removed completely. It's used only to check for these repeated registrations and nothing else. And we could get rid of resetRegisteredExperiments, too, making the experiments module stateless.
There was a problem hiding this comment.
The consent string is a carrot and this checks is a stick, I think it's important to have both. A global variable is just a feature flag – they are commonly used in similar scenarios. Why would it be overkill? I could prepare the necessary commit if you wish.
Hmmm ok I suppose. I just dislike global config 😅
The
registeredExperimentsarray could be removed completely. It's used only to check for these repeated registrations and nothing else. And we could get rid ofresetRegisteredExperiments, too, making the experiments module stateless.
Making experiments stateless is appealing to me. I imagine we'll face this exact problem again in a different context that requires loading modules multiple times e.g. adding HMR, adding lazy loading, turning WP Admin into more of a SPA.
Up to you @adamziel, it's your baby 😀
There was a problem hiding this comment.
@adamziel I'm coming late to this discussion so I don't have all the context -- but it seems to me that allowing double registration of the same module is just fine.
@jsnajdr The double registration protection is to prevent developers from opting-in to experiments by pretending to be one of the core packages. Everything would still work without it, but the private APIs would be less protected.
I imagine we'll face this exact problem again (...) adding HMR, adding lazy loading, turning WP Admin into more of a SPA.
@noisysocks IMHO Including strict checks in the initial launch will discourage tampering more effectively than not including them. Of course – the time may come when they have to be removed. That's fine. It's not like we'll advertise the adjustment anywhere. I'll update this PR to include the additional feature flag.
Edit: See b2585fc
|
An alternative to this would be to stabilise |
|
Hey all! Trying to catch up on a long queue, I initially missed the pings in this PR.
We could, although my preference would be not to commit to those APIs just yet. I'd really love to finish the Plus, most of the challenges that highlighted in this PR would apply to any other component — and therefore, we would need to overcome them anyways. Related to the other conversations in this PR, I'll answer inline :) |
|
Think I've addressed all feedback. Can I get a final review? |
|
Thank you all for collaborating on this! While reading through this PR I started reflecting about the The APIs definitely do a good job at drawing a clear boundary and making it explicit that locked (experimental) APIs are not guaranteed to be backwards-compatible in future releases. At the same time, as a core contributor, I see some potential drawbacks:
I realise that this extra “friction” may be in place exactly to act as a deterrent against adding and consuming unstable/experimental APIs. Zooming out a little bit and looking at the broader approach to compatibility, it’s important that we try to align the interests of both the core Gutenberg contributors and the broader WP community. In order to nurture a healthier developer ecosystem that is sustainable and modern, we need to allow the core Gutenberg contributors to be as effective as possible, trying to avoid technical debt having repercussions on the wider community. |
Yes we'll be able to make changes to
@adamziel can probably speak better to these points but I feel that they may be solvable with enhancements to I might spin up a GitHub Discussion for the higher level discussion about |
|
Size Change: +428 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
I might spin up a GitHub Discussion for the higher level discussion about lock() / unlock(), experimental APIs, DevX, and backwards compatibility. In this PR I'm aiming to make one experimental API not become a part of the WP 6.2 public API
That makes sense, thank you for your patience here.
Overall code changes look good, and the PR is testing well in Storybook and in the editor.
I wonder if we should use the ALLOW_EXPERIMENTS_REREGISTRATION env variable as suggested by Adam before merging, though.
Also, could we get a CHANGELOG entry for the components package too?
Thank you!
@ciampo The only consumers are packages in the Gutenberg repository. The experimental APIs are private and should not be used anywhere outside.
The friction is in separating experimental components from stable components for WordPress core.
We can retain type defs – it's just not implemented at the moment. I fleshed out the basic idea in one of the comments in this PR.
Ideally consuming experimental APIs would be easy in core and impossible outside of core. If you have any suggestions how to reduce any internal friction without reducing the privacy, I'd love to discuss! |
Thank you for the clarification — what I meant is that even if the consumers of the APIs are other Gutenberg packages, this extra layer of complexity could make it harder for core contributors (especially new/less experienced members of the community) to learn the codebase. But, as I mentioned before, this could also be "on purpose" — ie. discouraging folks from adding experimental APIs in the first place.
I saw that, and it definitely looks promising 😄
Absolutely — and thank you again for the work you've been doing on this. I'll definitely make sure to share feedback / suggestions if I come up with anything. |
|
Thanks @adamziel, appreciate you. I fixed the merge conflicts here. Should be ready again for final review. |
|
Flaky tests detected in e2b8e35. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4099603874
|
adamziel
force-pushed
the
update/privatise-__experimentalShowSelectedHint
branch
from
15c5915 to
d1a795f
Compare
|
Just squashed, let's hope the checks work this time. |
adamziel
force-pushed
the
update/privatise-__experimentalShowSelectedHint
branch
from
d1a795f to
1709b42
Compare
|
@ciampo The mobile tests are still failing, but it's no longer about the experimental check. The failures don't seem to be related at all so I rebased this one and pushed again. |
|
@ciampo Mobile unit tests passed! |
|
Thanks once again @adamziel! Pushed a fix for @ciampo's latest comment as well as a few minor things I noticed while reviewing the diff. It looks good to me. @adamziel: Hopefully tests pass and we can merge but, if not, could you take over this PR? I'll be out on parental leave for 8 weeks as of tomorrow 👶 |
|
@noisysocks Native e2e tests failed, I just restarted them. Fingers crossed! Also – that's awesome! 👶 Congratulations man! |
There was a problem hiding this comment.
Awesome job, @noisysocks and @adamziel 🚀
✅ Component behaves as expected in Storybook, no errors are printed to console
✅ Hints keep showing as expected in the Site editor, as per testing instructions
✅ The experimental API is locked
Native tests failures don't seem related. I have re-started them again, let's 🤞
Following up, it would be great to see how we can improve the retention of type defs (draft idea in #47229 (comment) ).
|
@ciampo the bad news is that the same native test is failing for 5 restarts now. The good news is it also failed for the last 5 commits to trunk. Let's merge this PR! |
It's an error when installing Playwright, trying to download stuff: @WunderBart Is this really an intermittent failure that eventually fixes itself, or maybe it's a sign that we have an outdated Ubuntu or something like that? |
Heads up that this indeed broke the mobile editor, it looks like we have some flakiness there on the mobile E2E tests, we will check those but we are always happy to double-check any CI checks failing in any PRs before merging to avoid future failures in |
ntsekouras
added
the
Backport to WP 6.7 Beta/RC
|
Oops, sorry about that @geriux – I'll let you know next time this happens! |
…wordpress/experiments (#47229) ## What? Part of #47196. Uses `@wordpress/experiments` (#46131) to make `__experimentalShowSelectedHint` in `CustomSelectControl` private. ## Why? We don't want to add any new experimental APIs to 6.2 as part of an effort to no longer expose experimental APIs in Core. ## How? https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/coding-guidelines.md#experimental-react-component-properties ## Testing Instructions 1. Use a block theme with more than 5 font sizes or manually edit `theme.json` to contain more than 5 font sizes in `settings.typography.fontSizes`. 2. Open the site editor. Appearance → Editor → Edit. 3. Go to Styles → Typography → Headings. 4. Select a heading level. 5. Toggle off the custom font size picker. 6. You should see a hint alongside the selected font size preset. Co-authored-by: Adam Zieliński <adam@adamziel.com>
|
Cherry-picked this PR to the wp/6.2 branch. |
ntsekouras
removed
the
Backport to WP 6.7 Beta/RC
What?
Part of #47196. Uses
@wordpress/experiments(#46131) to make__experimentalShowSelectedHintinCustomSelectControlprivate.Why?
We don't want to add any new experimental APIs to 6.2 as part of an effort to no longer expose experimental APIs in Core.
How?
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/coding-guidelines.md#experimental-react-component-properties
Testing Instructions
theme.jsonto contain more than 5 font sizes insettings.typography.fontSizes.