Skip to content

Components: refactor ResizableBox to pass exhaustive-deps #44370

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 28, 2022

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Sep 22, 2022

What?

Updates the ResizeBox component to satisfy the exhaustive-deps eslint rule

Why?

Part of the effort in #41166 to apply exhuastive-deps to the Components package

How?

debounceUnsetMoveXY and onResize were missing dependencies of the the useResizeLabel hook. Adding onResize (a passed in prop) will technically cause the effect to fire every time the hook is called, but that was already happening due to the width and height deps (unless we wanted to memoize those).

Now that it was a dependency, debounceUnsetMoveXY needed its own useCallback. This would, in turn, have needed unsetMoveXY as a dependency, leading to wrapping that method in yet another useCallback.

Instead, I moved unsetMoveXY into the single newly created useCallback. Now that we have only one function handling all of the unsetMoveXY it makes more sense to call that simply unsetMoveXY instead of debounceUnsetMoveXY.

The end result is a single unsetMoveXY method containing its own debounce logic, all wrapped in a single useCallback for the benefit of the useEffect's dependency array.

The only two dependencies for the new useCallback are fadeTimeout and isAxisControlled. While we could technically memoize isAxisControlled to prevent it triggering the useCallback on every render, we'll still have fadeTimeout which is a prop that likely won't be memoized before it's passed in, so I didn't see a benefit to an extra useMemo call.

Testing Instructions

  1. From your local Gutenberg directory, run npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/resizable-box/
  2. Confirm that the linter returns no errors
  3. Run Storybook locally, confirm the components stories and/or docs still work as expected

@chad1008 chad1008 requested review from ciampo and mirka September 22, 2022 13:24
@chad1008 chad1008 self-assigned this Sep 22, 2022
@chad1008 chad1008 added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components [Feature] Component System WordPress component system labels Sep 22, 2022
@chad1008 chad1008 marked this pull request as ready for review September 22, 2022 13:26
@chad1008 chad1008 requested a review from ajitbohra as a code owner September 22, 2022 13:26
@ciampo ciampo changed the title Components: refactor ResizeBox to pass exhaustive-deps Components: refactor ResizableBox to pass exhaustive-deps Sep 23, 2022
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes LGTM 🚀

When a "dependency" is only used inside a hook, it's totally reasonable to move the code for that dependency directly inside the hook, rather than adding it to the dependencies list

I just left a couple of comments, but don't consider them blocking — they're more of a suggestion and/or conversation than a strong change request.

@chad1008 chad1008 force-pushed the refactor/ResizableBox-exhuastive-deps branch from b0ba6e4 to 771341c Compare September 26, 2022 19:21
@ciampo
Copy link
Contributor

ciampo commented Sep 26, 2022

Great! Feel free to merge once all CI checks are ✅

@chad1008 chad1008 merged commit cc18f73 into trunk Sep 28, 2022
@chad1008 chad1008 deleted the refactor/ResizableBox-exhuastive-deps branch September 28, 2022 16:54
@github-actions github-actions bot added this to the Gutenberg 14.3 milestone Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants