Add useSelectors hook for getting a store's selectors

[Inwerpsel]

What?

Create a new function that can be called instead of useSelect to get a store's selectors.

export default function useSelectors( storeName ) {
	const registry = useRegistry();

	return registry.select( storeName );
}

Remove complexity from useSelect now that the function only does 1 thing.

Why?

In the previous code, 1 function is used to cover 2 use cases, switching between them with the first parameter's type. This never changes dynamically, code never depends on the function being able to do 2 things.

The original use case is to get data needed during render.

The 2nd use case simply returns a store's selectors, so they can be called in event handlers.

This use case does not need most of the hooks the original use is calling. However because of how hooks work, they needed to be called anyway in this path.

In the new function, the only remaining hook call for getting the controls is to useRegistry. All the other logic inside useSelect is completely irrelevant.

It also allows to simplify useSelect. It won't need any of the checks on the type of argument anymore, and can simply return the map output. For now I kept this out of this branch to make it easier to provide BC.

I didn't test the impact of not having to call these hooks, but I guess it's not 0. Especially if many components use this.

How?

• Add a new function that only gets the selectors.
• Make all usages of useSelect use this function instead.
• Remove complexity from useSelect that was only there to make the function do 2 things.

Testing Instructions

I tried to locate all relevant uses of the function, so depending on test coverage it should be relatively easy to confirm I didn't miss any.

Awaiting feedback on what to do with the public API, it could be that useSelect would preserve its duplicate function. If that's the case then this change should involve very little risk. For now I indeed kept useSelect unchanged so that this PR is unblocked. Perhaps in a follow up it's possible to add the simplified form already and keep a copy of the older one for BC.

TODO

The function is exposed with the old signature as a public API. While in practice it should work to check the argument and call the other function, it's being picked up by linting as a rules of hooks violation. Which it technically is, but in practice it only depends on the type which stays stable over time. Still seems like a non optimal solution and a bad example to set.

One option is to leave the old behavior in place in useSelect. That would also make it safer in case a usage was missed in this PR.

Alternatively keep an old copy of useSelect and export that as a public API. Then internal use could already use the simplified function.

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @Inwerpsel! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[ZebulanStanphill]

I absolutely agree that this is something that should be done, but because of backward compatibility requirements, we will almost certainly have to deprecate the 2nd use-case of useSelect (and promote the usage of useSelectors instead) for several releases before we can actually remove it.

[Inwerpsel]

@ZebulanStanphill Would it make sense to make it use the new version internally and keep a copy of the older one, just for exporting the API for now? I explored this here. Seems manageable as this function is supposed to not change. Only drawback is it would probably require changing every internal import of useSelect.

[jsnajdr]

I'm wondering whether we need to introduce the new useSelectors hook.

useRegistry().select( store )

is already very concise and does the same thing.

We can't remove the ! hasMappingFunction functionality right now because of backward compatibility. We'll need to mark it as deprecated and remove only much later.

One way to simplify the implementation is to make use of the fact that consumers can continue to use useSelect in either of the two modes, but they are extremely unlikely to switch between them at runtime. In other words, nobody ever does this:

const selectResult = useSelect( props.condition ? 'core' : ( select ) => { ... } );

A particular hook instance starts in certain mode and stays in that mode forever.

We can exploit that to write the hook this way:

function useSelect( storeOrMap ) {
  const mode = typeof storeOrMap === 'function' ? 'map' : 'select';
  const initialMode = useRef( mode );
  if ( mode !== initialMode ) {
    throw new Error( `useSelect hook tried to switch from ${ initialMode } to ${ mode } mode and changing mode is not possible.` );
  }
  if ( mode === 'select' ) {
    return useRegistry().select( storeOrMap );
  }

  /* Proceed to execute the complex mapping and subscribing hook */
}

This way we can avoid having to call all the dummy hooks.

[jsnajdr]

With the useSelectors behavior deprecated/removed, the mapSelect type is now merely Function.

[jsnajdr]

