Optimize useSelect calls (batch 1) #23255
Conversation
ZebulanStanphill
added
[Type] Performance
ZebulanStanphill
requested review from
ellatrix,
etoledom,
SergioEstevao and
youknowriad
as code owners
|
Size Change: -7 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
packages/block-editor/src/components/block-list/block-selection-button.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-list/insertion-point.js
Outdated
Show resolved
Hide resolved
| const isEmptyContent = isEmptyBlockList || isOnlyUnmodifiedDefault; | ||
| const isTemplatePickerAvailable = __experimentalUsePageTemplatePickerAvailable(); | ||
|
|
||
| return isEmptyContent && isTemplatePickerAvailable; |
There was a problem hiding this comment.
I think this regresses in terms of performance. Returning a boolean from useSelect is way more performant in terms of shallow comparison.
There was a problem hiding this comment.
Alright, I've changed it. Is it good now?
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
from
04c04d5 to
aa5710f
Compare
| isUnmodifiedDefaultBlock, | ||
| __experimentalUsePageTemplatePickerAvailable, |
There was a problem hiding this comment.
Thinking about this more, since these are external functions, and they'll always be the same functions, I can remove them from the dependency array. Am I correct?
There was a problem hiding this comment.
__experimentalUsePageTemplatePickerAvailable
This reads like a hook, I don't think we should be using it "inside" useSelect, we may break the hooks rules that way.
There was a problem hiding this comment.
Thanks again for catching that. I've moved that out of the useSelect call and changed the dependency array to include isTemplatePickerAvailable. I've also removed isUnmodifiedDefaultBlock from the dependency array. That should be fine, right?
|
Interesting, I think we fixed all occurrences of Great catch, thanks for keeping an eye 👏 We really need this lint rule from React to error at least when there are no deps defined for hooks unless there is no way to define custom hooks there... then our own lint rule? |
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
from
aa5710f to
8d35521
Compare
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
2 times, most recently
from
2a80d1d to
0d461e1
Compare
|
Anything blocking here? |
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
from
0d461e1 to
7c38334
Compare
|
@youknowriad The only blocking things are unrelated end-to-end test failures. 😛 I just rebased the branch and pushed the changes. When the tests go green, I'll merge. |
|
There are so many end 2 end tests failing that I don't think they are unrelated to this PR. Maybe it's better to split it into smaller PRs |
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
from
7c38334 to
8615cd5
Compare
ZebulanStanphill
force-pushed
the
fix/unoptimized-use-select-calls
branch
from
8615cd5 to
57e8b14
Compare
|
Alright, I shrunk down the PR, and it's passing now, so I'm going to go ahead and merge it. |
Description
It turns out that there are a lot of
useSelectcalls missing a dependency array to memoize their callbacks. I initially planned to create a single PR to fix all of these; but it turns out that there are so many unoptimizeduseSelectcalls that it's better to split it up into several batches. This is the first batch.I also took this opportunity to remove some unneeded destructuring, which should help reduce the number of objects being created.
In cases where I found a
useSelectcall that had an incorrect dependency array (which could lead to buggy behavior), I've created a separate PR for it, e.g. #23249.