Skip to content
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

Components: refactor useFlex to pass exhaustive-deps #45528

Merged
merged 3 commits into from
Nov 9, 2022

Conversation

chad1008
Copy link
Contributor

@chad1008 chad1008 commented Nov 3, 2022

What?

Updates the useFlex hook to pass the exhaustive-deps eslint rule.

Why?

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

How?

Removing isReverse from the dependency array, and then because it isn't used anywhere else in the hook, removing the variable entirely.

When this dependency was added, isReverse was used in several places for the creation of the classes object, but the hook has been refactored since then, removing any use of the isReverse variable in the hook.

Based on that, I think this dependency and variable just got left in - removing them doesn't cause any unexpected changes that I can see in Storybook.

cc @sarayourfriend: do you have any thoughts on drawbacks to this approach? If we do, in fact, still need to watch this variable and update the useMemo value when it changes, we'll need to either disable the exhuastive-deps check, or find another approach. Curious what you think 🙂

Testing Instructions

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

@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 Nov 3, 2022
@chad1008 chad1008 requested review from mirka and ciampo November 3, 2022 18:46
@chad1008 chad1008 self-assigned this Nov 3, 2022
@codesandbox
Copy link

codesandbox bot commented Nov 3, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

Copy link
Contributor

@sarayourfriend sarayourfriend left a comment

Choose a reason for hiding this comment

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

This seems like it should be fine. Because direction is already a dependency the styles will get recalculated anyway so the dependency on isReverse wasn't actually doing anything (as far as I can tell), not even in a hacky, sneaky way.

@chad1008
Copy link
Contributor Author

chad1008 commented Nov 8, 2022

thanks @sarayourfriend!

@chad1008 chad1008 merged commit 2d63b15 into trunk Nov 9, 2022
@chad1008 chad1008 deleted the refactor/Flex-exhaustive-deps branch November 9, 2022 00:33
@github-actions github-actions bot added this to the Gutenberg 14.6 milestone Nov 9, 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.

2 participants