Components: refactor ToolsPanel to pass exhaustive-deps
#45028
Conversation
e95a41c to
9d0faaf
Compare
There was a problem hiding this comment.
Nice work @chad1008, appreciate the detailed notes and context 👍
This tests well for me:
✅ No linting errors with exhaustive-deps
✅ Unit tests are passing
✅ Storybook examples continue to function
✅ No differences in behaviour in Block and Site Editors
If we really want the isResetting value to trigger a re-render to update this memo it should go into state
I believe the isResetting value passed via context to the tools panel items was so that individual items could prevent calling their onSelect/onDeselect callbacks when the overall panel was executing a "reset all". When the "reset all" is being done, the menu items state is updated which would force the useMemo update. So as you noted, we should be safe to simply remove the dependency.
9d0faaf to
59ddeb2
Compare
|
Thanks @aaronrobertshaw! Much appreciated! @ciampo how are you feeling about this one? I particularly wanted to double check your thoughts on the added dependencies in
If that sounds logical to you, I'll wait for the checks to pass and get this merged 🙂 |
|
Changes look good to me — although I'm not sure I understand this part of your reasoning:
I may be wrong, but I don't think that destructuring values from those contexts automatically means that the hooks will run on every render. Apart from that, I also gave a quick at how the component behaves in the editor and I couldn't spot any unexpected behavior. Plus, this component was mostly written by @aaronrobertshaw (including lots of unit tests!), and therefore seeing that he already approved this PR is great 🚀 |
What?
Updates the
ToolsPanelcomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
In
tools-panel/hook.ts, we can safely removegenerateMenuItemsfrom dep arrays. React can't tell if an outer scope value like this changes, so it would never trigger a re-render, nor would it cause the effect to actually fire.We can also safely remove
isResetting.currentfrom the dependency arrays. Including aref.currentvalue in a dependency array isn't recommended in general.In the relevant
useCallback, the value is updated, but never actually read... so even if it had changed, that wouldn't impact the callback's behavior.In the relevant
useMemo, the value is read, so changes matter, but including the dependency in the array gives a false sense of security. If that were the only dep to change on a given render, React wouldn't notice, and the memoized value wouldn't update. If we really want theisResettingvalue to trigger a re-render to update this memo it should go into state, or a reducer. Because the component has functioned properly as it is now thus far, I think we should hold off on introducing new state (which will be sure to trigger additional renders and potentially other behavior changes) and simply remove the dependency. cc @aaronrobertshaw in case you have any insights, as you did some work with theisResettingvalue here most recently.Finally, in
tools-panel-item/hook.ts, a number of missing dependencies were added. In each case the added dep(s) are values being destructured from one of two contexts, so they'd cause their hooks to fire on every render. However, all of these hooks already have dependencies that are being destructured in this way, or derived from similar variables... so these new dependencies shouldn't change the existing behavior of anything. (Please feel free to correct me if my logic doesn't sound right here!)Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/tools-panel