Components: refactorDraggable to pass exhaustive-deps#41499
Merged
Conversation
44 tasks
|
Size Change: 0 B Total Size: 1.24 MB ℹ️ View Unchanged
|
fluiddot
approved these changes
Jun 2, 2022
Contributor
fluiddot
left a comment
There was a problem hiding this comment.
LGTM 🎊 !
I tested the changes in both Android and iOS platforms and confirm that the change doesn't modify the draggable logic.
| const providerValue = useMemo( () => { | ||
| return { panGestureRef, isDragging, isPanActive, draggingId }; | ||
| }, [] ); | ||
| }, [ isDragging, isPanActive, draggingId ] ); |
Contributor
There was a problem hiding this comment.
isDragging, isPanActive and draggingId are shared value object types created using the hook useSharedValue provided by the React Native's Reanimated library.
gutenberg/packages/components/src/draggable/index.native.js
Lines 48 to 50 in 3512a38
The shared values are similar to the React references in the sense that their value doesn't update after they are initialized. Therefore, it's not really necessary to add them as dependencies of useMemo because the value won't change. However, I understand that since this type is not recognized by the exhuastive-deps ESLint rule, we could add them in order to comply with it 👍 .
Contributor
There was a problem hiding this comment.
Thank you both!
Maybe we could leave an inline comment in code, so that the next time folks look at this line of code they have more context about isDragging, isPanActive and draggingId ?
Contributor
There was a problem hiding this comment.
Yep good point, we could leave a comment when using the shared value objects in hooks to outline that they are not required to be added in the dependencies array 👍 .
3512a38 to
8567280
Compare
jhnstn
pushed a commit
to jhnstn/gutenberg
that referenced
this pull request
Jun 15, 2022
…41499) * Draggable: update `useEffect` dependency array * Draggable: update changelog (merge conflict) * move changelog update to the correct section
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
3 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
Draggablecomponent to satisfy theexhaustive-depseslint rule (specifically by updating theControlPointssubcomponent)Why?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
Add the three values returned as an object by
useMemoto the dependency array so they're current.Previously this was an empty array so it would only occur on the initial render. I'm not 100% sure if was intentional or not. My testing of this change didn't raise any red flags, but I'd love some feedback from someone more familiar with React native (cc @geriux)
Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/draggable