fix: add ref handling for useEventListener #2939
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| target.removeEventListener(type, eventListener, options); | ||
| }; | ||
| }, [type, target, options]); | ||
| }, [type, targetValue, options]); |
There was a problem hiding this comment.
If we are passing the RefObject, wont this not trigger re-renders when it changes?
Although if this fixes the underlying issues, I trust this is the right change.
There was a problem hiding this comment.
mutating the ref.current value won't trigger re-renders. But the ref object itself does. I'm basing this because you would normally get a warning when using ref.current in useEffect
Mutable values like 'ref.current' aren't valid dependencies because mutating them doesn't re-render the component. (react-hooks/exhaustive-deps)
But I've searched online and see that a common solution is to use useCallback, but I think that would require a lot of code modification. Do you think I should look more in this direction?
update: we could just use const [element, setElement] = useState(cellRef); and pass in element to the hooks. This is way simpler and has the same effect & logic imo.
Let me revert if I don't find something better.
There was a problem hiding this comment.
if this fixes it, i am totally ok with it! i think this was a bug that was already there, but I added ReactCompiler which memoizes a lot such that subtle bugs like this could crop up.
also, you might have fixed this #2940 (thank you!)
There was a problem hiding this comment.
looking at this implementation, looks like you are right: https://usehooks-ts.com/react-hook/use-event-listener
and we should be passing in the Ref
|
going to merge this! i can use this for another bugfix, thanks @Light2Dark! |
2 participants
📝 Summary
useEventListener wasn't working properly when you passed in ref.current. For example, the
ArrowUp&ArrowDownkeys to move between cells. This is because changes to ref.current will not trigger a re-render.🔍 Description of Changes
useEventListener lets you pass in a ref object instead of ref.current and ensure it re-renders correctly when ref.current is no longer null.
Stumbled upon this as I was trying to add new eventListeners. Shoutout to Claude for helping me understand & write 😅.
📋 Checklist
📜 Reviewers
@akshayka OR @mscolnick