Components: refactor Navigation to pass exhaustive-deps
#41612
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
44 tasks
chad1008
changed the title
Navigation to pass exhaustive-depsNavigation to pass exhaustive-deps
chad1008
added
[Type] Code Quality
ciampo
reviewed
Jun 10, 2022
There was a problem hiding this comment.
Thank you for working on this, @chad1008 !
I tested this in Storybook, and it seems to break the "controlled" behavior of the component:
- Open the "Controlled State" example
- Click on either the "Open the Nested Sub-Menu menu" or the "Navigate to Child 2 item" buttons
- Nothing happens (expected: the component navigates programmatically to another menu)
Personally I try to avoid wrapping variables in useRef, as it makes the code harder and harder to follow — although I agree that, prior to this PR, this component was already written in a way that I found a bit convoluted (including a few variables around the "active" menu as a prop, internal state, and context value).
The exhaustive-deps ESLint warnings are usually a sign that the current code patterns are not a good fit for the specific use case, and that the code could be refactored in a way that doesn't violate the exhaustive-deps ESLint rule without needing workarounds that feel "hacky".
|
I actually opened #41668 to improve the unit tests for |
|
#41668 got merged, let's rebase this PR to see if the additional unit tests can help us to catch any potential regression 🤞 Update: they do! |
chad1008
force-pushed
the
enhance/Navigation-exhaustive-deps
branch
from
682fcd2
to
d2c2893
Compare
chad1008
force-pushed
the
enhance/Navigation-exhaustive-deps
branch
from
d2c2893
to
a1ac5de
Compare
Good catch. I see what I missed here: the This new fix addresses that, so the Controlled state works again, and I've confirmed the effect is firing the expected number of times. I also went ahead and combined these two values into a single ref. I felt like that made it more readable and clear what the ref was for, but I'm not sure how you feel about that pattern. Happy to split them up if that's preferred!
Thanks for that insight! I'll admit when I first encountered wrapping variables in I looked into a more complete refactor to resolve these issues another way but didn't get too far, so the current fix still employs |
ciampo
reviewed
Jun 29, 2022
There was a problem hiding this comment.
I can confirm that the component seems to work well after the latest changes, although I personally think that the current fix doesn't add much value to the component, while making the code more complex.
At this point, also given the fact that we'd like to move away from this component in the future, I'd prefer adding a comment to ignore ESLint's warning for that line, an an associated comment explaining why the ignore rule is in place (and maybe a link to this PR too).
| @@ -50,9 +55,15 @@ export default function Navigation( { | |||
| } | |||
| }, [] ); | |||
|
|
|||
| // Used to prevent excessive useEffect fires when navigation is being controlled by parent component | |||
| const controlledMenuUpdate = useRef( { setActiveMenu, menu } ); | |||
| useLayoutEffect( () => { | |||
There was a problem hiding this comment.
Any particular reason why you opted for useLayoutEffect instead of useEffect ?
There was a problem hiding this comment.
Just to make sure it's always up to date.. If I'd used a useEffect here, and then some other future change tried to use this hook from a useLayoutEffect, that would be a stale reference because layoutEffects fire first.
Updating the ref with useLayoutEffect here should ensure it's always up to date, regardless of what kind of effect tries to call it later.
Sounds good! I'll update the PR shortly! |
chad1008
force-pushed
the
enhance/Navigation-exhaustive-deps
branch
from
a1ac5de
to
c8bf77b
Compare
stokesman
reviewed
Jul 8, 2022
…seEffect` handling menu `activeMenu` updates
Co-authored-by: Mitchell Austin <mr.fye@oneandthesame.net>
ciampo
force-pushed
the
enhance/Navigation-exhaustive-deps
branch
from
1a2d0a1
to
df2b594
Compare
|
Thanks for all the helpful feedback (and for that rebase!) @ciampo! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Updates the
Navigationcomponent tosatisfyignore theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
Update: Because we ultimately prefer
NavigatortoNavigation, and fixing this component looks like it'd take either a heavier refactor or excessive reliance onuseRef, we're option to disable the rule in this case rather than refactor the component.Missing dependency:menuThe component has an effect that checks to see if theactiveMenuprop has been changed, which would currently only happen if it came from the parent component. The effect does this by checking theactiveMenuprop against the currentmenustate. If they don't match,menuis updated toactiveMenu. This effectively allows the parent component to control what menu or submenu is being displayed.We can't addmenuas an effect dependency because ifmenuwas just changed (triggering a re-render and our effect) we know it no longer matchesactiveMenu. This means the effect will updatemenuback to once again matchactiveMenu.To fix this, we can update a ref of the currentmenustate value, and have the effect check against that instead. We no longer needmenuin the dep array forexhaustive-deps, so we won't be firing the effect on every submenu navigation that updates themenustate.To keepmenuandmenuRefin state, we can use anotheruseEffectcall. The dependency onmenuin this case will limit the ref updates for us.Missing dependency:setActiveMenuAddingsetActivemenumenu as a dep here is no good. We'd need to wrap it in auseCallbackand then its own dependencies would update too often, redeclaring the function and triggering the effect on all menu navigations. Instead, if we use a Ref of the function we eliminate the need for a dep, while still giving the effect access to the functionality.Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/navigation/index.js