useSelect: implement with useSyncExternalStore #46538
Conversation
|
Size Change: -476 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
| @@ -104,7 +105,100 @@ const renderQueue = createQueue(); | |||
| * ``` | |||
| * @return {UseSelectReturn<T>} A custom react hook. | |||
| */ | |||
| function Selecter( registry ) { | |||
| const NOTHING = {}; | |||
There was a problem hiding this comment.
Should this be declared outside of the function so always the same object would be reused?
There was a problem hiding this comment.
Yes, if it continues to be necessary (mere undefined doesn't work currently because we also support "really undefined" return value), I will extract it.
| let lastMapResult = NOTHING; | ||
| let lastMapResultValid = false; | ||
|
|
||
| const NULLSCRIBER = () => { |
There was a problem hiding this comment.
I like the word game here. If we're introducing this new terminology, let's add a comment to explain what it is.
There was a problem hiding this comment.
I managed to eliminate the NULLSCRIBER in the latest version 🙂 I'm still looking for a good name for the Selecter function though 🙂
There was a problem hiding this comment.
Nice!
Well, I was going to suggest "store" or "subscription manager", but I see you already went with that 😉
|
|
||
| const NULLSCRIBER = () => { | ||
| lastMapResultValid = false; | ||
| return () => {}; |
There was a problem hiding this comment.
Should this return NOTHING as well?
| return () => {}; | |
| return () => NOTHING; |
There was a problem hiding this comment.
No, this function is an "unsubscribe" function that does nothing.
There was a problem hiding this comment.
I see, we might want to use a comment to explain the subtle difference between "nothing" and NOTHING 😅
| return NULLSCRIBER; | ||
| } | ||
|
|
||
| return ( lis ) => { |
There was a problem hiding this comment.
Should lis be called something more comprehensible, like listener for example?
| if ( | ||
| lastMapResult === NOTHING || | ||
| ! isShallowEqual( lastMapResult, mapResult ) | ||
| ) { | ||
| lastMapResult = mapResult; | ||
| } | ||
| lastMapResultValid = true; |
There was a problem hiding this comment.
This looks a bit repetitive as seen in getValue(). Should we abstract it to a separate function?
There was a problem hiding this comment.
I managed to extract an updateValue function in the latest version of the hook. Used both by the getValue function and also by the store listeners.
| } | ||
|
|
||
| return ( lis ) => { | ||
| lastMapResultValid = false; |
There was a problem hiding this comment.
Is this necessary? Won't we mark it as invalid on each store subscription a few lines below?
There was a problem hiding this comment.
Yes, it is necessary. It ensures that right after the subscription is created, we freshly check the current store value, in case it changed in the meantime. There is always some delay between the first call to getValue and subscribing with subscribe. We need to invalidate the current value both when the subscription is created, and when a store update arrives.
| return { getValue, subscribe }; | ||
| } | ||
|
|
||
| const listeningStores = { current: null }; |
There was a problem hiding this comment.
Is there a reason why we're not defaulting to an empty array but to a null here?
There was a problem hiding this comment.
Nobody would use the empty array. The __unstableMarkListeningStores function always creates a new array and assigns it to the ref.
| const registry = useRegistry(); | ||
| const selecter = useMemo( () => Selecter( registry ), [ registry ] ); | ||
| const selector = useCallback( mapSelect, deps ); | ||
| const { getValue, subscribe } = selecter.select( selector, !! deps ); |
There was a problem hiding this comment.
Do we need to be a bit smarter about whether the deps changed? Won't this always be truthy when any dependencies are provided?
There was a problem hiding this comment.
The main place where deps are used is the useCallback hook that creates a memoized version of mapSelect. useCallback with no deps is an identity function, which returns the callback without memoizing. The !! deps boolean is used to distinguish between two useSelect modes:
- Without deps. That's what mainly the
withSelectHOC does. In that case, the hook only subscribes to the stores once, on mount, and doesn't resubscribe whenmapSelectchanges.mapSelectcan be a different function on every call, and the latest version will always be used. - With deps. In that case, the hook resubscribes to stores on every
depschange. And it can also avoid calls tomapSelectif the last value is valid and memoizedmapSelecthasn't changed. See the condition at the beginning ofupdateValue. Therefore,depslet us run through theuseSelecthook really fast if nothing relevant has changed.
I added a lot of comments to the new hook implementation to explain all the details.
| return useSyncExternalStore( subscribe, getValue, getValue ); | ||
| } | ||
|
|
||
| export function oldUseSelect( mapSelect, deps ) { |
There was a problem hiding this comment.
Let's change this to useOldSelect to keep this a hook as per the general React convention.
There was a problem hiding this comment.
I will remove it completely once the new useSelect starts working and I won't need to consult the old implementation any more.
| }; | ||
| } | ||
|
|
||
| function select( mapSelect, resub ) { |
There was a problem hiding this comment.
This will break for all instances that access useSelect() with this syntax, won't it:
import { store } from '@wordpress/core-data';
...
useSelect( store );
...
I believe we should support that syntax if we use the useSelect API. That means we'll need to add the workaround that exists in the old useSelect() implementation.
There was a problem hiding this comment.
Yes, this part is still missing from the new version. Also, it turns out we don't have any unit tests for it!
|
I pushed the latest version of the new |
|
I'll refrain from adding further noise for now, but when you feel this is in a more reviewable state, let me know and I'll take another fresh look! |
jsnajdr
force-pushed
the
update/use-select-sync-external-store
branch
from
f9fa6d1 to
cca4333
Compare
|
I think this is reviewable now. Everything is implemented, including async mode and suspense. The only reason why unit tests pass is the React fake timers bug. If I fix it locally in I'll have a closer look at |
| // use that list to create subscriptions to specific stores. | ||
| const listeningStores = { current: null }; | ||
| updateValue( () => | ||
| registry.__unstableMarkListeningStores( |
There was a problem hiding this comment.
Since this is done only on initialization, it means we assume the subscribed "stores" don't change as a response to a store data change? Maybe I'm missing something but I'm not sure this assumption is always correct.
There was a problem hiding this comment.
Yes, we assume that a given instance of mapSelect function always selects the same stores. Store data change can only change the results of the selects, but not the actual calls. In other words, a mapSelect function like this is unreliable:
( select ) => select( 'a' ).bOrC() ? select( 'b' ).get() : select( 'c' ).get()because it will subscribe only to either b or c but never to both. Depending on what got selected on the first call.
But this is already an existing behavior in the old useSelect, I'm merely reproducing it here.
There was a problem hiding this comment.
But this is already an existing behavior in the old useSelect, I'm merely reproducing it here.
Interesting, I wonder if we should fix that behavior (not necessarily in this PR)
There was a problem hiding this comment.
Store data change can only change the results of the selects, but not the actual calls. In other words, a
mapSelectfunction like this is unreliable
Could you elaborate a little bit more on this? I thought that this was always supported 😅 .
There was a problem hiding this comment.
We can try that, but I'd prefer another PR because at the moment I'm not sure if it's really feasible. When calling updateValue inside a store listener callback, we can't resubscribe at that moment -- it must happen in an effect, after useSyncExternalStore returns a new subscribe function. But we're not in a useSyncExternalStore call, we are in a store listener. The timings can get complicated.
Also there currently are no practical problems with the existing approach. Conditional selects either don't happen, or happen in a compatible way, like:
useSelect( ( select ) => {
const { bOrC } = select( 'a' );
const { get: getB } = select( 'b' );
const { get: getC } = select( 'c' );
return bOrC() ? getB() : getC();
}, [] );This pattern, select and destructure getters first and call them second, is very common.
There was a problem hiding this comment.
I thought that this was always supported
In the original useSelect, if you look at the useLayoutEffect that does the subscribing, it runs only when the dependencies change (depsChangedFlag). With useSelect without dependencies, the deps default to [], i.e., the hook subscribes only on mount, to the stores that were called during the first call to mapSelect. When useSelect is called with explicit dependencies, subscribed stores can change only when dependencies change. If mapSelect selects from different stores on every call, based on store state and not captured in dependencies, then re-subscription never happens.
This has been true for a very long time, maybe ever since granular subscriptions were introduced.
There was a problem hiding this comment.
Since it doesn't seem to change the behavior and I agree that it's probably very uncommon behavior, let's leave it as is for now.
There was a problem hiding this comment.
I see! FWIW, I think this might be an oversight by me back in #26724 though. We should resubscribe when listeningStores changes. I did an ad-hoc change locally and it passed all unit tests (well, except for the one that explicitly guards this). I agree that we should do it in another PR though.
There was a problem hiding this comment.
Thanks! Also, earlier today, I accidentally stumbled upon a selector that selects conditionally:
It might not subscribe to blockEditorStore if the earlier select from interfaceStore returns certain value. And then, when interfaceStore changes, subsequent changes in blockEditorStore won't be catched.
jsnajdr
force-pushed
the
update/use-select-sync-external-store
branch
from
cca4333 to
db17eb3
Compare
|
I'm trying to unblock the unit tests in #46714, changing the Jest default timers from |
jsnajdr
force-pushed
the
update/use-select-sync-external-store
branch
from
db17eb3 to
d09df1c
Compare
There was a problem hiding this comment.
Disclaimer: I don't have much experience with useSyncExternalStore besides playing with it when React 18 was released and peeking into sources of other libraries to check their usage.
The logic inside the Store function looks good to me. Thanks for the inline comments, by the way.
I also spent some time performing different actions in editors using this branch and couldn't spot any breakage.
| // lifetime, so the rules of hooks are not really violated. | ||
| return staticSelectMode | ||
| ? useStaticSelect( mapSelect ) | ||
| : useMappingSelect( false, mapSelect, deps ); |
There was a problem hiding this comment.
I assume we're using this order of arguments to keep deps optional. Is that correct?
There was a problem hiding this comment.
Yes, more or less 🙂 useSelect has some arguments, at this moment there are two, but one day there might be three or more. And useMappingSelect has a parameter convention to receive its own specific arguments first, and then attach ...useSelectArgs at the end.
|
All checks are green now ✅ Can be merged after approval. |
|
For the protocol, I'm on my annual support rotation this week, but this is my top-priority PR to test and review for early next week. It's fantastic to see this ready @jsnajdr 🥇 |
There was a problem hiding this comment.
Awesome work here @jsnajdr
I don't see any meaningful performance impact at first sight. 🚢
Implementing
useSelectwithuseSyncExternalStore. It mostly works, except async mode and the specialuseSelect( storeDescriptor )case.