events: improve argument handling, start passive

Co-authored-by: Benjamin Gruenbaum <benjamingr@gmail.com>

PR-URL: https://github.com/nodejs/node/pull/34015
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

lib/internal/event_target.js

@@ -203,15 +203,31 @@ class EventTarget {
   [kRemoveListener](size, type, listener, capture) {}
 
   addEventListener(type, listener, options = {}) {
-    validateListener(listener);
-    type = String(type);
+    if (arguments.length < 2)
+      throw new ERR_MISSING_ARGS('type', 'listener');
 
+    // We validateOptions before the shouldAddListeners check because the spec
+    // requires us to hit getters.
     const {
       once,
       capture,
       passive
     } = validateEventListenerOptions(options);
 
+    if (!shouldAddListener(listener)) {
+      // The DOM silently allows passing undefined as a second argument
+      // No error code for this since it is a Warning
+      // eslint-disable-next-line no-restricted-syntax
+      const w = new Error(`addEventListener called with ${listener}` +
+                          ' which has no effect.');
+      w.name = 'AddEventListenerArgumentTypeWarning';
+      w.target = this;
+      w.type = type;
+      process.emitWarning(w);
+      return;
+    }
+    type = String(type);
+
     let root = this[kEvents].get(type);
 
     if (root === undefined) {
@@ -242,9 +258,15 @@ class EventTarget {
   }
 
   removeEventListener(type, listener, options = {}) {
-    validateListener(listener);
+    if (!shouldAddListener(listener))
+      return;
+
     type = String(type);
-    const { capture } = validateEventListenerOptions(options);
+    // TODO(@jasnell): If it's determined this cannot be backported
+    // to 12.x, then this can be simplified to:
+    //   const capture = Boolean(options?.capture);
+    const capture = options != null && options.capture === true;
+
     const root = this[kEvents].get(type);
     if (root === undefined || root.next === undefined)
       return;
@@ -426,13 +448,17 @@ Object.defineProperties(NodeEventTarget.prototype, {
 
 // EventTarget API
 
-function validateListener(listener) {
+function shouldAddListener(listener) {
   if (typeof listener === 'function' ||
       (listener != null &&
        typeof listener === 'object' &&
        typeof listener.handleEvent === 'function')) {
-    return;
+    return true;
   }
+
+  if (listener == null)
+    return false;
+
   throw new ERR_INVALID_ARG_TYPE('listener', 'EventListener', listener);
 }
 

test/parallel/test-eventtarget-whatwg-passive.js

@@ -0,0 +1,69 @@
+// Flags: --expose-internals
+'use strict';
+
+const common = require('../common');
+
+const {
+  Event,
+  EventTarget,
+} = require('internal/event_target');
+
+const {
+  fail,
+  ok,
+  strictEqual
+} = require('assert');
+
+// Manually ported from WPT AddEventListenerOptions-passive.html
+{
+  const document = new EventTarget();
+  let supportsPassive = false;
+  const query_options = {
+    get passive() {
+      supportsPassive = true;
+      return false;
+    },
+    get dummy() {
+      fail('dummy value getter invoked');
+      return false;
+    }
+  };
+
+  document.addEventListener('test_event', null, query_options);
+  ok(supportsPassive);
+
+  supportsPassive = false;
+  document.removeEventListener('test_event', null, query_options);
+  strictEqual(supportsPassive, false);
+}
+{
+  function testPassiveValue(optionsValue, expectedDefaultPrevented) {
+    const document = new EventTarget();
+    let defaultPrevented;
+    function handler(e) {
+      if (e.defaultPrevented) {
+        fail('Event prematurely marked defaultPrevented');
+      }
+      e.preventDefault();
+      defaultPrevented = e.defaultPrevented;
+    }
+    document.addEventListener('test', handler, optionsValue);
+    // TODO the WHATWG test is more extensive here and tests dispatching on
+    // document.body, if we ever support getParent we should amend this
+    const ev = new Event('test', { bubbles: true, cancelable: true });
+    const uncanceled = document.dispatchEvent(ev);
+
+    strictEqual(defaultPrevented, expectedDefaultPrevented);
+    strictEqual(uncanceled, !expectedDefaultPrevented);
+
+    document.removeEventListener('test', handler, optionsValue);
+  }
+  testPassiveValue(undefined, true);
+  testPassiveValue({}, true);
+  testPassiveValue({ passive: false }, true);
+
+  common.skip('TODO: passive listeners is still broken');
+  testPassiveValue({ passive: 1 }, false);
+  testPassiveValue({ passive: true }, false);
+  testPassiveValue({ passive: 0 }, true);
+}

test/parallel/test-eventtarget.js

@@ -5,7 +5,6 @@ const common = require('../common');
 const {
   Event,
   EventTarget,
-  NodeEventTarget,
   defineEventHandler
 } = require('internal/event_target');
 
@@ -16,12 +15,26 @@ const {
   throws,
 } = require('assert');
 
-const { once, on } = require('events');
+const { once } = require('events');
+
+const { promisify } = require('util');
+const delay = promisify(setTimeout);
 
 // The globals are defined.
 ok(Event);
 ok(EventTarget);
 
+// The warning event has special behavior regarding attaching listeners
+let lastWarning;
+process.on('warning', (e) => {
+  lastWarning = e;
+});
+
+// Utility promise for parts of the test that need to wait for eachother -
+// Namely tests for warning events
+/* eslint-disable no-unused-vars */
+let asyncTest = Promise.resolve();
+
 // First, test Event
 {
   const ev = new Event('foo');
@@ -135,35 +148,6 @@ ok(EventTarget);
   eventTarget.addEventListener('foo', fn, { once: true });
   eventTarget.dispatchEvent(ev);
 }
-{
-  const eventTarget = new NodeEventTarget();
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  deepStrictEqual(eventTarget.eventNames(), []);
-
-  const ev1 = common.mustCall(function(event) {
-    strictEqual(event.type, 'foo');
-    strictEqual(this, eventTarget);
-  }, 2);
-
-  const ev2 = {
-    handleEvent: common.mustCall(function(event) {
-      strictEqual(event.type, 'foo');
-      strictEqual(this, ev2);
-    })
-  };
-
-  eventTarget.addEventListener('foo', ev1);
-  eventTarget.addEventListener('foo', ev2, { once: true });
-  strictEqual(eventTarget.listenerCount('foo'), 2);
-  ok(eventTarget.dispatchEvent(new Event('foo')));
-  strictEqual(eventTarget.listenerCount('foo'), 1);
-  eventTarget.dispatchEvent(new Event('foo'));
-
-  eventTarget.removeEventListener('foo', ev1);
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  eventTarget.dispatchEvent(new Event('foo'));
-}
-
 
 {
   const eventTarget = new EventTarget();
@@ -179,88 +163,6 @@ ok(EventTarget);
   eventTarget.addEventListener('foo', fn, false);
   eventTarget.dispatchEvent(event);
 }
-{
-  const eventTarget = new NodeEventTarget();
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  deepStrictEqual(eventTarget.eventNames(), []);
-
-  const ev1 = common.mustCall((event) => {
-    strictEqual(event.type, 'foo');
-  }, 2);
-
-  const ev2 = {
-    handleEvent: common.mustCall((event) => {
-      strictEqual(event.type, 'foo');
-    })
-  };
-
-  strictEqual(eventTarget.on('foo', ev1), eventTarget);
-  strictEqual(eventTarget.once('foo', ev2, { once: true }), eventTarget);
-  strictEqual(eventTarget.listenerCount('foo'), 2);
-  eventTarget.dispatchEvent(new Event('foo'));
-  strictEqual(eventTarget.listenerCount('foo'), 1);
-  eventTarget.dispatchEvent(new Event('foo'));
-
-  strictEqual(eventTarget.off('foo', ev1), eventTarget);
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  eventTarget.dispatchEvent(new Event('foo'));
-}
-
-{
-  const eventTarget = new NodeEventTarget();
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  deepStrictEqual(eventTarget.eventNames(), []);
-
-  const ev1 = common.mustCall((event) => {
-    strictEqual(event.type, 'foo');
-  }, 2);
-
-  const ev2 = {
-    handleEvent: common.mustCall((event) => {
-      strictEqual(event.type, 'foo');
-    })
-  };
-
-  eventTarget.addListener('foo', ev1);
-  eventTarget.once('foo', ev2, { once: true });
-  strictEqual(eventTarget.listenerCount('foo'), 2);
-  eventTarget.dispatchEvent(new Event('foo'));
-  strictEqual(eventTarget.listenerCount('foo'), 1);
-  eventTarget.dispatchEvent(new Event('foo'));
-
-  eventTarget.removeListener('foo', ev1);
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  eventTarget.dispatchEvent(new Event('foo'));
-}
-
-{
-  const eventTarget = new NodeEventTarget();
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  deepStrictEqual(eventTarget.eventNames(), []);
-
-  // Won't actually be called.
-  const ev1 = () => {};
-
-  // Won't actually be called.
-  const ev2 = { handleEvent() {} };
-
-  eventTarget.addListener('foo', ev1);
-  eventTarget.addEventListener('foo', ev1);
-  eventTarget.once('foo', ev2, { once: true });
-  eventTarget.once('foo', ev2, { once: false });
-  eventTarget.on('bar', ev1);
-  strictEqual(eventTarget.listenerCount('foo'), 2);
-  strictEqual(eventTarget.listenerCount('bar'), 1);
-  deepStrictEqual(eventTarget.eventNames(), ['foo', 'bar']);
-  eventTarget.removeAllListeners('foo');
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  strictEqual(eventTarget.listenerCount('bar'), 1);
-  deepStrictEqual(eventTarget.eventNames(), ['bar']);
-  eventTarget.removeAllListeners();
-  strictEqual(eventTarget.listenerCount('foo'), 0);
-  strictEqual(eventTarget.listenerCount('bar'), 0);
-  deepStrictEqual(eventTarget.eventNames(), []);
-}
 
 {
   const uncaughtException = common.mustCall((err, event) => {
@@ -328,7 +230,6 @@ ok(EventTarget);
     1,
     {},  // No handleEvent function
     false,
-    undefined
   ].forEach((i) => {
     throws(() => target.addEventListener('foo', i), {
       code: 'ERR_INVALID_ARG_TYPE'
@@ -339,8 +240,7 @@ ok(EventTarget);
     'foo',
     1,
     {},  // No handleEvent function
-    false,
-    undefined
+    false
   ].forEach((i) => {
     throws(() => target.removeEventListener('foo', i), {
       code: 'ERR_INVALID_ARG_TYPE'
@@ -354,25 +254,6 @@ ok(EventTarget);
   target.dispatchEvent(new Event('foo'));
 }
 
-{
-  const target = new NodeEventTarget();
-
-  process.on('warning', common.mustCall((warning) => {
-    ok(warning instanceof Error);
-    strictEqual(warning.name, 'MaxListenersExceededWarning');
-    strictEqual(warning.target, target);
-    strictEqual(warning.count, 2);
-    strictEqual(warning.type, 'foo');
-    ok(warning.message.includes(
-      '2 foo listeners added to NodeEventTarget'));
-  }));
-
-  strictEqual(target.getMaxListeners(), NodeEventTarget.defaultMaxListeners);
-  target.setMaxListeners(1);
-  target.on('foo', () => {});
-  target.on('foo', () => {});
-}
-
 {
   const target = new EventTarget();
   const event = new Event('foo');
@@ -543,19 +424,41 @@ ok(EventTarget);
   target.dispatchEvent(ev);
 }
 
-(async () => {
-  // test NodeEventTarget async-iterability
-  const emitter = new NodeEventTarget();
-  const interval = setInterval(() => {
-    emitter.dispatchEvent(new Event('foo'));
-  }, 0);
-  let count = 0;
-  for await (const [ item ] of on(emitter, 'foo')) {
-    count++;
-    strictEqual(item.type, 'foo');
-    if (count > 5) {
-      break;
-    }
-  }
-  clearInterval(interval);
-})().then(common.mustCall());
+{
+  const eventTarget = new EventTarget();
+  // Single argument throws
+  throws(() => eventTarget.addEventListener('foo'), TypeError);
+  // Null events - does not throw
+  eventTarget.addEventListener('foo', null);
+  eventTarget.removeEventListener('foo', null);
+  eventTarget.addEventListener('foo', undefined);
+  eventTarget.removeEventListener('foo', undefined);
+  // Strings, booleans
+  throws(() => eventTarget.addEventListener('foo', 'hello'), TypeError);
+  throws(() => eventTarget.addEventListener('foo', false), TypeError);
+  throws(() => eventTarget.addEventListener('foo', Symbol()), TypeError);
+  asyncTest = asyncTest.then(async () => {
+    const eventTarget = new EventTarget();
+    // Single argument throws
+    throws(() => eventTarget.addEventListener('foo'), TypeError);
+    // Null events - does not throw
+
+    eventTarget.addEventListener('foo', null);
+    eventTarget.removeEventListener('foo', null);
+
+    // Warnings always happen after nextTick, so wait for a timer of 0
+    await delay(0);
+    strictEqual(lastWarning.name, 'AddEventListenerArgumentTypeWarning');
+    strictEqual(lastWarning.target, eventTarget);
+    lastWarning = null;
+    eventTarget.addEventListener('foo', undefined);
+    await delay(0);
+    strictEqual(lastWarning.name, 'AddEventListenerArgumentTypeWarning');
+    strictEqual(lastWarning.target, eventTarget);
+    eventTarget.removeEventListener('foo', undefined);
+    // Strings, booleans
+    throws(() => eventTarget.addEventListener('foo', 'hello'), TypeError);
+    throws(() => eventTarget.addEventListener('foo', false), TypeError);
+    throws(() => eventTarget.addEventListener('foo', Symbol()), TypeError);
+  });
+}

test/parallel/test-nodeeventtarget.js

@@ -0,0 +1,164 @@
+// Flags: --expose-internals --no-warnings
+'use strict';
+
+const common = require('../common');
+const {
+  Event,
+  NodeEventTarget,
+} = require('internal/event_target');
+
+const {
+  deepStrictEqual,
+  ok,
+  strictEqual,
+} = require('assert');
+
+const { on } = require('events');
+
+{
+  const eventTarget = new NodeEventTarget();
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  deepStrictEqual(eventTarget.eventNames(), []);
+
+  const ev1 = common.mustCall(function(event) {
+    strictEqual(event.type, 'foo');
+    strictEqual(this, eventTarget);
+  }, 2);
+
+  const ev2 = {
+    handleEvent: common.mustCall(function(event) {
+      strictEqual(event.type, 'foo');
+      strictEqual(this, ev2);
+    })
+  };
+
+  eventTarget.addEventListener('foo', ev1);
+  eventTarget.addEventListener('foo', ev2, { once: true });
+  strictEqual(eventTarget.listenerCount('foo'), 2);
+  ok(eventTarget.dispatchEvent(new Event('foo')));
+  strictEqual(eventTarget.listenerCount('foo'), 1);
+  eventTarget.dispatchEvent(new Event('foo'));
+
+  eventTarget.removeEventListener('foo', ev1);
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  eventTarget.dispatchEvent(new Event('foo'));
+}
+
+{
+  const eventTarget = new NodeEventTarget();
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  deepStrictEqual(eventTarget.eventNames(), []);
+
+  const ev1 = common.mustCall((event) => {
+    strictEqual(event.type, 'foo');
+  }, 2);
+
+  const ev2 = {
+    handleEvent: common.mustCall((event) => {
+      strictEqual(event.type, 'foo');
+    })
+  };
+
+  strictEqual(eventTarget.on('foo', ev1), eventTarget);
+  strictEqual(eventTarget.once('foo', ev2, { once: true }), eventTarget);
+  strictEqual(eventTarget.listenerCount('foo'), 2);
+  eventTarget.dispatchEvent(new Event('foo'));
+  strictEqual(eventTarget.listenerCount('foo'), 1);
+  eventTarget.dispatchEvent(new Event('foo'));
+
+  strictEqual(eventTarget.off('foo', ev1), eventTarget);
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  eventTarget.dispatchEvent(new Event('foo'));
+}
+
+{
+  const eventTarget = new NodeEventTarget();
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  deepStrictEqual(eventTarget.eventNames(), []);
+
+  const ev1 = common.mustCall((event) => {
+    strictEqual(event.type, 'foo');
+  }, 2);
+
+  const ev2 = {
+    handleEvent: common.mustCall((event) => {
+      strictEqual(event.type, 'foo');
+    })
+  };
+
+  eventTarget.addListener('foo', ev1);
+  eventTarget.once('foo', ev2, { once: true });
+  strictEqual(eventTarget.listenerCount('foo'), 2);
+  eventTarget.dispatchEvent(new Event('foo'));
+  strictEqual(eventTarget.listenerCount('foo'), 1);
+  eventTarget.dispatchEvent(new Event('foo'));
+
+  eventTarget.removeListener('foo', ev1);
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  eventTarget.dispatchEvent(new Event('foo'));
+}
+
+{
+  const eventTarget = new NodeEventTarget();
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  deepStrictEqual(eventTarget.eventNames(), []);
+
+  // Won't actually be called.
+  const ev1 = () => {};
+
+  // Won't actually be called.
+  const ev2 = { handleEvent() {} };
+
+  eventTarget.addListener('foo', ev1);
+  eventTarget.addEventListener('foo', ev1);
+  eventTarget.once('foo', ev2, { once: true });
+  eventTarget.once('foo', ev2, { once: false });
+  eventTarget.on('bar', ev1);
+  strictEqual(eventTarget.listenerCount('foo'), 2);
+  strictEqual(eventTarget.listenerCount('bar'), 1);
+  deepStrictEqual(eventTarget.eventNames(), ['foo', 'bar']);
+  eventTarget.removeAllListeners('foo');
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  strictEqual(eventTarget.listenerCount('bar'), 1);
+  deepStrictEqual(eventTarget.eventNames(), ['bar']);
+  eventTarget.removeAllListeners();
+  strictEqual(eventTarget.listenerCount('foo'), 0);
+  strictEqual(eventTarget.listenerCount('bar'), 0);
+  deepStrictEqual(eventTarget.eventNames(), []);
+}
+
+{
+  const target = new NodeEventTarget();
+
+  process.on('warning', common.mustCall((warning) => {
+    ok(warning instanceof Error);
+    strictEqual(warning.name, 'MaxListenersExceededWarning');
+    strictEqual(warning.target, target);
+    strictEqual(warning.count, 2);
+    strictEqual(warning.type, 'foo');
+    ok(warning.message.includes(
+      '2 foo listeners added to NodeEventTarget'));
+  }));
+
+  strictEqual(target.getMaxListeners(), NodeEventTarget.defaultMaxListeners);
+  target.setMaxListeners(1);
+  target.on('foo', () => {});
+  target.on('foo', () => {});
+}
+
+(async () => {
+  // test NodeEventTarget async-iterability
+  const emitter = new NodeEventTarget();
+  const interval = setInterval(() => {
+    emitter.dispatchEvent(new Event('foo'));
+  }, 0);
+  let count = 0;
+  for await (const [ item ] of on(emitter, 'foo')) {
+    count++;
+    strictEqual(item.type, 'foo');
+    if (count > 5) {
+      break;
+    }
+  }
+  clearInterval(interval);
+})().then(common.mustCall());