useSelect: normalise getting selectors for callbacks #31078
Conversation
ellatrix
added
[Type] Enhancement
| getBlockRootClientId: selectors.getBlockRootClientId, | ||
| getBlockIndex: selectors.getBlockIndex, | ||
| getBlockCount: selectors.getBlockCount, | ||
| getDraggedBlockClientIds: selectors.getDraggedBlockClientIds, |
There was a problem hiding this comment.
Especially here, the benefits are clear.
| [] | ||
| ); | ||
| const { getBlockType } = useSelect( | ||
| ( select ) => select( blocksStore ), |
There was a problem hiding this comment.
And here too. We're returning the entire selector list.
|
Size Change: -857 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
|
There are two sides to this proposal:
|
|
The other thing as well is that it encourages multiple useSelect calls on the same component. This might have some very small impact on perf. |
| } = select( blockEditorStore ); | ||
|
|
||
| return { | ||
| canInsertBlockType: _canInsertBlockType, |
There was a problem hiding this comment.
To have the full picture, this could also be refactored as:
return {
...pick( select( blockEditorStore ), [ 'canInsertBlockType, getBlockIndex, getClientIdsOfDescendants ),
hasUploadPermissions: getSettings().mediaUpload,
};There was a problem hiding this comment.
It's doesn't solve my main problem: it's awkward to return selector functions in a mapping.
There was a problem hiding this comment.
Right, it's a workaround. I think for withDispatch we were providing an 3rd argument so it could use selectors without passing it down from withSelect. Here it won't work so your exploration is really worth conisdering.
Right, for now it's not documented and all the examples show you to map. We can add examples for this as well, but maybe use it internally only for now. Alternatively we could create a separate experimental hook called
They are not the same performance-wise. If you don't pass a mapping function, all the mapping logic is skipped so it will perform much better. |
| @@ -140,6 +152,10 @@ export default function useSelect( _mapSelect, deps ) { | |||
| } ); | |||
|
|
|||
| useIsomorphicLayoutEffect( () => { | |||
| if ( isWithoutMapping ) { | |||
There was a problem hiding this comment.
@youknowriad useSelect( storeKey) and useSelect( map ) is not the same in terms of performance because all the mapping logic is skipped. It's not the same as calling a useSelect( map ) twice.
|
It seems like the question is: do we integrate this into |
|
I think just integrate it with useSelect but I believe the documentation is the problem here. How can we make sure folks use it properly, is there a way to lint this somehow? |
|
I don't think it would be easy to write such a lint rule, but I can try. We have to check if all selector functions that are retrieved this way are called in callbacks. |
|
It doesn't have to be here, I'm just proposing an idea, we can start by a clear piece of documentation. |
|
When I look at the React lint rules for hooks, it's a bit discouraging to know that they need almost 2000 lines of code to create the exhaustive dependencies hook. |
| @@ -793,9 +793,25 @@ any price in the state for that currency is retrieved. If the currency prop | |||
| doesn't change and other props are passed in that do change, the price will | |||
| not change because the dependency is just the currency. | |||
|
|
|||
| When data is only used in an event callback, the data should not be retrieved | |||
| on render, so it may be useful to get the selectors function instead. | |||
There was a problem hiding this comment.
I think we should say something like: don't use this API while calling the selectors directly on the render function, your component won't rerender when changes happen.
There was a problem hiding this comment.
This actually makes me wonder: why should we retrieve the selectors during render at all if we could also retrieve them in the callback itself? In other words, in the callbacks, we could just use const { selector } = select( key )?
There was a problem hiding this comment.
The global select/dispatch/subscribe function should be avoided as much as we can. These don't rely on the RegistryProvider. For me ideally, these should become WP-only APIs added as inline styles and not something the package exports by default.
There was a problem hiding this comment.
That would have the additional benefit of not having to pass the function as a dependency of callbacks. I'll explore this quickly in a separate PR.
There was a problem hiding this comment.
I think you misread my comment, we shouldn't do it :)
There was a problem hiding this comment.
Oh, Github didn't show me your comment at the time so I didn't see it :)
| _Parameters_ | ||
|
|
||
| - _\_mapSelect_ `Function`: Function called on every state change. The returned value is exposed to the component implementing this hook. The function receives the `registry.select` method on the first argument and the `registry` on the second argument. | ||
| - _\_mapSelect_ `Function|WPDataStore|string`: Function called on every state change. The returned value is exposed to the component implementing this hook. The function receives the `registry.select` method on the first argument and the `registry` on the second argument. When a store key is passed, all selectors for the store will be returned. This is only meant for usage of these selectors in event callbacks, not for data needed to create the element tree. |
There was a problem hiding this comment.
I added documentation now. I feel like we're overloading the useSelect a bit though, so this may be another argument to create a separate useSelectForCallbacks hook. It could be simpler code-wise as well since we don't need to add logic for skipping the mapping.
There was a problem hiding this comment.
Either way, it would be rather confusing whatever we do at this stage. We could have useSelectors or something like that but you still need to read docs to understand the difference so maybe it's fine to add jQuery-like flexibility in terms what passed arguments trigger 😄
|
Thanks for the quick reviews! Let's try this and also use it anywhere we're just using the data in a callback. |
|
I think in principle this approach has merit. I'm personally leaning more towards a separate (experimental) hook to start off with. I'm wary of having significant difference of behaviour caused by different argument types passed into The other concern I have with overloading the existing function behaviour is it requires some understanding of |
|
Right, both ways have benefits. I don't mind either way. |
| @@ -62,6 +57,7 @@ function useAutosaveNotice() { | |||
| } ), | |||
| [] | |||
| ); | |||
| const { getEditedPostAttribute } = useSelect( 'core/editor' ); | |||
There was a problem hiding this comment.
You forgot to remove getEditedPostAttribute from the previous useSelect call. :)
|
When calling const { getBlockList } = useSelect( 'core/blocks' );
setTimeout( () => { console.log( getBlockList() ); }, 1000 );the Calling If we're going to recommend this pattern, we should document how exactly the state reading is done. |
|
I believe the experimental thunk actions introduced in #27276 could greatly simplify the components modified in this PR. We could verly clearly separate the data that we need to render the UI and the data that we need to perform a UI action: const handleKeyDown = ( clientId, keyCode ) => ( { select, dispatch } ) => {
if ( keyCode === DELETE ) {
return dispatch.removeBlock( clientId );
}
if ( keyCode === DOWN ) {
const selectedBlockClientId = select.getSelectedBlockClientId();
dispatch.moveBlockToPosition( clientId, selectedBlockClientId );
}
}
function BlockSelectionButton() {
const dispatch = useDispatch( blockEditorStore );
const onKeyDown = ( event ) => dispatch( handleKeyDown( clientId, event.keyCode ) );
return <div onKeyDown={ onKeyDown } />;
}This way, the Without thunks, both functions need to be melted together in one big function, with little hope to separate them out. |
|
Where's the value in separating them? And can't you do this same currently like this: |
Calling getBlockList (the selector) implies that you get the data fresh from the store, so the data at the time of calling the selector. While this is clear to me, we could mention it in the documentation too. |
It's valuable in the same way as refactoring in general is perceived as valuable. Breaking up something complex into multiple simpler parts and the composing them in a simple way. Removing code smells like "long functions" or "high cyclomatic complexity".
Where import { dispatch, useSelect } from '@wordpress/data';
function onKeyDown() {
dispatch( 'store' ).moveBlockDown();
}
function BlockList() {
const blockList = useSelect( select => select( 'store' ).getBlockList() );
return <ul list={ blockList } onKeyDown={ onKeyDown } />;
}Here both functions get their data registry from two very different sources. When dispatching a thunk action, the registry always comes from the same source. These kinds of issues may seem minor, but can cause major headaches when trying to use the Gutenberg modules outside the Gutenberg plugin itself, in a different context and environment. |
|
No, I mean like this: const handleKeyDown = ( clientId, keyCode, select, dispatch ) => {
if ( keyCode === DELETE ) {
return dispatch.removeBlock( clientId );
}
if ( keyCode === DOWN ) {
const selectedBlockClientId = select.getSelectedBlockClientId();
dispatch.moveBlockToPosition( clientId, selectedBlockClientId );
}
}
function BlockSelectionButton() {
const select = useSelect( blockEditorStore );
const dispatch = useDispatch( blockEditorStore );
const onKeyDown = ( event ) => handleKeyDown( clientId, event.keyCode, select, dispatch );
return <div onKeyDown={ onKeyDown } />;
} |
Oh yes, I see. I didn't realize that's possible. That would work 👍 |
* useSelect: normalise getting selectors for callbacks * Add docs * Address docs feedback * Clean up a bit
|
I just found out about this. What's the difference between |
|
@kevin940726 I'm assuming you're asking about the global exported "select" function. Both global select, dispatch and subscribe should be considered bad patterns because they only work on the default registry and not on custom |
|
@youknowriad Thanks for the explanation! Is it documented anywhere? Have we considered deprecating global |
|
@kevin940726 For a lot of third-party plugins, it's the only way to extend the editor (access and manipulate data) in the editor. I'd say these functions only make sense in the context of WordPress but not in the context of the packages (core gutenberg or third-party JS usage). I guess this means we can't deprecate them officially but yeah mentioning this at least in the architecture docs would be great. I think we can also consider moving the declaration of these function to |
Description
The pattern of just getting selectors to be used for callbacks (vs for the render tree) becomes more widely used, yet it almost feels discouraged, wrong or awkward to return selectors in a select map instead of calling them immediately.
useSelecthas mostly been used that way: call selectors and return the result. Returning the selector functions almost feels like a hack that you're not supposed to do.This PR adjusts
useSelectto allow getting selectors in a way that's less verbose and that "feels" right. It's similar to howuseDispatchworks.Like this, you can get the selectors and call them in callbacks without it looking awkward and encourages this pattern to improve performance. We should generally avoid calling selectors more often if the result is not going to be used in the render tree.
With this pattern, you also don't have to provide dependencies since it will always be empty (
[]).Additionally, we can make some performance optimisations internally in
useSelectwhen you're not mapping the results. We can just skip all mapping logic.How has this been tested?
Everything should work as before.
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).