Components: refactorFocalPointPicker to pass exhaustive-deps#41520
Merged
Components: refactorFocalPointPicker to pass exhaustive-deps#41520
FocalPointPicker to pass exhaustive-deps#41520Conversation
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
|
44 tasks
Member
Hey there! I was AFK for a few days, sorry for the delay. I'll check it out 👍 |
geriux
reviewed
Jun 10, 2022
| }, | ||
| } ), | ||
| [ containerSize ] | ||
| [ containerSize, pan, onChange, shouldEnableBottomSheetScroll ] |
Member
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.
Contributor
Author
Contributor
Author
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! 😬
Contributor
Author
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).
Member
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
Member
dcalhoun
left a comment
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
| }, | ||
| } ), | ||
| [ containerSize ] | ||
| [ containerSize, pan, onChange, shouldEnableBottomSheetScroll ] |
Member
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.
86ba326 to
5bd332d
Compare
Contributor
Author
|
Well spotted @dcalhoun! Thanks for the suggested fix! |
Contributor
dcalhoun
approved these changes
Jun 29, 2022
Member
dcalhoun
left a comment
There was a problem hiding this comment.
Aside from a small typo I noted and merge conflict resolution, this LGTM. 🚀 Thanks!
…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 <438664+dcalhoun@users.noreply.github.com>
0b34818 to
3ebaf4a
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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