ToolsPanel: Memoize callbacks and context to prevent unnecessary rerenders #38037
Conversation
aaronrobertshaw
added
[Feature] UI Components
|
Size Change: +15 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work @aaronrobertshaw, this is testing well for me:
✅ Tests pass
✅ All storybook examples test well
✅ Creating a variety of paragraph and group blocks with various Typography and Spacing settings works just as it does on trunk, adding, resetting individual controls, and resetting all 👍
✅ Controls when multiple blocks are select also still work as expected
✅ Confirmed before and after in React Dev Tools that excess renders when hovering between blocks no longer occurs 👍
In the dependency arrays of some of the useCallback calls in tools-panel/hook.ts, I noticed that they're set up to only change if the panelItems change, but those functions do depend on functions / variables outside of them (e.g. isResetting, setMenuItems, resetAll). I think this is fine because if the panelItems change, then those functions also change, so the function references there should never go out of sync (i.e. there shouldn't be any risk that we're calling the wrong resetAll), but just thought I'd mention it just in case.
I can't see any issues with getting this PR in, but I thought I'd ping @ciampo in case he has any feedback on the useCallbacks, too.
LGTM!
|
Thanks for the early review @andrewserong 👍
Yep. I missed the |
|
@andrewserong I've updated the dependency arrays. Let me know if I've missed anything else. In the meantime, I'll open this up for wider review. |
|
Nice one, thanks @aaronrobertshaw — that looks good to me, and I just gave it a quick re-test that it's still working nicely in the editor 👍 |
|
Thanks for following up here :) This is good? Was it necessary to memoize the whole context object, or just its properties? |
To prevent the re-rendering you highlighted in #32392 (comment), all I needed was to memorize the As for the changes other than the mentioned If it's overkill, I'm happy to roll some of the changes back. Whatever is best. |
|
That makes sense to me, thanks Aaron |
There was a problem hiding this comment.
Thank you for the ping!
I think this is fine because if the panelItems change, then those functions also change, so the function references there should never go out of sync (i.e. there shouldn't be any risk that we're calling the wrong resetAll), but just thought I'd mention it just in case.
My preference in general would be to be as exhaustive as possible in listing the dependencies in useCallback / useMemo, as it avoids some nasty bugs in the future.
setMenuItems is a state setter so my understanding is they are guaranteed to be stable
That is my understanding too — I left a few comments to point out the instances in which a state setter was added as a dependency. We may want to remove it, since we established that it's redundant.
Agreed. To be "exhaustive as possible" though would also entail adding all dependencies including those state setters.
They may be redundant as far as React is concerned. I added them to be exhaustive as it likely helps readers of those hooks to quickly establish if all dependencies are present without having to scroll to confirm whether or not a function is in fact a state setter and safe to omit. On that front, as you note, the React docs do state that it is safe to omit them. I'm not sure that means they should be omitted. There are plenty of other locations within the codebase where these are added to dependency arrays. As their inclusion does make the dependency arrays truly exhaustive, do no harm, and potentially make the hooks easier to grok, I'm inclined to keep them as that's why they were added while addressing earlier feedback. I'll resolve the conversations highlighting their uses for now. If you feel strongly as to omitting them being the better practice let me know and we can revisit this. Thanks for taking a look at this one 👍 |
I'm not sure if there's any standards in Gutenberg,, but I don't personally feel strongly about it — we can keep things as they are! |
aaronrobertshaw
deleted the
try/reducing-block-support-tools-panel-rerenders
branch
Raised in #32392 (comment)
Description
This PR adds memoization to the callbacks and context used by the
ToolsPanelandBlockSupportsToolsPanelto prevent unnecessary rerenders.How has this been tested?
ToolsPanelunit tests:npm run test-unit packages/components/src/tools-panel/testToolsPanelstorybook examples still function as expected:npm run storybook:devToolsPanelfunctions as expected for block supports within the block/site editors.ToolsPanelis not excessively rerendering.Testing of rerendering fix
trunkand open block editorToolsPanelrerenderingScreenshots
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).