Discord.js: Change EventEmitter to eventemitter3

[ghost]

Is your feature request related to a problem? Please describe.
The default EventEmitter included with Node.js in the events module is quite inefficient, and the eventemitter3 module is proven to be faster, and is quite lightweight. You can read more about it on its readme. It is already used (optionally) by Eris, and I use it in my own projects in place of native EventEmitters, with noticeable performance improvements.

Describe the solution you'd like
Add eventemitter3 as a dependency, and switch imports of events for EventEmitter to eventemitter3 or have eventemitter3 optional and use events if and only if eventemitter3 is not installed.

[appellation]

Allowing unlimited listeners is mildly unfortunate. Also, lack of support for fatal error events could be an issue.

[SimplyLinn]

Wouldn't that add the listener to the error event, creating the error array, and never, y'know, actually trigger the throw? I better solution would be to check if the error listener array length is equal to one

[ghost]

@NbOpposite if there is only one event it registers it directly, not as an array.

[SimplyLinn]

That seems rather inconsistent imho, but yeah, if that's the case then it'll work. I'd probably futureproof it though by checking if it's an array, if the array has a size of 1.

Are you also sure that it reverts back to a direct register if you, say, deregister a listener, or will it then turn into a single-entry array?

[kyranet]

What you want is to extend EventEmitter3 with the following method:

emit(event, ...args) {
  if (event === 'error' && !Array.isArray(this._events.error)) {
    throw args[0];
  }
  return super.emit(event, ...args);
}

But this is inconsistent and would need extra code just to make both work similarly.

[appellation]

not thrilled at patching functionality using undocumented private apis

[SimplyLinn]

EventEmitter#listenerCount is documented though, we could use that.

Issue being that, EventEmitter#removeAllListeners would clear away our throw listener. So we have to take that into consideration as one difference when we evaluate potential breaking changes and their impacts if we switch

[ghost]

Forgive my ignorance, but why is it important that we throw any unhandled error events?

[DevYukine]

Because its best practise to have a listener on the error event and its a node.js standart to emit errors on that event for Eventlisteners see node.js docs.

[iCrawl]

Any further input on this @Shinobu1337

[ghost]

It is my opinion that the performance benefits outweigh the "con" of not throwing error events. Additionally, I do not think this would be a very difficult transition to make. If a few people have problems because they are not listening to an error event, they could get help in the Discord.js support server, and perhaps a small sentence could be added to the docs if necessary. Eris already implements this, so it is not unprecedented in any way.

[DevYukine]

in my opinion this is pointless since the "perfomance" increase is extremly small while you need an extra dependency. -1 from me

[SimplyLinn]

I went over the benchmarks and compiled a list of relative performance:

  7% - Get a list of listeners
 87% - Instantiate emitter
 87% - Emit, single event, single listener, with prior listeners being registered and removed
 15% - Emit with multiple listeners to a single event
109% - Emit with `this` bound
235% - Registering `.once` listener
 -9% - Emit to 10 events with 10 listeners each
 96% - Emit to single listener with a single event registered
280% - Add and then remove a listener

(Percentages are relative improvements to node events, 100% means it performs its operations twice as fast, 0% means equal performance)

There are quite a few impressive numbers in there, but most of the more impressive ones have to do with instantiation, registration or when there's only a single event and listener. Most notably, in the case where there were multiple events and multiple listeners, EE3 performed worse, and in the case where there were multiple listeners to only a single event, the improvement was only 15%. It was missing a benchmark I think is most relevant to us though, which is single listener, per event, multiple events.

Overall I would not be against this switch, seeing as the overall performance is better, but I also don't think that the time it takes to emit an event is a large enough portion of the overall execution that it's important to optimize. I honestly don't care that much about which one we use, but I thought I'd provide the statistics so the maintainers can at a glance put the metrics into context of the PR to aid them in the decision making

(Note the benchmarks are provided by the EE3 repo, and I used the EE3 master branch when doing the comparison)

[DevYukine]

Hmm when i see these numbers its actuall more performant then i though i guess it would not the worst to make it an optional dependency like uws, bufferutil and other optional dependencies from d.js and give it a try but i would still prefer to throw on no registered error listeners tho without using private api stuff. is there really no way for that without using private API's @Shinobu1337 ?

[ghost]

@Dev-Yukine take a look at @kyranet's comment earlier. If we made a class to extend EE3, it would work. However, performance would have to be taken into serious account with how we extend the emit method. We could use the listenerCount method, which is a public API.

[ghost]

Something like this, perhaps:

const EE3 = require('eventemitter3');

class EventEmitter extends EE3 {
  emit(event, ...args) {
    if (event === 'error' && this.listenerCount('error') === 0)
      throw args[0];
    return super.emit(event, ...args);
  }
}

[amishshah]

We've discussed adding support for EE3 but since it's slower at actually emitting events (and adding this wrapper to support error events properly would make it even slower) we've decided to not add support for EE3.