[major] Make WebSocket#addEventListener() ignore non standard events

Make `WebSocket.prototype.addEventListener()` a noop if the `type`
argument is not one of `'close'`, `'error'`, `'message'`, or `'open'`.

Author: lpinca

doc/ws.md

@@ -370,7 +370,9 @@ handshake. This allows you to read headers from the server, for example
     at most once after being added. If `true`, the listener would be
     automatically removed when invoked.
 
-Register an event listener emulating the `EventTarget` interface.
+Register an event listener emulating the `EventTarget` interface. This method
+does nothing if `type` is not one of `'close'`, `'error'`, `'message'`, or
+`'open'`.
 
 ### websocket.binaryType
 

lib/constants.js

@@ -2,9 +2,10 @@
 
 module.exports = {
   BINARY_TYPES: ['nodebuffer', 'arraybuffer', 'fragments'],
+  EMPTY_BUFFER: Buffer.alloc(0),
   GUID: '258EAFA5-E914-47DA-95CA-C5AB0DC85B11',
+  kListener: Symbol('kListener'),
   kStatusCode: Symbol('status-code'),
   kWebSocket: Symbol('websocket'),
-  EMPTY_BUFFER: Buffer.alloc(0),
   NOOP: () => {}
 };

lib/event-target.js

@@ -1,5 +1,7 @@
 'use strict';
 