The storeName type is StoreDescriptor | string.

[Inwerpsel]

Thanks for the quick feedback @jsnajdr! I had just removed the changes from useSelect about the same time, but I might re-add them based on the suggestions.

is already very concise and does the same thing

I also considered this, though a custom hook is even slightly more concise. The change compared to useSelect is also less, you only need to use a different function. That would make it easier for people to switch to the new API.

A particular hook instance starts in certain mode and stays in that mode forever.
We can exploit that to write the hook this way:

I tried this and though it should work, it's picked up as a violation of the rules of hooks. But I agree this is the "cleanest" solution, allowing to simplify the logic already.

[jsnajdr]

it's picked up as a violation of the rules of hooks.

That's fine, the lint rule can't understand what we're up to. We're breaking the rules very intentionally here.

[Inwerpsel]

I'll try this later today/tomorrow 🤞

[Mamaduka]

I'm going to close the PR. Why?

• The useSelect hook was refactored in useSelect: implement with useSyncExternalStore #46538, which reduced some complexity when subscribing to the store and using a static getter.
• I don't think introducing yet another data hook will help DX. We can always make useStaticSelect public in the future.
• The change doesn't justify the amount of refactoring required for core code, IMO.

[Inwerpsel]

I primarily created this PR because, at the time, the way in which the 2 different uses were merged into a single function caused a significant performance impact and maintainability concern, because hooks used in 1 of 2 uses had to be called constantly on every render in both uses anyway.

I guess with the refactors that are mentioned, the scope of that problem has been reduced since less hooks are called.

Now I haven't really followed this part of WP the last couple years, so I may be missing something. But afaict it's still, at runtime, figuring out which of the 2 cases it's in, even though they're mutually exclusive and the code literally says they should only ever be using 1 of the 2 paths per invocation.

gutenberg/packages/data/src/components/use-select/index.js

Lines 314 to 315 in 303a1e1

	// On initial call, on mount, determine the mode of this `useSelect` call
	// and then never allow it to change on subsequent updates.

Consider that when you step through debug your own components that interact with this API, you will very often run through sections like this:

gutenberg/packages/data/src/components/use-select/index.js

Lines 319 to 325 in 303a1e1

	if ( staticSelectMode !== staticSelectModeRef.current ) {
	const prevMode = staticSelectModeRef.current ? 'static' : 'mapping';
	const nextMode = staticSelectMode ? 'static' : 'mapping';
	throw new Error(
	`Switching useSelect from ${ prevMode } to ${ nextMode } is not allowed`
	);
	}

While I understand the hesitation to split a single API into 2, especially if it's common, also consider the DX argument that this code will run every time render of every component that calls useSelect, and it doesn't exactly help step through debugging.

The fact that the code has to runtime check and throw an error in case someone switch to the other use of the function, seems quite fragile as well.

[Inwerpsel]

As for the amount of changes not being justified, it's nowhere near as much as it looks like just glancing the numbers.

The PR has 2 types of changes:

• Changes to the hooks: simplify useSelect, add useSelector (really simple function)
• Change some uses of useSelect to useSelector, with no changes to the surrounding logic

I remember that it didn't take very long to create these changes, and most of my time was spent figuring out what the existing thing was supposed to do 😄

[jsnajdr]

It was a design mistake to overload useSelect with two completely different functions, but now it's hard to go back.

useSelectors can be very simply written as:

const selectors = useRegistry().select( blocksStore );

so if you want to avoid the "static" version of useSelect, you can do this. It's just 8 characters more than when you have useSelectors.

[Inwerpsel]

Why is it hard to go back? Does this code really, for eternity, will need to ask itself which mode it is in and whether it should throw an error, every time your component renders an update to the screen, even though it's not supposed to change?

To be clear, I'm not at all requesting this be solved now, or the PR reopened. In fact, I kept the PR open just to demonstrate what the problem is and that it can be fixed. So I'm just mentioning here for future reference that this quirk still exists and might be addressable when changing the architecture of state management at some point in the future.