useEventListener logic is broken

[mendrik]

usually in the first cycle the element ref is null and even though a ref element is present it will attach a listener to the window object. If ref is given it should wait until ref.current is available before attaching any events

[juliencrn]

Hi @mendrik, good observation, but I think in this situation, we are done.

Like you can see in this image, the ref is set before the effect was triggered, so, in other word, when we are in the useEffect callback, the ref must be OK.

You can make another test, create a component like this:

function Component() {
  const ref = useRef<null | HTMLDivElement>(null)

  useEffect(() => {
    console.log(ref)
  }, [])

  return <div ref={ref}>Example</div>
}

You can see in your console that in the only one console.log output, the ref is well-defined.

However, I can miss something, if you have an use-case with an issue, I'm open to re-check, any feedback is always welcome!

[mendrik]

hmm maybe I debugged something wrong then. I thought it had attached the listener to window object. closing

[schontz]

I think making the target an optional parameter is a recipe for errors. Consider the following:

const LogClicks = forwardRef((props, ref) => {
  useEventListener('click', (e) => console.log(e.target), ref)
  return <div ref={ref}>{children}</div>
})

const UsesComponent = () => {
  return (<div><LogClicks>Hello</LogClicks></div>)
}

Since no ref is passed, it will remain null, and the click handler will be attached to the window. This is but one example of "what if the ref is not set?"

I believe it would be wiser to make the target required and never fallback to window, as it is easy to provide and prevents any confusion. This would also make the signature similar to the actual DOM method:

// If there is no target, no op
useEventListener(target, 'eventName', callbackFn)

// Looks about like:
target.addEventListener('eventName', callbackFn)