+const { kListener } = require('./constants');
+
 /**
  * Class representing an event.
  *
@@ -126,8 +128,6 @@ const EventTarget = {
    * @public
    */
   addEventListener(type, listener, options) {
-    if (typeof listener !== 'function') return;
-
     function onMessage(data, isBinary) {
       listener.call(
         this,
@@ -150,35 +150,31 @@ const EventTarget = {
     const method = options && options.once ? 'once' : 'on';
 
     if (type === 'message') {
-      onMessage._listener = listener;
+      onMessage[kListener] = listener;
       this[method](type, onMessage);
     } else if (type === 'close') {
-      onClose._listener = listener;
+      onClose[kListener] = listener;
       this[method](type, onClose);
     } else if (type === 'error') {
-      onError._listener = listener;
+      onError[kListener] = listener;
       this[method](type, onError);
     } else if (type === 'open') {
-      onOpen._listener = listener;
+      onOpen[kListener] = listener;
       this[method](type, onOpen);
-    } else {
-      this[method](type, listener);
     }
   },
 
   /**
    * Remove an event listener.
    *
    * @param {String} type A string representing the event type to remove
-   * @param {Function} listener The listener to remove
+   * @param {Function} handler The listener to remove
    * @public
    */
-  removeEventListener(type, listener) {
-    const listeners = this.listeners(type);
-
-    for (let i = 0; i < listeners.length; i++) {
-      if (listeners[i] === listener || listeners[i]._listener === listener) {
-        this.removeListener(type, listeners[i]);
+  removeEventListener(type, handler) {
+    for (const listener of this.listeners(type)) {
+      if (listener === handler || listener[kListener] === handler) {
+        this.removeListener(type, listener);
       }
     }
   }

lib/websocket.js

@@ -15,6 +15,7 @@ const {
   BINARY_TYPES,
   EMPTY_BUFFER,
   GUID,
+  kListener,
   kStatusCode,
   kWebSocket,
   NOOP
@@ -522,22 +523,23 @@ Object.defineProperty(WebSocket.prototype, 'CLOSED', {
   Object.defineProperty(WebSocket.prototype, `on${method}`, {
     enumerable: true,
     get() {
-      const listeners = this.listeners(method);
-      for (let i = 0; i < listeners.length; i++) {
-        if (listeners[i]._listener) return listeners[i]._listener;
+      for (const listener of this.listeners(method)) {
+        if (listener[kListener]) return listener[kListener];
       }
 
       return undefined;
     },
-    set(listener) {
-      const listeners = this.listeners(method);
-      for (let i = 0; i < listeners.length; i++) {
+    set(handler) {
+      for (const listener of this.listeners(method)) {
         //
         // Remove only the listeners added via `addEventListener`.
         //
-        if (listeners[i]._listener) this.removeListener(method, listeners[i]);
+        if (listener[kListener]) this.removeListener(method, listener);
       }
-      this.addEventListener(method, listener);
+
+      if (typeof handler !== 'function') return;
+
+      this.addEventListener(method, handler);
     }
   });
 });

test/websocket.test.js

@@ -11,7 +11,7 @@ const fs = require('fs');
 const { URL } = require('url');
 
 const WebSocket = require('..');
-const { EMPTY_BUFFER, GUID, NOOP } = require('../lib/constants');
+const { EMPTY_BUFFER, GUID, kListener, NOOP } = require('../lib/constants');
 
 class CustomAgent extends http.Agent {
   addRequest() {}
@@ -2157,88 +2157,82 @@ describe('WebSocket', () => {
 
       assert.strictEqual(listeners.length, 2);
       assert.strictEqual(listeners[0], NOOP);
-      assert.strictEqual(listeners[1]._listener, NOOP);
+      assert.strictEqual(listeners[1][kListener], NOOP);
 
       ws.onclose = NOOP;
 
       listeners = ws.listeners('close');
 
       assert.strictEqual(listeners.length, 2);
       assert.strictEqual(listeners[0], NOOP);
-      assert.strictEqual(listeners[1]._listener, NOOP);
+      assert.strictEqual(listeners[1][kListener], NOOP);
     });
 
-    it('adds listeners for custom events with `addEventListener`', () => {
+    it('supports the `addEventListener` method', () => {
+      const events = [];
       const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() });
 
-      ws.addEventListener('foo', NOOP);
-      assert.strictEqual(ws.listeners('foo')[0], NOOP);
+      ws.addEventListener('foo', () => {});
+      assert.strictEqual(ws.listenerCount('foo'), 0);
 
-      //
-      // Fails silently when the `listener` is not a function.
-      //
-      ws.addEventListener('bar', {});
-      assert.strictEqual(ws.listeners('bar').length, 0);
-    });
+      ws.addEventListener('open', () => {
+        events.push('open');
+        assert.strictEqual(ws.listenerCount('open'), 1);
+      });
 
-    it('allows to add one time listeners with `addEventListener`', (done) => {
-      const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() });
+      assert.strictEqual(ws.listenerCount('open'), 1);
 
       ws.addEventListener(
-        'foo',
+        'message',
         () => {
-          assert.strictEqual(ws.listenerCount('foo'), 0);
-          done();
+          events.push('message');
+          assert.strictEqual(ws.listenerCount('message'), 0);
         },
         { once: true }
       );
 
-      assert.strictEqual(ws.listenerCount('foo'), 1);
-      ws.emit('foo');
+      assert.strictEqual(ws.listenerCount('message'), 1);
+
+      ws.emit('open');
+      ws.emit('message', EMPTY_BUFFER, false);
+
+      assert.deepStrictEqual(events, ['open', 'message']);
     });
 
     it('supports the `removeEventListener` method', () => {
       const ws = new WebSocket('ws://localhost', { agent: new CustomAgent() });
 
       ws.addEventListener('message', NOOP);
       ws.addEventListener('open', NOOP);
-      ws.addEventListener('foo', NOOP);
 
-      assert.strictEqual(ws.listeners('message')[0]._listener, NOOP);
-      assert.strictEqual(ws.listeners('open')[0]._listener, NOOP);
-      assert.strictEqual(ws.listeners('foo')[0], NOOP);
+      assert.strictEqual(ws.listeners('message')[0][kListener], NOOP);
+      assert.strictEqual(ws.listeners('open')[0][kListener], NOOP);
 
       ws.removeEventListener('message', () => {});
 
-      assert.strictEqual(ws.listeners('message')[0]._listener, NOOP);
+      assert.strictEqual(ws.listeners('message')[0][kListener], NOOP);
 
       ws.removeEventListener('message', NOOP);
       ws.removeEventListener('open', NOOP);
-      ws.removeEventListener('foo', NOOP);
 
       assert.strictEqual(ws.listenerCount('message'), 0);
       assert.strictEqual(ws.listenerCount('open'), 0);
-      assert.strictEqual(ws.listenerCount('foo'), 0);
 
       ws.addEventListener('message', NOOP, { once: true });
       ws.addEventListener('open', NOOP, { once: true });
-      ws.addEventListener('foo', NOOP, { once: true });
 
-      assert.strictEqual(ws.listeners('message')[0]._listener, NOOP);
-      assert.strictEqual(ws.listeners('open')[0]._listener, NOOP);
-      assert.strictEqual(ws.listeners('foo')[0], NOOP);
+      assert.strictEqual(ws.listeners('message')[0][kListener], NOOP);
+      assert.strictEqual(ws.listeners('open')[0][kListener], NOOP);
 
       ws.removeEventListener('message', () => {});
 
-      assert.strictEqual(ws.listeners('message')[0]._listener, NOOP);
+      assert.strictEqual(ws.listeners('message')[0][kListener], NOOP);
 
       ws.removeEventListener('message', NOOP);
       ws.removeEventListener('open', NOOP);
-      ws.removeEventListener('foo', NOOP);
 
       assert.strictEqual(ws.listenerCount('message'), 0);
       assert.strictEqual(ws.listenerCount('open'), 0);
-      assert.strictEqual(ws.listenerCount('foo'), 0);
     });
 
     it('wraps text data in a `MessageEvent`', (done) => {