Fix private registry selectors #50985
Closed
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
noisysocks
added
[Type] Bug
noisysocks
commented
May 26, 2023
| @@ -164,6 +165,43 @@ describe( 'Private data APIs', () => { | |||
| ); | |||
| expect( subPrivateSelectors.getSecretDiscount() ).toEqual( 800 ); | |||
| } ); | |||
|
|
|||
| it( 'should support registry selectors', () => { | |||
There was a problem hiding this comment.
This is a passing test for a fix I included in #50643.
noisysocks
commented
May 26, 2023
| expect( privateSelectors.getPrice() ).toEqual( 1000 ); | ||
| } ); | ||
|
|
||
| it( 'should support calling a private registry selector from a public selector', () => { |
There was a problem hiding this comment.
This is the failing test that I can't figure out how to fix.
14 tasks
|
Size Change: -17.9 kB (-1%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 87a9833. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5086304201
|
|
No thoughts about eager assignment yet, but what if |
It's lazy because private selectors (and also actions) are not registered during store creation, but later, calling the export const store = createReduxStore( STORE_NAME, storeConfig );
register( store );
unlock( store ).registerPrivateSelectors( privateSelectors );
unlock( store ).registerPrivateActions( privateActions );In theory I can register new private selectors at any time. That's why each |
|
@jsnajdr perhaps it would make more sense to only allow the registration at the time the store is created - this would be more consistent and remove this problem entirely. I can’t quite recall if there is any reason not to do so. |
jsnajdr
reviewed
May 26, 2023
| const storeB = createReduxStore( 'b', { | ||
| reducer: ( state = {} ) => state, | ||
| selectors: { | ||
| getPrice: ( state ) => getPrice( state ), |
There was a problem hiding this comment.
This test it bad, you can't really expect that it will work.
- a registry selector must be registered with a store to "activate" it, i.e., to set the
selector.registryreference. Because instoreAit's only registered lazily, and never called, it's not activated yet. - in
storeByou should register it directly, withselectors: { getPrice }. Then the test would work. - because the implemenation sets a
.registryproperty on thegetPriceobject returned fromcreateRegistrySelector, you should really usegetPricejust once. And not register it in two stores. Otherwise both usages will be fighting for theselector.registryfield and the results can in theory be unpredictable.
Where is the real-world situation where you're running into this kind of issue?
There was a problem hiding this comment.
- a registry selector must be registered with a store to "activate" it
The private getPrice selector is associated with storeA.
Because in
storeAit's only registered lazily, and never called, it's not activated yet.
That's the bug, isn't it? It shouldn't be lazy.
in storeB you should register it directly, with selectors: { getPrice }. Then the test would work.
But then it won't be private?
I shouldn't have named both selectors getPrice in the test. The intention is that they are two different selectors. Also it's not necessary (and confusing) that there are two stores. I'll update the test to fix this.
Where is the real-world situation where you're running into this kind of issue?
In #50857, select( blockEditorStore ).canInsertBlockTypeUnmemoized is using select( blockEditorStore ).getBlockEditingMode which is a^ private registry selector.
^ Well, was a private registry selector. I've added a workaround for now.
There was a problem hiding this comment.
Thanks for clarifying the test. In the previous version, behavior of store B depended on what was registered in store A, and that was fishy 🙂
I cherry-picked the tests into #51051 and fixed it there by pre-binding private selectors on store instantiation.
I had to modify the test so that the private selector registration is done before the store is instantiated by registry.register. Otherwise we don't have a good opportunity to pre-bind selectors.
The registerPrivateSelectors API is weird in the sense that it allows registering selectors i) multiple times, and ii) after the store has been instantianted one or more times, and iii) the late registration affects also all the instances that already exist, it is retroactive!
That should probably also be eventually fixed. We'll get to that sooner or later.
@adamziel suggests this:
createReduxStore( 'a', {
selectors: lock( { foo }, { privateFoo } )
} );It's syntactically elegant, but semantically it's nonsense 🙂 To register a private selector, we need to unlock a private API of @wordpress/data, but here we're achieving that by locking.
What we'd like to do is:
- Register private selectors inside the
createReduxStorecall, so that the entire registration is atomic, andcreateReduxStorereturns a store descriptor that is immutable. - Expose private selectors registration only as a private API, and that means having to call
unlocksomewhere.
One way to satisfy that could be:
unlock( createReduxStore )( 'a', {
selectors: { foo },
privateSelectors: { privateFoo },
} );Are there others?
There was a problem hiding this comment.
It's syntactically elegant, but semantically it's nonsense 🙂 To register a private selector, we need to unlock a private API of @wordpress/data, but here we're achieving that by locking.
This is just a way of passing the information into createReduxStore. It would have to unlock the selectors object and then process the private selectors.
Exposing a privateSelectors options on an unlocked version of createReduxStore indeed sounds much better. This is exactly the way in which using private arguments with lock()/unlock() is documented 👍 I can't think there are many other ways, too, or at least I couldn't come up with any when I was updating the contributing guidelines.
There was a problem hiding this comment.
A private privateSelectors option makes complete sense to me.
Then we would need to add new options to createReduxStore( 'a', {
selectors: { foo },
privateSelectors: { privateFoo },
} );Or to allow calling |
Or do this: createReduxStore( 'a', {
selectors: lock( { foo }, { privateFoo } )
} ); |
|
Closing this in favour of #51051. |
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, why, how
Currently one gets an error if you call a private registry selector (created using
createRegistrySelector) from a regular public selector.This is happens because the
mapValuesthat setsselector.registryhappens lazily for private selectors.gutenberg/packages/data/src/redux-store/index.js
Lines 218 to 237 in 382eda6
It needs to be eager (the same as public selectors) so that selectors can be composed.
I can't figure out a good way to fix this, so this PR just adds a failing test. @jsnajdr @adamziel: could you please help me? 😀
Testing Instructions