Clean up lint warnings#3612
Conversation
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Can you make a ticket to fix the dep array ones? otherwise looks good
we could also start failing the lint job on warnings to help us in the future, but that can be in another PR
ktabors
left a comment
ktabors
left a comment
There was a problem hiding this comment.
I thought the plan wasn't to ignore the warnings?
| if (isDisabled) { | ||
| triggerHoverEnd({currentTarget: state.target}, state.pointerType); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
It seemed like there are a lot of skip/ignores and I randomly looked at this one. Why didn't we add the other dependencies like state.target and state.pointerType?
There was a problem hiding this comment.
This will get handled in a follow-up ticket to review where we ignore this rule
| useEffect(() => { | ||
| return () => { | ||
| if (!allowTextSelectionOnPress) { | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
And too followup on me trying to understand the ignores. I thought react-hooks/exhaustive-deps is about dependencies and useEffect/useLayoutEffect issues? This line looks like a function call. Looking at the lint warning it mentioned dependencies, but seemed to indicate the code should be wrapped in a useEffect/useLayoutEffect or something equivalent. Is this the correct label for the ignore or can we solve it without ignoring it?
There was a problem hiding this comment.
The warning was:
The ref value 'ref.current' will likely have changed by the time this effect cleanup function runs. If this ref points to a node rendered by React, copy 'ref.current' to a variable inside the effect, and use that variable in the cleanup function.
There was a problem hiding this comment.
+1 on @snowystinger's suggestion to eventually fix those dep arrays, and so that this also doesn't become a pattern.
|
Build successful! 🎉 |
5 participants
Picking up from #1199
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: