Add useStoreSelectors hook #26692
Add useStoreSelectors hook #26692
Conversation
adamziel
added
[Package] Data
adamziel
requested review from
draganescu,
mcsf,
youknowriad,
noisysocks,
talldan,
gziolo and
kevin940726
|
Size Change: +29 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
What if we limit useSelect to a single store per call? If you want to use multiple store, make multiple calls. This is already the case for useSelect( 'core/block-editor', ( { getSomething } ) => getSomething( dependency ), [ dependency ] );That also makes it more similar to useSelect( storeKey, mapping, dependencies );
useDispatch( storeKey ); |
|
@ellatrix I like it! let's try that |
| if ( arguments.length === 3 ) { | ||
| [ storeKey, _mapSelect, deps ] = arguments; | ||
| } else { | ||
| // Backwards compatible signature | ||
| storeKey = null; | ||
| [ _mapSelect, deps ] = arguments; | ||
| } |
There was a problem hiding this comment.
This is here to keep the arguments in README and make the linter happy. I don't like it too much. The alternative is having a new useStoreSelect selector and deprecate this one, but I don't like a longer name for the recommended default. Thoughts?
There was a problem hiding this comment.
Backwards compatibility code is good to have?
useStoreSelect would be interesting if we export it for the modules creating the store instead of importing the general useSelect from the data module. Tbh, I think that makes more sense than hardcoding the keys as strings. @gziolo was also working on a PR to export keys from the components creating the stores.
There was a problem hiding this comment.
It is good to have, I just don't like processing arguments like that.
useStoreSelect would be interesting if we export it for the modules creating the store instead of importing the general useSelect from the data module. Tbh, I think that makes more sense than hardcoding the keys as strings. @gziolo was also working on a PR to export keys from the components creating the stores.
Sounds like same goes for useDispatch(). It would work, we'd just have to update all the store-registering modules and remember about exporting this in the newly created ones 🤔
There was a problem hiding this comment.
Hm..
// Deprecated, calls the callback with a `select` function
import { useSelect } from '@wordpress/data';
// Recommended, calls the callback with module-specific selectors:
import { useSelect as useCoreDataSelect } from '@wordpress/core-data';
import { useSelect as useEditWidgetsSelect } from '@wordpress/edit-widgets';I'm not sure yet if I like it or not 😄
There was a problem hiding this comment.
Similarly:
// Deprecated, a function expecting a module as an argument
import { useDispatch } from '@wordpress/data';
// Recommended, function that takes no arguments
import { useDispatch as useCoreDataDispatch } from '@wordpress/core-data';
import { useDispatch as useEditWidgetsDispatch } from '@wordpress/edit-widgets';There was a problem hiding this comment.
I'm intrigued though, I'll get in touch with @gziolo and let's see where it goes.
|
@ellatrix I updated the code and PR description. |
|
It would be nice it update e.g. all |
|
Performance improvements aside, I prefer this API. There's less boilerplate and it makes |
| if ( arguments.length < 3 ) { | ||
| // Backwards compatible signature | ||
| [ storeKey, _mapSelect, deps ] = [ | ||
| null, | ||
| arguments[ 0 ], | ||
| arguments[ 1 ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Can we instead just check the type of the first argument? If it's a function then it's mapSelect, if it's a string then it's storeKey?
There was a problem hiding this comment.
I agree that it might be handled differently.
In general, extending API surface for useSelect creates some challenges:
- how to update JSDoc comment to cover new usage to ensure that proper documentation is generated for developers
- how to announce new API in a way that it doesn't become confusing, is it clear when to use which form?
| @@ -203,4 +203,79 @@ describe( 'useSelect', () => { | |||
| } | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'uses memoized selector if dependent stores do not change', async () => { | |||
There was a problem hiding this comment.
Nitpick: we don't need the async keyword here?
| const selectSpyFoo = jest | ||
| .fn() | ||
| .mockImplementation( ( { getCounter } ) => getCounter() ); |
There was a problem hiding this comment.
Nitpick: We can just write it like
| const selectSpyFoo = jest | |
| .fn() | |
| .mockImplementation( ( { getCounter } ) => getCounter() ); | |
| const selectSpyFoo = jest.fn( ( { getCounter } ) => getCounter() ); |
There was a problem hiding this comment.
Isn't it how Prettier formats it? I would personally leave all formatting choices to tools.
There was a problem hiding this comment.
I think @kevin940726 meant omitting the .mockImplementation part
|
More audacious explorations here: #26723
Totally agreed! I'll be happy to explore that once we settle on specific API so that my explorations can double as a merge-able PR. As a side note - even if the performance gains are marginal I think we came up with a cleaner API that's worth having anyway. |
There was a problem hiding this comment.
Nice exploration. I wanted to point to prior work done in the same area at the tame when React hooks weren't so widely used, but some of the challenges might still be valid:
- Data: withSelect: Subscribe only to reducer keys referenced in mapSelectToProps #12877
- Packages: Add package babel-plugin-transform-with-select #13177
The main reason why this idea was abandoned is:
There are good ideas here, but with consideration of registry-enhanced selectors as introduced by #13662, the ability to determine store dependencies from a withSelect has become much more complex than this naive implementation can support.
where #13662 allows to:
In some situations, you want to build selectors that target multiple stores at the same time. Until now we were relying on the global select function but the issue is that it only targets the default registry. If we have a separate provider, this might not work as expected. In this PR, I'm introducing a
createRegistrySelectorhelper used to mark a selector a cross-stores selector and providing a registry object.
An example of the selector that depends on another store:
gutenberg/packages/block-directory/src/store/selectors.js
Lines 67 to 76 in a63cc62
A follow-up comment might be still relevant in this context: #13177 (comment).
There is also this issue that should be probably addressed first, which is how to ensure that there is a simple way to identify a given store as in many aspects using hardcoded strings doesn't scale well. See my proposal in #26655.
| if ( arguments.length < 3 ) { | ||
| // Backwards compatible signature | ||
| [ storeKey, _mapSelect, deps ] = [ | ||
| null, | ||
| arguments[ 0 ], | ||
| arguments[ 1 ], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
I agree that it might be handled differently.
In general, extending API surface for useSelect creates some challenges:
- how to update JSDoc comment to cover new usage to ensure that proper documentation is generated for developers
- how to announce new API in a way that it doesn't become confusing, is it clear when to use which form?
|
There is one more thing that we should keep in mind when working towards this proposal. In some cases The more high-level components, the higher the chance that this optimization is going to be harder to apply. |
|
My feedback here aligns very much along the lines of what @gziolo has already outlined. However, I do like the idea of optionally being able to subscribe to a single store (this has come up numerous times). Before committing to this significant change though, have you done any performance measurements to see what actual impact this has? Relying on |
|
All good notes, thank you @gziolo and @nerrad !
Of course! My point was more illustrative than qualitative. I didn't talk about actual performance improvements because I wasn't quite sure how to measure the potential gains without actually refactoring everything to store-specific selectors. Some baseline measures I took in chrome devtools showed that runSelector takes anywhere between 8%-13% of total blocking time depending on how I use the editor (fast typing, pressing enter, triggering toolbar). If I interact with the editor heavily and JS gets blocked occasionally, that's more like 8%-13% of the real time, e.g. 130ms out of every second is spent on just re-running selectors. I hoped maybe we could get that to ~20ms by just not calling most of them. I spent more time thinking about this and I found a way to assess potential gains. I figured I could keep track of the stores used by each selector like this: const dependentStore = useRef();
const mapSelect = useCallback( ( select, registry ) => {
registry.__experimentalStartRecordingSelectStores();
const retval = _mapSelect( select( storeKey ), registry );
const stores = registry.__experimentalStopRecordingSelectStores();
const newDependentStore = stores.length === 1 ? stores[0] : false;
if ( newDependentStore !== dependentStore.current ) {
dependentStore.current = newDependentStore;
forceRender();
}
return retval;
}, deps );Which then makes it trivial to subscribe to either the entire registry or just a single store, depending on the value of if ( dependentStore.current ) {
unsubscribe = registry.stores[ dependentStore.current ].subscribe( onChange );
} else {
unsubscribe = registry.subscribe( onChange );
}Which, by the way, makes it possible to keep track of the exact stores a selector needs to subscribe to even when registry selectors are involved (cc @gziolo @kevin940726). Unfortunately, even with that in place I saw the same 8%-13% range in the performance monitor. Boo hoo. If that makes sense as a methodology, I think it concludes the part of the discussion about performance gains. There's also the part about having a potentially nicer API like this: const {
selectionStart,
selectionEnd,
} = useStoreSelectors(
'core/editor',
( { getEditorSelectionStart, getEditorSelectionEnd } ) => ( {
selectionStart: getEditorSelectionStart(),
selectionEnd: getEditorSelectionEnd(),
} )
);Or even this, using a shorthand notation: const {
getEditorSelectionStart: selectionStart,
getEditorSelectionEnd: selectionEnd,
} = useStoreSelectors( 'core/editor', [ 'getEditorSelectionStart', 'getEditorSelectionEnd' ] );I updated this PR with a draft of what could become |
|
I guess it's very difficult to verify the overall impact on performance without doing a major refactor.
One interesting thing about introducing
I'm not entirely sure how this shorthand version would work. Given the efforts required to explain how this notation should be used and in which cases it is applicable, it's probably better to remove it from the scope of this PR. |
|
@gziolo I removed the array shorthand and added tests to this PR. I still think there's a value in the array notation, but let's call it out of scope for this PR - I may explore it in another one, maybe as another hook. |
packages/data/README.md
Outdated
| @@ -712,6 +712,24 @@ _Returns_ | |||
|
|
|||
| - `Function`: A custom react hook. | |||
|
|
|||
| <a name="useStoreSelectors" href="#useStoreSelectors">#</a> **useStoreSelectors** | |||
|
|
|||
| A shorthand for useSelect() that returns all the selectors from a given store. | |||
There was a problem hiding this comment.
useSelect is explained as:
Custom react hook for retrieving props from registered selectors.
Documentation should follow the same theme. Doesn't it rather returns data processed by selectors from a single store.
There was a problem hiding this comment.
A usage example would be very helpful as well.
One more, why did you pick the ending Selectors rather the Select that the original hook uses?
There was a problem hiding this comment.
One more, why did you pick the ending Selectors rather the Select that the original hook uses?
The original goes like this: useSelect( (select) => select('store').selector() ) - what you get is the select function, hence the name useSelect makes sense.
This hook goes like: useStoreSelectors( ({selector}) => selector() ) - what you get are all the selectors from a given store, hence the name useStoreSelectors makes more sense than useStoreSelect.
There was a problem hiding this comment.
Yes, it sounds convincing. 👍
Aside: I'm wondering if @nerrad had the same mindset when proposing useSelect :)
adamziel
force-pushed
the
try/use-select-store-deps
branch
from
887f5fd to
c83ad3f
Compare
|
Hey there! I'm not really convinced about this new hook, for me it's just a duplicate API over useSelect. it's a bit nicer for sure if you just need selectors from a store but that's rather a specific use-case and I'm not sure we want to maintain duplicate APIs for this. |
|
Also, why selectors and not actions? For me, it feels like all what this will end up doing is losing consistency in our components since most would still need useSelect while some would use that one. Is the shorthand syntax here worth it enough to introduce this inconsistency and force contributors to learn a new hook? |
|
I don't get the merged API. 🤔 So we're not reducing the amount of the subscriptions and selector calls by subscribing to one store only? We still go for the hardcoded store key strings? |
|
@youknowriad @ellatrix I think this is pivoting into a more general discussion about selectors and registry API. In the next hour I'll spin a new issue to fully describe my thinking and potentially centralize all the different threads we're pulling. It may or may not be resolved quickly - what do you think about renaming this one to |
|
Actually I changed my mind, let's at least try to continue here. I'm still happy to create a new issue for that anytime if someone brings it up. @youknowriad This work is very emergent but I'll do my best to answer all the questions. It will be indirect at first, but I promise I'll get there! OverviewI imagine the data flow across components and selectors as a complex graph like this:
Circles are selectors. Some selectors are simple and rely on just a single store. Other selectors depend on selectors from other stores which means they require registry access. Since we don't declare dependencies statically, it's hard or impossible to tell which selectors rely on which stores. Thus the components must listen to all changes on the entire registry:
This is essentially a star topology with every change affecting every node in one way or another. Adding nodes and interactions scales non-linearly and following the graph-based data flow is harder than necessary. Now, what is emerging from my recent discussions with @gziolo is that this entire model could be simplified like this:
In this model, there are no registry selectors (an idea explored in another thread). Instead, components pull the data directly from the stores they are interested in. There is no concept of dependency graph at all, only a shallow list of dependent stores. Of course, registry selectors sometimes contain complex logic - where should that logic go? Well, there already exists a mechanism that makes it possible to achieve exactly the same thing as registry selectors: React hooks. RationaleAs noted in another thread, registry selectors may be a solution to a problem we just don't have anymore in core. If that's true, then they are just an inferior alternative to react hooks:
Also, as @nerrad mentioned above, the idea of subscribing to specific stores came up multiple times already:
Where we are nowAs you noticed, for now this is just a duplicate API. I am a big proponent of having a moderate amount of well-placed shorthands, I believe they make the final code more succinct and easier to read. The maintenance and learning cost doesn't seem large to me as it is just a simple wrapper fulfilling the mythical 100% code coverage goal. However, I am fairly new to Gutenberg so I will fully trust the judgement of more experienced folks. But let's look beyond that. Having a store-specific selector is a necessary first step for store-based subscriptions which I believe is an ideal state worth pursuing. Besides the rationale laid out in the previous sections, there are potential performance gains involved. They are hard to assess upfront, for example I tried here: #26692 (comment) I found that selectors-related code takes 10% of the blocking time when typing heavily. That's not 90% or 40% and there are definitely heavier tasks such as multiple re-renders going on. Still, it's 10% that we could potentially take down to 5% or 2%. I don't have a good idea how to reliably measure the impact of a full refactor. I tried to hack together a performance test, but it didn't show anything meaningful so I just abandoned this line of discussion until I'm able to get a better measurement. Surface areaNone of what you'll see below includes the numbers for registry selectors - that would be much harder to analyze. Still, I took a few naive measures to assess how many components relies on a single store vs multiple stores. According to the pipeline below: There are 177 components relying on just a single store, 30 relying on two stores, and only a single component relying on 6 stores. I also measured how many components rely on each store: Direct answersI talked about the maintenance/learning cost earlier, but there was also a concern @youknowriad raised about the consistency:
My thinking is that if we can get rid of the dependency graph, it would be worth it. But yes, at the moment this is just a tiny wrapper that doesn't do much.
I'd like to get there in the end, this is just a "foot in the door" so to speak. I'm happy to make it an experimental API for now.
I'm all for object-based store keys, it's just beyond the scope of this PR. @gziolo is working on that change in another PR I believe - I don't want to steal his thunder :-) Other considerations
const { parentBlockType, firstParentClientId, shouldHide } = useSelect(
( select ) => {
const {
getBlockName,
getBlockParents,
getSelectedBlockClientId,
} = select( 'core/block-editor' );
const { hasBlockSupport } = select( 'core/blocks' );
const selectedBlockClientId = getSelectedBlockClientId();
const parents = getBlockParents( selectedBlockClientId );
const _firstParentClientId = parents[ parents.length - 1 ];
const parentBlockName = getBlockName( _firstParentClientId );
const _parentBlockType = getBlockType( parentBlockName ); |
How is this any different from any redux store, it's still a global store with everything in it. The only difference in the data module is that we try to split some smaller stores in an organized fashion low level stores but in the end, the top level store say edit-post or editor, still need too access data across stores and merge them in a single selector. If we don't do it at the selector level like we do with
Overall, my opinion is that we're trying to solve the wrong problem here tbh. What are we trying to achieve with these? It'sn to clear to me, it's it about performance? Show me the numbers this has, I don't believe it has any meaningful impact personally but happy to be wrong here. For me, until we know what we're trying to solve, it's very premature to introduce APIs we might regret later. |
|
@youknowriad let's separate two threads we're touching on here. Thread 1 - this PR
For now I don't have much more than my strong opinion about shorthands 😄 I hear what you're saying, let's revert this PR and revisit if / when the part below reaches a conclusion. Thread 2 - the bigger picture
The big picture isn't inherently different, it's just this is a highly interactive app with a high number of state actions and subscriptions. It differs in details - the store is divided into smaller modules. IMHO that division could involve less code and a simpler mental model.
This part is more about the graphs and circles I posted above. I think core data is pretty complex and I think we are very much capable of reducing that complexity. Performance gains are possible but since it's hard to take reliable measures upfront, that's more of a gamble than a solid basis.
Not necessarily duplicating, my thinking is that the logic could still live in a single place. Consider: // component.js:
const postContent = useSelect(
( select ) => select( 'core/editor' ).getEditedPostContent(),
[]
);
// selectors.js:
export const getEditedPostContent = createRegistrySelector(
( select ) => ( state ) => {
const postId = getCurrentPostId( state );
const postType = getCurrentPostType( state );
const record = select( 'core' ).getEditedEntityRecord(
'postType',
postType,
postId
);
if ( record ) {
if ( typeof record.content === 'function' ) {
return record.content( record );
} else if ( record.blocks ) {
return serializeBlocks( record.blocks );
} else if ( record.content ) {
return record.content;
}
}
return '';
}
);The following hook seems to be interchangeable: // component.js:
const postContent = useEditedPostContent();
// hooks.js
export const useEditedPostContent = () => {
const [ postId, postType ] = useStoreSelectors( 'core/editor', ( { getCurrentPostId, getCurrentPostType } ) => ([
getCurrentPostId(), getCurrentPostType()
]));
const record = useStoreSelectors( 'core', ( { getEditedEntityRecord }) => getEditedEntityRecord(
'postType',
postType,
postId
), [ postType, postId ]);
if ( record ) {
if ( typeof record.content === 'function' ) {
return record.content( record );
} else if ( record.blocks ) {
return serializeBlocks( record.blocks );
} else if ( record.content ) {
return record.content;
}
}
return '';
} );Although I'm not sure what to do with usages like this: |
|
Either way, the conclusion for now is to revert this PR. If I can come up with a solid design improvement for selectors, I'll create a new issue. |
|
@adamziel, great job doing all explorations, I personally learned a ton. The discussion triggered in this PR following the merge is a clear sign that the data layer became very complex over time because of many legacy implications like the limitation HOCs created. Hooks open new possibilities that are worth exploring but the number of backward compatibility considerations makes the process extremely challenging. @youknowriad thanks for sharing more context which isn't obvious to other contributors that can't follow closely development happening in this area. |
youknowriad
removed
the
Needs Dev Note
Description
This PR evolved into a shorthand hook called
useStoreSelectors, used like this:Dev notes
Added useStoreSelectors hook
useStoreSelectorsis a custom react hook for retrieving selectors from registered stores. It could be considered a shorthand foruseSelect()that provides all the selectors from a given store. For example:In the above example, when
HammerPriceDisplayis rendered into an application, the price will be retrieved from the store state using themapSelectorscallback onuseStoreSelectors. If the currency prop changes then 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 willnot change because the dependency is just the currency.
Original description for posterity:
Every key stroke in the editor re-runs all registered selectors, sometimes multiple times.
useSelect()is great for monitoring state changes. but problem with it is that themapSelectfunction is executed on each registry change, related or not. Fortunately the component is only re-rendered if the newly computed value is different from the last one, but the selector is still executed every time.This PR proposes a first argument to
useSelectcalledstore. If provided,useSelectwill only subscribe to specific stores instead of the entire registry. Consider the following setup:Typing "Gutenberg" in the textarea would log the following:
Before:
After:
Adding store dependencies like:
Reduces the number of times the selector is triggered:
Note that the following code implies that
depswill be passed touseCallback:I think it's an okay limitation though as the alternative would be fairly complex to implement. One is better off using
depsanyway.Other considerations
Specifying dependent stores for all selectors should yield noticeable performance gains, but we can go even further. Some ideas:
mapSelectimmediately, instead use throttle of let's say 10ms (Throttle useSelect calls #26695)How has this been tested?
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: