events: fire handlers in correct oder

PR-URL: #35931 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Nov 7, 2020 · d396900 · d396900
1 parent 1c99cde
commit d396900
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 9 deletions.
diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -9,6 +9,7 @@ const {
   ObjectDefineProperties,
   ObjectDefineProperty,
   ObjectGetOwnPropertyDescriptor,
+  ReflectApply,
   SafeMap,
   String,
   Symbol,
@@ -578,24 +579,47 @@ function emitUnhandledRejectionOrErr(that, err, event) {
   process.emit('error', err, event);
 }
 
+function makeEventHandler(handler) {
+  // Event handlers are dispatched in the order they were first set
+  // See https://github.com/nodejs/node/pull/35949#issuecomment-722496598
+  function eventHandler(...args) {
+    if (typeof eventHandler.handler !== 'function') {
+      return;
+    }
+    return ReflectApply(eventHandler.handler, this, args);
+  }
+  eventHandler.handler = handler;
+  return eventHandler;
+}
+
 function defineEventHandler(emitter, name) {
   // 8.1.5.1 Event handlers - basically `on[eventName]` attributes
   ObjectDefineProperty(emitter, `on${name}`, {
     get() {
-      return this[kHandlers]?.get(name);
+      return this[kHandlers]?.get(name)?.handler;
     },
     set(value) {
-      const oldValue = this[kHandlers]?.get(name);
-      if (oldValue) {
-        this.removeEventListener(name, oldValue);
-      }
-      if (typeof value === 'function') {
-        this.addEventListener(name, value);
-      }
       if (!this[kHandlers]) {
         this[kHandlers] = new SafeMap();
       }
-      this[kHandlers].set(name, value);
+      let wrappedHandler = this[kHandlers]?.get(name);
+      if (wrappedHandler) {
+        if (typeof wrappedHandler.handler === 'function') {
+          this[kEvents].get(name).size--;
+          const size = this[kEvents].get(name).size;
+          this[kRemoveListener](size, name, wrappedHandler.handler, false);
+        }
+        wrappedHandler.handler = value;
+        if (typeof wrappedHandler.handler === 'function') {
+          this[kEvents].get(name).size++;
+          const size = this[kEvents].get(name).size;
+          this[kNewListener](size, name, value, false, false, false);
+        }
+      } else {
+        wrappedHandler = makeEventHandler(value);
+        this.addEventListener(name, wrappedHandler);
+      }
+      this[kHandlers].set(name, wrappedHandler);
     },
     configurable: true,
     enumerable: true

diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js
@@ -524,3 +524,15 @@ let asyncTest = Promise.resolve();
   strictEqual(descriptor.configurable, true);
   strictEqual(descriptor.enumerable, true);
 }
+{
+  const target = new EventTarget();
+  defineEventHandler(target, 'foo');
+  const output = [];
+  target.addEventListener('foo', () => output.push(1));
+  target.onfoo = common.mustNotCall();
+  target.addEventListener('foo', () => output.push(3));
+  target.onfoo = () => output.push(2);
+  target.addEventListener('foo', () => output.push(4));
+  target.dispatchEvent(new Event('foo'));
+  deepStrictEqual(output, [1, 2, 3, 4]);
+}