Prevent unnecessary event listeners

[corymharper]

There are indications that the useEffect on line 52 could be generating extra click event listeners every time the menu open or closes. This should be researched to determine if it is the case, and a PR opened to erect the issue.

[corymharper]

This does not appear to be an issue because of this behavior of Event Listeners:

Multiple identical event listeners
If multiple identical EventListeners are registered on the same EventTarget with the same parameters, the duplicate instances are discarded. They do not cause the EventListener to be called twice, and they do not need to be removed manually with the removeEventListener() method. Note however that when using an anonymous function as the handler, such listeners will NOT be identical since anonymous functions are not identical even if defined using the SAME unchanging source-code simply called repeatedly, even if in a loop. However, repeatedly defining the same named function in such cases can be more problematic. (see Memory issues below.)

The function being called is a named function, so they have the same signature and the subsequent attempts to create an event listener are discarded.

[WesCossick]

However, repeatedly defining the same named function in such cases can be more problematic. (see Memory issues below.)

handleEveryClick() is defined every time its containing useEffect() Hook is called, which sounds like it's a problem.

[corymharper]

Very likely. From the same source as the above comment:

Actually, regarding memory consumption, the lack of keeping a function reference is not the real issue; rather it is the lack of keeping a STATIC function reference. In both problem-cases below, a function reference is kept, but since it is redefined on each iteration, it is not static. In the third case, the reference to the anonymous function is being reassigned with each iteration. In the fourth case, the entire function definition is unchanging, but it is still being repeatedly defined as if new (unless it was [[promoted]] by the compiler) and so is not static. Therefore, though appearing to be simply [[Multiple identical event listeners]], in both cases each iteration will instead create a new listener with its own unique reference to the handler function. However, since the function definition itself does not change, the SAME function may still be called for every duplicate listener (especially if the code gets optimized.)

This could easily be solved by simply moving the function definition into the outer scope, or even to a separate file for import.

[WesCossick]

Okay, go ahead and find an optimal way to solve the issue in a new PR.

[corymharper]

In trying a number of approaches to solving this issue I have encountered a few problems, here is what I have tried and the issues I have encountered:

My first approach to creating a completely stable signature for the function was to create it in a separate file as a utility function and export it. This meant a loss of context for many values the function used, but they could be passed along to it as arguments. That approach looked something like:

handleEveryClick(event: MouseEvent, isOpen: boolean, setIsOpen: () => void) {
...
}

export default handleEveryClick;

The issue I encountered with this approach is that there is no way to pass along the arguments it needs (That I am currently aware of) when it is called by the event listener, except to create a callback function that calls it with those arguments, like so:

document.addEventListener('click', (event) => handleEveryClick(event, isOpen, setIsOpen));

But this just recreates the same problem we are trying to solve by importing the function from another file, since the callback function is now either anonymous and never has a stable signature, or a named function that gets redefined every time the useEffect is fired.

My other approach was simply moving handleEveryClick out of the useEffect into the outer scope, so it still had the context of those values. However, that poses a similar issue in that every time the hook is called the function gets redefined, so possibly on every render of the component implementing the hook. I thought I could mitigate this my memoizing the callback, so I tried an implementing useCallback, but I have not been successful in passing our tests with the memoized function.

[WesCossick]

Would .bind() come in handy? Maybe you could bind isOpen and setIsOpen instead of passing them as arguments. I don't use .bind() very often, though, so I could be mistaken.

[WesCossick]

Maybe first we need to come up with a way to verify whether or not our concerns are valid. Have you been able to show definitively that we're adding duplicate event listeners?

[corymharper]

I was actually pretty much able to verify the opposite. While addEventListener is being called multiple times, the browser is discarding the identical event listeners. Which means that the function is maintaining it's signature at least regardless of the menu being closed/opened. I don't know if that would remain true if other unrelated features of a component caused rerenders.

[WesCossick]

Okay, then I assume we can safely close this issue then?

[corymharper]

I think that would be fine, I don't see any evidence of performance issues as things stand.