Components: refactorFocalPointPicker to pass exhaustive-deps
#41520
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
chad1008
commented
Jun 2, 2022
packages/components/src/focal-point-picker/tooltip/index.native.js
Outdated
Show resolved
Hide resolved
chad1008
commented
Jun 2, 2022
packages/components/src/focal-point-picker/tooltip/index.native.js
Outdated
Show resolved
Hide resolved
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
chad1008
added
[Type] Code Quality
44 tasks
Hey there! I was AFK for a few days, sorry for the delay. I'll check it out 👍 |
geriux
reviewed
Jun 10, 2022
| @@ -120,7 +120,7 @@ function FocalPointPicker( props ) { | |||
| setSliderKey( ( prevState ) => prevState + 1 ); | |||
| }, | |||
| } ), | |||
| [ containerSize ] | |||
| [ containerSize, pan, onChange, shouldEnableBottomSheetScroll ] | |||
There was a problem hiding this comment.
It looks like onChange is changing on every change that's happening on the Focal point picker when updating the sliders and during the touch events, making the panResponder to be created constantly.
This prop is coming from here. So I think we should add a useCallback there (for setPosition) to prevent this from happening.
There was a problem hiding this comment.
Actually scratch that. There's a typo in this commit that was preventing the linter from detecting further issues. Stand by! 😬
There was a problem hiding this comment.
Okay, fixed again in 86ba326.
One odd wrinkle is that exhaustive-deps doesn't seem to mind the empty dep array, when I'd have expected it to want us to add setPosition as a dependency. If we do add it, we get a warning from the linter (unsurprisingly) that it's changing on every render.
I'm thinking it might be that the linter can detect that setPosition just calls setDraftFocalPoint, which in turn comes from useState and will therefor never change. My understanding is that ESLint is smart enough not to require state-setters as deps, but I didn't realize it could parse through an additional layer of abstraction like this (assuming I'm understanding things correctly here).
There was a problem hiding this comment.
It looks like
onChangeis changing on every change that's happening on the Focal point picker when updating the sliders and during the touch events, making thepanResponderto be created constantly.
@geriux good catch! How did you come across the changing onChange value? Did you notice a performance issue or observe it in a different manner? Primarily curious to improve my ability to identify this type of thing in the future.
dcalhoun
reviewed
Jun 17, 2022
There was a problem hiding this comment.
Thank you for taking the time to dive into this, @chad1008! Also, I appreciate the well-written PR description — great context. 🙇🏻
I tested the proposed changes on an iPhone SE (iOS 15.6) and Samsung Galaxy S20 (Android 12). Aside from the issue I called out for the memoized onChange, these changes appear to function as expected. 🎉
packages/components/src/mobile/focal-point-settings-panel/index.native.js
Outdated
Show resolved
Hide resolved
| @@ -120,7 +120,7 @@ function FocalPointPicker( props ) { | |||
| setSliderKey( ( prevState ) => prevState + 1 ); | |||
| }, | |||
| } ), | |||
| [ containerSize ] | |||
| [ containerSize, pan, onChange, shouldEnableBottomSheetScroll ] | |||
There was a problem hiding this comment.
It looks like
onChangeis changing on every change that's happening on the Focal point picker when updating the sliders and during the touch events, making thepanResponderto be created constantly.
@geriux good catch! How did you come across the changing onChange value? Did you notice a performance issue or observe it in a different manner? Primarily curious to improve my ability to identify this type of thing in the future.
chad1008
force-pushed
the
enhance/FocalPointPicker-exhuastive-deps
branch
from
86ba326
to
5bd332d
Compare
|
Well spotted @dcalhoun! Thanks for the suggested fix! |
dcalhoun
approved these changes
Jun 29, 2022
…ponder`s `useEffect` dep array
… `startAnimation` in a `useCallback` with relevant deps.
… `useCallback` to prevent excessive re-renders
…x.native.js Co-authored-by: David Calhoun <[email protected]>
chad1008
force-pushed
the
enhance/FocalPointPicker-exhuastive-deps
branch
from
0b34818
to
3ebaf4a
Compare
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
FocalPointPickercomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
Drag handle
useEffectAdd
panto theuseEffectdep arraypanResponder
locationPageOffsetXandlocationPageOffsetYwere both declared asuseRef().current, which is effectivelyundefinedand not an actual ref object.This meant that later on, when mutating these variables, we weren't actually updating a ref, but assigning values that could simply be lost on future renders.
Instead, we now declare both values as actual refs (
useRef()) and then update the.currentvalue later on, preserving the assigned value between renders.We then added
pan,onChange, andshouldEnableBottomSheetScrollto theuseEffectdep array.Tooltip
startAnimationwas expected as a dependency for theuseEffectcall. Doing so also requires wrappingstartAnimationin its ownuseCallback, dependent onanimationValue.Another option on the Tooltip would be to simply move the code executed by
startAnimationdirectly intouseEffect. That function isn't being called anywhere else, so it doesn't really need a separate declaration. This would require addinganimationValuetouseEffect's dep array.I went with the first option, because while more verbose, I felt the named function improved readability, but I'm happy with either path.
Note:
I've done the best I can with the code changes themselves, but these are all unfortunately React native changes, so I'm a bit out of my depth on actually testing them, so I'm crossing my fingers and calling in the cavalry: cc @geriux.
I'm hopeful these changes and any subsequent adjustments work well 🤞 but if they're too messy I may defer further work on this to a React native expert with better tools/dev environment in place.
Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/focal-point-picker