Add an "enabled" option to useEntityRecord and useEntityRecords #39288
Conversation
adamziel
added
[Type] Code Quality
|
Size Change: -5 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
Does the abstraction have to cover every use case? I'd say better to cover 80% of cases and keep using |
|
Alternatively maybe something simple is to make const { records, hasResolved } = useEntityRecords(
'root',
'menuItem',
menuId && {
menus: menuId,
per_page: -1,
context: 'view',
}
); |
|
Thanks for exploring this. Could you advise how you'd handle the following with the current proposal? <button onClick={triggerFetchMenuItems}>Get the Menu Items</button>I don't love the
There's a reasonable argument here. That said, it does feel like we're saddling the logic for running the query into the hook rather than asking the consuming component to handle it. |
This is exactly the same thought I have. While it addresses an edge case, the API is very confusing. In general, avoiding running React hooks conditionally is limitation number one: https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
In practice, you might want to have this type of conditional execution for most of the React hooks that might trigger resource-intensive operations. I did a quick Google search and maybe some sort of a higher-order React hook like in https://dev.to/lexlohr/conditional-hooks-1fpj could do the trick. Both the original proposal and the alternative shared by @noisysocks in #39288 (comment) workaround this limitation in a clever way. There is another alternative, provide a local hook that does the same trick: function useMenuItemEntityRecordsConditionally( execute, menuId ) {
if ( ! execute ) {
return {};
}
const { records, hasResolved } = useEntityRecords(
'root',
'menuItem',
{
menus: menuId,
per_page: -1,
context: 'view',
},
);
return { records, hasResolved };
} |
<button onClick={triggerFetchMenuItems}>Get the Menu Items</button>To me, this seems like a use case best handled by dispatching an action imperatively rather than using this hook. However, I could imagine something like a
It's used in
I think this still violates the rules of hooks, you can't put hooks inside conditional, even when it's inside a custom hook. |
Correct. The other term is potentially quite emotive and the enable/disable is a much better description of what we're attempting. |
Right, good catch. Indeed, it technically goes against those rules. Edit: |
I don't think we can, and I don't suggest we do. IIRC, the ordering of the hooks is the hard requirement of the hook rules, it's an error level rule in eslint, not the same as exhaustive deps which is only a warning level rule. I think conditionally calling a hook will break the application since React will lose track of the hooks the component's calling. Here's a deep dive into why calling order matters in React hooks. |
|
Given the discussion, what is your stance on this API now @gziolo? Do you have any alternative ideas, given that the conditional local hook wouldn't work with React? |
I think my previous answer to the comment from @noisysocks is still valid:
If you all agree that this API is going to be popular enough then feel free to introduce it. You definitely shouldn't hold the functionality proposed in this PR only because I very much dislike the limitations of React hooks which negatively impact developer experience. We at least tried to exercise some alternatives. 😄 One note, if you land this functionality for |
In my mind, this is within the boundaries of the 80% use case. Even across the Gutenberg codebase, we have a lot of these gutenberg/packages/block-library/src/comment-template/edit.js Lines 242 to 248 in 9b0e9e4 gutenberg/packages/block-library/src/post-terms/use-post-terms.js Lines 24 to 36 in 9b0e9e4 gutenberg/packages/edit-post/src/editor.js Lines 70 to 73 in 9b0e9e4 gutenberg/packages/editor/src/components/page-attributes/parent.js Lines 82 to 87 in 9b0e9e4 If you inspect these files, these are often paired with
@gziolo Oh yes, I would love to be able to just use an I did mark that option as an experimental API to make reverting easy in case it doesn't pan out somewhere down the road. I'd say let's try it and see how it goes. |
adamziel
changed the title
Good idea! I just updated that name @kevin940726 |
adamziel
changed the title
👍 done! |
|
Would you mind reviewing @getdave and @kevin940726 ? |
kevin940726
left a comment
kevin940726
left a comment
There was a problem hiding this comment.
Looks good! Just some nitpicks/questions here and there.
| } | ||
|
|
||
| interface Options { | ||
| __experimentalEnabled: boolean; |
There was a problem hiding this comment.
Why do we want the option to be experimental? The hook itself is experimental, I don't think we have to experiment with it twice (inception noises) 😆 ?
| [ kind, name, queryAsString ] | ||
| ( query ) => { | ||
| if ( ! options.__experimentalEnabled ) { | ||
| return {}; |
There was a problem hiding this comment.
Is it expected to return an empty object rather than null here? It seems weird to me 🤔 , I'd imagine that it at least returns an empty array than an object.
There was a problem hiding this comment.
If this was null, then the destructuring would error. Returning an empty object makes the data undefined in line 86. Perhaps this could return an object like { data: [] }, but I don't think it matters that much – it's just a placeholder value for when the hook is disabled.
There was a problem hiding this comment.
Actually, reconsidering it – if you had to ask that question then it reads confusing. I turned it into
if ( ! options.enabled ) {
return {
data: [],
};
}If only to make it more readable.
adamziel
force-pushed
the
add/conditional-execution-to-use-entity-records
branch
2 times, most recently
from
7ab6409 to
28716cb
Compare
Co-authored-by: Greg Ziółkowski <[email protected]>
adamziel
force-pushed
the
add/conditional-execution-to-use-entity-records
branch
from
28716cb to
2cf3181
Compare
| ( query ) => { | ||
| if ( ! options.enabled ) { | ||
| return { | ||
| data: [], |
There was a problem hiding this comment.
This probably should const EMPTY_ARRAY, so a new reference isn't returned each time.
There was a problem hiding this comment.
Good spot! Let's tackle it as a part of making this a public API
Description
A part of #38135
Following up on an excellent discussion from #38827, this PR takes a closer look at the conditional execution of
getEntityRecordsto make it a viable tool in cases such as this one:With the new option proposed in this PR, the snippet above would look like this instead:
As demonstrated in the update made to
use-navigation-entities.js.Test plan
Confirm the tests are green. The relevant navigation block use-cases are covered by e2e so there should be no additional testing required.
Alternatives
@kevin940726 suggested opening
useQuerySelectAPI as an alternative:It has a few upsides, such as enabling the use of conditional, loops, infinite loading, etc.
It could make a great follow-up discussion – I still think this PR has merit on its own.
cc @gziolo @getdave @kevin940726 @noisysocks