Use experiment locking/unlocking system for block interface selector and actions #47375
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Package] Block editor
|
Size Change: -190 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
talldan
commented
Jan 24, 2023
packages/block-editor/src/components/block-tools/selected-block-popover.js
Show resolved
Hide resolved
14 tasks
This was referenced Jan 25, 2023
ntsekouras
force-pushed
the
experimentalize/hide-and-show-block-interface-api
branch
from
7ccb8f1
to
04ace1e
Compare
ntsekouras
reviewed
Jan 26, 2023
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function __experimentalHideBlockInterface() { |
There was a problem hiding this comment.
I think we settled that we don't need to have the __experimental prefix anymore, since we lock them. That goes to the other places as well, like selectors, etc..
There was a problem hiding this comment.
How does that affect things like the documentation generation, which is currently configured to skip APIs with the __experimental prefix?
There was a problem hiding this comment.
I removed the prefix in f8e6574, but if we decide we want to change it back that commit can be reverted 👍
adamziel
added a commit
that referenced
this pull request
Jan 26, 2023
…try.registerStore() and for sub registries. (#47421) `registerPrivateActions` and `registerPrivateSelectors` only supported stores created using `createReduxStore`. On stores registered using the deprecated `registerStore` helper the private actions are not returned upon `unlock()`-ing the `useDispatch()` call (as reported by @talldan in #47375 (comment)). This PR adds the missing support for the `registerStore()` stores as well as for the children stores in sub registries.
talldan
force-pushed
the
experimentalize/hide-and-show-block-interface-api
branch
from
04ace1e
to
d270040
Compare
talldan
commented
Jan 27, 2023
| * @param {boolean} stripExperimentalSettings Whether to strip experimental settings. | ||
| * @return {Object} Action object | ||
| */ | ||
| export function __experimentalUpdateSettings( |
There was a problem hiding this comment.
This one was introduced in #47319 as another locked action.
I've moved it to the private-actions file, which is a bit easier to handle compared to specifying every private action/selector in the index file.
(I imagine we want to remove the __experimental prefix from this one too, I don't mind doing that in a separate PR)
There was a problem hiding this comment.
I used the __experimental prefix to differentiate it from the unprefixed updateSettings action. In theory the unlocked store could shadow the stable action with a private one with the same name, but I worry about hard to identify bugs where the private action isn't registered for some reason and you call the public one without realizing and without a clear error message. It would be good to have a separate discussion focused just on prefixing as the topic comes up in every other PR.
There was a problem hiding this comment.
I agree that we can skip the __experimentalUpdateSettings for now and discuss/explore it separately. For newly created actions, selectors, etc.. I think it's good to skip the prefix.
|
All tests passing now, and in manual testing it works as expected 👍 |
adamziel
reviewed
Jan 27, 2023
adamziel
reviewed
Jan 27, 2023
adamziel
reviewed
Jan 27, 2023
adamziel
reviewed
Jan 27, 2023
adamziel
reviewed
Jan 27, 2023
| isBlockInterfaceHidden: true, | ||
| }; | ||
|
|
||
| expect( isBlockInterfaceHidden( state ) ).toBe( true ); |
There was a problem hiding this comment.
I like to test actions and selectors by creating an actual registry and confirming the entire system is well-behaved, e.g.
const registry = createRegistry();
registry.register( blockEditorStore );
await unlock(registry.dispatch( blockEditorStore ))
. hideBlockInterface();
// Check that blocks store contains the new block.
const isHidden =
unlock(registry.select( blockEditorStore ))
. isBlockInterfaceHidden();
expect( isHidden ).toBeTruthy();This reflects how the code is actually used and tells us when the action/selector feedback loop breaks – I find it really useful.
On the other hand, testing a selector with a mock state will only throw an error if the state property name is updated – even if the action, reducer, and the selector itself have been updated properly.
There was a problem hiding this comment.
I like that approach too. It's a bit beyond the scope of this PR though, so I'll look into changing this separately.
Co-authored-by: Adam Zielinski <adam@adamziel.com>
talldan
force-pushed
the
experimentalize/hide-and-show-block-interface-api
branch
from
38dd317
to
a69e0d4
Compare
talldan
commented
Jan 30, 2023
Comment on lines
+37
to
+47
| unlock( store ).registerPrivateActions( privateActions ); | ||
| unlock( store ).registerPrivateSelectors( privateSelectors ); | ||
|
|
||
| // We will be able to use the `register` function once we switch | ||
| // the "preferences" persistence to use the new preferences package. | ||
| const registeredStore = registerStore( STORE_NAME, { | ||
| ...storeConfig, | ||
| persist: [ 'preferences' ], | ||
| } ); | ||
|
|
||
| unlock( registeredStore ).registerPrivateActions( { | ||
| __experimentalUpdateSettings, | ||
| } ); | ||
| unlock( registeredStore ).registerPrivateActions( privateActions ); | ||
| unlock( registeredStore ).registerPrivateSelectors( privateSelectors ); |
There was a problem hiding this comment.
Guessing I need to register private action/selectors on both the store and registeredStore?
There was a problem hiding this comment.
Locking only the registeredStore should be enough.. It didn't work for you if you did that? 🤔
|
Flaky tests detected in f9f5188. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4042215115
|
|
🎉 |
|
One potential drawback to calling |
|
@gziolo good note. There are two solutions I can think of:
I don't like either of these solutions. This issue could use a separate discussion thread. |
It might help if the prefix was automatically prepended by the |
|
That's a good observation @gziolo.
I don't think it'd require two const { public, private } = useSelect( select ) => {
const { publicSelector } = select( myStore );
const { privateSelector } = unlock( select( myStore ) );
return { public: publicSelector, private: privateSelector };
} ); |
|
I filed #47830, where I summarized our discussion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
See #47196.
This is an attempt to use the locking/unlocking system for the block interface actions and selector from #45131.
Unfortunately it doesn't seem to work, so I'm not sure if I did something wrong.
Why?
These APIs aren't ready to be made stable yet. They're currently plugin only, and should stay that way.
How?
I followed the guide here:
https://github.com/WordPress/gutenberg/blob/trunk/docs/contributors/code/coding-guidelines.md#experimental-selectors-and-actions
Testing Instructions
It shouldn't work any differently to trunk from an API point of view. To test, do the following: