-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@wordpress/data: Create useSelect
hook.
#15512
Conversation
Thiis receives a store name and returns enhanced selectors for the given store: - an object is returned of all new selector hooks. - Each selector has been converted to a custom hook that has its own subscription to the store state.
To demonstrate its viability.
Related discussion in Slack (link requires registration): https://wordpress.slack.com/archives/C02QB2JS7/p1557341876475900 |
const unsubscribe = subscribe( () => { | ||
setResult( selector( ...args ) ); | ||
} ); | ||
return () => unsubscribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing I'd like to explore here once #15436 is available, is if the selector has a resolver and the return is undefined
then don't set the state. This would prevent unnecessary renders while the selector is resolving. It effectively would normalize undefined
returns to null
for all selectors with resolvers. That's one issue I've seen in the wild with withSelect
currently in that unless the mapping function handles undefined
results, re-renders are more frequent.
|
||
function PostTrashCheck( { isNew, postId, children } ) { | ||
export default function PostTrashCheck( { children } ) { | ||
const { isEditedPostNew, getCurrentPostId } = useSelect( 'core/editor' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convention for hooks is kinda broken here, so I think I'll have to make sure that we return the custom enhanced selector hooks with the use
prefix. So this would become something like:
const { useIsEditedPostNew, useGetCurrentPostId } = useSelect( 'core/editor' );
Necessary because otherwise it won't be obvious (without knowing the internals ) that these are in fact react hooks and thus the rules of Hooks apply to them.
closing this in favour of the approach happening on #15737 |
Description
See #15473 for background. The purpose of this pull is to do some experiments on a new
useSelect
hook. In this initial iteration:useSelect( storeName: string ): Object
Example usage:
gutenberg/packages/editor/src/components/post-trash/check.js
Lines 1 to 15 in faa39a7
Rationale:
Subscriptions are not set until a returned enhanced selector is invoked. This does allow for some lazy subscription registration on selector calls. Example:Some things to consider & potential drawbacks:
withSelect
). For instance, one of the concerns here is that with multiple selectHooks with resolvers, there's more re-rendering that could happen as each selector is resolved.mapSelectToProps
option as that would reduce the number of registry subscriptions. However in the case of providing a mapSelectToProps, the returned object would be the props returned from themapSelectToProps
function. This version would be more similar to the current behaviour ofwithSelect
use
prefix. Although I didn't do that there, there's some advantages to returning our enhanced selectors with that prefix because it: 1. makes it clear these are hooks and should be treated as such. 2. Exposes them to the eslinting on hooks (which helps prevents incorrect usage - such as implementation order matters).Todos:
mapSelectToProps
like callback and if its a string, it's assumed to be a store name (and thus return the enhanced selectors on that store).withSelect
to incorporate a similar signature as what we land on foruseSelect
(although I don't think we need to strive for complete consistency because there is varying paradigms in play here).Dependencies for this work:
useContext
for the registry is possible.How has this been tested?
Checklist: