Noissue - Use Map to store EventEmitter listeners

lib/events.js

Index: lib/events.js
diff --git a/lib/events.js b/lib/events.js
index 3e9067a2b1bfe88ccace767b19ded6fdc42c5be1..ace413d2a26ac6e45f29cbef6f7d61acdc83284f 100644
--- a/lib/events.js
+++ b/lib/events.js
@@ -24,7 +24,7 @@
  */
 exports.EventEmitter = function()
 {
-  this._listeners = Object.create(null);
+  this._listeners = new Map();
 };
 
 exports.EventEmitter.prototype = {
@@ -36,10 +36,10 @@ exports.EventEmitter.prototype = {
    */
   on(name, listener)
   {
-    if (name in this._listeners)
-      this._listeners[name].push(listener);
+    if (this._listeners.has(name))

[Manish Jethani, 2018/04/01 08:44:16]

It's usually not worth calling Map.has if you're going to call Map.get anyway.
It would be better to do this instead:

  let listeners = this._listeners.get(name);
  if (listeners)
    listeners.push(listener);
  else
    this._listeners.set(name, [listener]);

[kzar, 2018/04/03 17:27:55]

Done.

+      this._listeners.get(name).push(listener);
     else
-      this._listeners[name] = [listener];
+      this._listeners.set(name, [listener]);
   },
 
   /**
@@ -50,7 +50,7 @@ exports.EventEmitter.prototype = {
    */
   off(name, listener)
   {
-    let listeners = this._listeners[name];
+    let listeners = this._listeners.get(name);

[Manish Jethani, 2018/04/01 08:44:16]

While we're at it, perhaps it would be a good idea to make listeners a Set
object. Two benefits: (1) no duplicate listeners (similar to other event APIs),
and (2) faster deletion.

[kzar, 2018/04/03 17:27:55]

Good idea, but I'd rather not since this would change behaviour subtly and if
we're unlucky lead to a regression somewhere. I would support the change in the
future if someone™ takes the time to verify nothing (including libadblockplus)
breaks!

[Sebastian Noack, 2018/04/03 20:42:34]

What change of behavior are you concerned about? Mind that Set() objects
preserve the order in which values are added. So the only difference in behavior
would be that if the same listener is attempted to be added more than once, it
will effectively only be registered once. I cannot think of any situation where
it would be intended that the same listener is called more than once. On the
other hand it would notably speedup listener removal (and simplifies the code a
bit). So I think the risk here (if any) would be justified.

FWIW, replacing a plain object with a Map() already introduces (similarly small)
changes in behavior, e.g. keys are no longer coerced to strings.

[Manish Jethani, 2018/04/04 06:54:49]

Dave has a point that this would be more likely to break something. For example
every time you open a window you might add the same "close" listener, which
operates on the event's target and removes itself. The number of times it's
added would be equal to the number of windows open. Now this code would break
because the listener would get removed after the first window is closed.

So perhaps it should be a separate patch.
This is less serious in my opinion.

     if (listeners)
     {
       let idx = listeners.indexOf(listener);
@@ -87,7 +87,7 @@ exports.EventEmitter.prototype = {
    */
   listeners(name)
   {
-    let listeners = this._listeners[name];
+    let listeners = this._listeners.get(name);
     return listeners ? listeners.slice() : [];
   },