-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Components: refactor CustomGradientBar
to pass exhaustive-deps
#41463
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
Conversation
Size Change: +9 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do wonder whether we would've structured these functions differently (i.e. easier to machine-verify correctness) had the exhaustive-deps rule already been in place, I think this is definitely an improvement!
9578c7c
to
dc84cd4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just flagging the bug fix in #41492 — it may be worth to give priority to that PR, and make any required adjustments in this PR once it gets merged.
Thanks for highlighting. I'll hold off on merging this and review things again after |
Just flagging that #41492 has been merged — let's rebase and give this PR a final round of reviews :) |
957d031
to
8ca2eb2
Compare
Rebased and not seeing any new/unexpected behavior. Let me know what you think @ciampo/@mirka! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚀
…entPickerDomRef` in `useMemo` dependencies
…entListeners` function
16c57bc
to
86b149b
Compare
What?
Updates the
CustomGradientBar
component to satisfy theexhaustive-deps
eslint rule (specifically by updating theControlPoints
subcomponent)Why?
Part of the effort in #41166 to apply
exhuastive-deps
to the Components packageHow?
GradientPickerDropdown
Update the
popoverProps
useMemo
dependency array to includegradientPickerDomRef
instead ofgradientPickerDomRef.current
.useEffect
to clean event listeners on unmountThe dependency array is currently empty, which works to trigger the cleanup only on unmount. Unfortunately
exhaustive-deps
expects to see the function being called in the dependency array, which would cause it to trigger on every render.To avoid that, if we wrap
cleanEventListeners
in its ownuseCallback
the listeners aren't able to be cleaned properly. Using a ref of the function instead allows us to exclude it from the dependency array so ouruseEffect
can provide the intended cleanup, without disrupting the actual user interactions with the component.Took me a little bit to arrive at this solution, and I'm very open to feedback if there are other/better approaches :)
Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/custom-gradient-bar