Should this be declared outside of the function so always the same object would be reused?

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.

Sounds good to me 👍

I like the word game here. If we're introducing this new terminology, let's add a comment to explain what it is.

I managed to eliminate the NULLSCRIBER in the latest version 🙂 I'm still looking for a good name for the Selecter function though 🙂

Nice!

Well, I was going to suggest "store" or "subscription manager", but I see you already went with that 😉

Should this return NOTHING as well?

No, this function is an "unsubscribe" function that does nothing.

I see, we might want to use a comment to explain the subtle difference between "nothing" and NOTHING 😅

Should lis be called something more comprehensible, like listener for example?

This looks a bit repetitive as seen in getValue(). Should we abstract it to a separate function?

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.

Is this necessary? Won't we mark it as invalid on each store subscription a few lines below?

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.

Is there a reason why we're not defaulting to an empty array but to a null here?

Nobody would use the empty array. The __unstableMarkListeningStores function always creates a new array and assigns it to the ref.

Do we need to be a bit smarter about whether the deps changed? Won't this always be truthy when any dependencies are provided?

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:

1. Without deps. That's what mainly the withSelect HOC does. In that case, the hook only subscribes to the stores once, on mount, and doesn't resubscribe when mapSelect changes. mapSelect can be a different function on every call, and the latest version will always be used.
2. With deps. In that case, the hook resubscribes to stores on every deps change. And it can also avoid calls to mapSelect if the last value is valid and memoized mapSelect hasn't changed. See the condition at the beginning of updateValue. Therefore, deps let us run through the useSelect hook really fast if nothing relevant has changed.

I added a lot of comments to the new hook implementation to explain all the details.

Let's change this to useOldSelect to keep this a hook as per the general React convention.

I will remove it completely once the new useSelect starts working and I won't need to consult the old implementation any more.

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.

Yes, this part is still missing from the new version. Also, it turns out we don't have any unit tests for it!

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.

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.

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)

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

Could you elaborate a little bit more on this? I thought that this was always supported 😅 .

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.

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.

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.

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.

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.

I assume we're using this order of arguments to keep deps optional. Is that correct?

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.