Refactor useSelect usages to useEntityRecords #38827
Conversation
adamziel
requested review from
ajitbohra,
draganescu,
getdave,
kevin940726,
noisysocks,
talldan and
tellthemachines
as code owners
|
Size Change: +2.3 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
@gziolo I wonder what to do with usages like this one: Where
I'm curious about your thoughts here |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
The usage of useEntityRecord looks great. We need to measure performance implications for useEntityRecords because we pass a new object instance on every component's re-render. I left a note next to the usage in code.
I really like how more predictable code becomes in addition to the less line of code. Developers should also benefit from TypeScript support when using IDEs. I like the direction very much 😄
| const { categories, isResolving } = useEntityRecords( | ||
| 'taxonomy', | ||
| 'category', | ||
| query |
There was a problem hiding this comment.
Just double checking here. On every re-render, we will have a different query object here, but it shouldn't matter because @wordpress/core-date picks data based on the standardised key computed from the query, right? I guess what I mean here is that before useSelect would get executed only when showOnlyTopLevel changes or when the store triggers an update. However, with the new implementation, it has to run useEntityRecords every time we pass a new query object.
I believe the same concern applies to every usage of useEntityRecords where query is dynamically computed which is all cases in this PR. We should check if there are any performance implications here.
There was a problem hiding this comment.
@gziolo AFAIR deps are tested for shallow equality, so it doesn't matter as long as there are no nested objects such as Edit: I think I got that wrong.{ _fields: [ 'id' ] }. In such a case, it would re-run the select callback, which then will return the exact same data.
This may or may not have performance implications, depending on the specifics. I'd like to make sure this just never re-runs. I have two ideas:
- Add useMemo() in all such places
- Don't include queryArgs in deps and instead, rely on
JSON.stringify( queryArgs )– this removes the need to remember about useMemo() and should still be fast.
cc @kevin940726
There was a problem hiding this comment.
Instead of trying to guess whether the query has changed or not, I removed it as a dependency and added an additional deps argument instead.
There was a problem hiding this comment.
I would recommend not introducing new APIs with custom dependencies array, as suggested by the React team (with that in mind, we should probably refactor useQuerySelect at some point):
Generally saying, we recommend most custom Hooks to not use the dependencies argument, and instead provide a higher-level API that is more focused around a specific use case.
Serializing the query into a stabilized key is a possible solution. I believe getEntityRecords has already done that internally as @gziolo mentioned. We could do similar things here, maybe implicitly in useQuerySelect? I think this is how react-query does it too, but they leave this option to the users to provide their own stabilized key for each query.
I would love to see this hook refactored first in a way where we branch code when Maybe it's not worth the effort trying to use those new hooks in this place. In general, the rules of hooks and conditional code execution doesn't play along well 😞
That's a good reason to postpone the decision for now and revisit when you have some examples where this pattern is exercised. |
adamziel
force-pushed
the
refactor/use-entity-hooks
branch
from
1d98151 to
77e854a
Compare
adamziel
requested review from
TimothyBJacobs,
ellatrix,
mkaz,
nerrad,
ntwb,
ryanwelcher and
spacedmonkey
as code owners
adamziel
force-pushed
the
refactor/use-entity-hooks
branch
from
0122560 to
7549861
Compare
|
All the tests pass, yay! cc @gziolo; I still need to write the PR description, but let's not block reviewing on that |
|
I just wanted to share a quick observation before I take a deeper dive into the code changes proposed. I see
|
Co-authored-by: Greg Ziółkowski <[email protected]>
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
I'm happy about the current state of PR. I would encourage you to wait for some confirmation from folks familiar with the Navigation and Widget blocks before you move forward with this PR.
|
Luckily, I worked on both :-) But of course this would still greatly benefit from more eyes, cc @noisysocks @talldan @draganescu @getdave |
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
This looks good from a Navigation POV.
Nit: although more verbose and time consuming it might have been easier to break this up into smaller PRs allowing them to be reviewed merged incrementally.
Great work though - this new API makes this kind of thing a lot more readable.
There was a problem hiding this comment.
Where getEntityRecord is conditional. I see the following solutions:
I wonder could we modify the API to allow for something like this:
const isImmediate = false;
const {
execute: fetchMenuItems,
records: menuItems
isResolving,
hasResolved
} = getMenuItems( ...menuItemsParameters, isImmediate);
useEffect(()=> {
if(hasSelectedMenu) {
fetchMenuItems();
}
},[hasSelectedMenu, fetchMenuItems ])
return {
menuItems: menuItems,
hasResolvedMenuItems: hasResolved,
};I would make execute() an optional API which you could consume as required for edge cases such as this. It would be the responsibility of the consumer to invoke it to trigger the resolution.
Passing false to the hook defers the resolution of the selector until execute is called. We would make this true by default thereby meaning the 80% use case would still have the API you've already designed.
Also noting this would require some perf checks to ensure that fetchMenuItems is wrapped to ensure a consistent reference.
|
The Legacy Widget block changes lgtm 👍 |
const { records, isResolving, hasResolved } = useEntityRecords(
'root',
'menu',
{ per_page: -1, context: 'view' },
{ enabled: shouldEnabled }
);Or maybe we can refactor useEntityRecords('root', 'menu', { per_page: -1, context: 'view' });
// is essentially the same as...
useQuerySelect((query) => {
return query(coreStore).selectAll('root', 'menu', { per_page: -1, context: 'view' });
});
// So that skipping/pausing a query is as trivial as...
useQuerySelect((query) => {
if (enabled) {
return query(coreStore).selectAll('root', 'menu', { per_page: -1, context: 'view' });
}
});Not sure how possible the second solution is, but it could solve a lot of the issues with hooks at the same time (conditional, loops, infinite loading, etc). |
Co-authored-by: Robert Anderson <[email protected]>
|
The discussion seems to be mostly about the conditional execution – let's keep it going separately, and I'll go ahead and merge this PR. |
Description
This PR refactors the verbose usages of
getEntityRecordandgetEntityRecordslike this:Into more succinct ones like this:
Based on the new useEntityRecord and useEntityRecords hooks introduced starting at #38135
Test plan
Confirm all the tests are green