Preventing duplicate event listener registrations

[seungdeng17]

I encountered an issue where event listeners were being registered multiple times.

I discovered that when calling scrollTo-related functions in the animate-scroll module, event listeners were being registered through the subscribe method.

There was no logic to remove registered events or check for duplicates, resulting in event listeners being registered multiple times.

I have thought of the simplest solution: preventing duplicate registrations by using the name of the listener function passed as a parameter to the addPassiveEventListener function as a key.

It is important to note that you should pass a named function with a unique name as the listener parameter, rather than an anonymous function.

Fix for #451

---

• As-is

• To-be

[fisshy]

Thanks for your PR!

Why do we need to enforce a named function?

if (!listener.name) throw new Error('Listener must be a named function.');

Using this will break functionality for current users, we should probably just console.warn instead?

[seungdeng17]

This is because we prevent duplicate event listener registration by using the name of the named function as a key.

if (!listener.name) throw new Error('Listener must be a named function.');

This code isn't necessary, but was added as a defensive measure. It doesn't seem to cause any issues in operation. If you are concerned, replacing it with the console.warn you suggested is a good idea.

I will add a commit 😀

[fisshy]

Maybe we should also fallback on eventName if listener.name is empty?

[seungdeng17]

Using the eventName as a key seems like a reasonable alternative when an anonymous function is passed.

What do you think about this approach?

export const addPassiveEventListener = (target, eventName, listener) => {
  let listenerName = listener.name;
  if (!listenerName) {
    listenerName = eventName + new Date().getTime();
    console.warn('Listener must be a named function.');
  }
  ...
}

Since eventNames are usually not unique, I considered appending the return value from the getTime method.

By doing this, the same behavior as before is assured even if this PR is merged.

[fisshy]

Do we really need to append more events with getTime?

Couldn't we just replace the old event with the new, given same name?

Also removeEventListener wouldnt work with getTime

[seungdeng17]

I've written it as defensively as possible.

If it's sufficient to replace an existing event with a new one, it's fine to just use eventName.

[fisshy]

Looks good to me!

[nsunga]

@seungdeng17 hello 👋

Sorry to bother - this is also my first time digging into the source code for this package

But ever since upgrading to this commit, every time scrollTo is called in scroller.js, I keep getting the console.warn noise
console.warn('Listener must be a named function.');

• scroller.js line for reference: https://github.com/fisshy/react-scroll/blob/master/modules/mixins/scroller.js#L28

Do you have any pointers to stop these console warns? I'll start digging into the call stack but I figured I'd ask here in this change if you know the correct way around this