Skip to content

Commit

Permalink
[major] Make Websocket#addEventListener() ignore non standard events
Browse files Browse the repository at this point in the history 
Make `Websocket.prototype.addEventListener()` a noop if the `type`
argument is not one of `'close'`, `'error'`, `'message'`, or `'open'`.
  • Loading branch information
@lpinca
lpinca committed Jul 16, 2021
1 parent 77a675c commit a421eb5
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 58 deletions.
4 changes: 3 additions & 1 deletion doc/ws.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion lib/constants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: () => {}
};
26 changes: 11 additions & 15 deletions lib/event-target.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

const { kListener } = require('./constants');

/**
* Class representing an event.
*
Expand Down Expand Up @@ -126,8 +128,6 @@ const EventTarget = {
* @public
*/
addEventListener(type, listener, options) {
if (typeof listener !== 'function') return;

function onMessage(data, isBinary) {
listener.call(
this,
Expand All @@ -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);

This comment has been minimized.

Sign in to view

Copy link
@Y--

Y-- Sep 15, 2021

@lpinca a couple of comments about this change:

  • shouldn't the else case throw instead of silently ignoring the event? (that would save time for people using the API incorrectly :-)
  • shouldn't the ping event be supported here?
  • shouldn't on(type, listener) behave like addEventListener(type, listener)?

Let me know if that makes sense, I'm happy to submit a PR!

This comment has been minimized.

Sign in to view

Copy link
@lpinca

lpinca Sep 15, 2021

Author Member

shouldn't the else case throw instead of silently ignoring the event? (that would save time for people using the API incorrectly :-)

Perhaps but this was clearly documented in https://github.com/websockets/ws/releases/tag/8.0.0. I'm not very happy with it.

shouldn't the ping event be supported here?

No, it's a non "standard" event.

shouldn't on(type, listener) behave like addEventListener(type, listener)?

No, this was removed on purpose for the same reason.

This comment has been minimized.

Sign in to view

Copy link
@lpinca

lpinca Sep 15, 2021

Author Member

This API should work in the same way it does in the browser. For non "standard" events use the EventEmitter API.

This comment has been minimized.

Sign in to view

Copy link
@Y--

Y-- Sep 15, 2021

I see, thanks for the quick response and clarifications.
Just to be sure, when you say "Perhaps but this was clearly documented [...] I'm not very happy with it", you're not happy with the idea of having it throw or are you referring to something else? :-)

This comment has been minimized.

Sign in to view

Copy link
@lpinca

lpinca Sep 15, 2021 •
edited
Loading

Author Member

You're not happy with the idea of having it throw

Yes, I would prefer to not throw errors. No error is thrown in the browser if the event is not supported.
}
},

/**
* 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);
}
}
}
Expand Down
18 changes: 10 additions & 8 deletions lib/websocket.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const {
BINARY_TYPES,
EMPTY_BUFFER,
GUID,
kListener,
kStatusCode,
kWebSocket,
NOOP
Expand Down Expand Up @@ -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);
}
});
});
Expand Down
60 changes: 27 additions & 33 deletions test/websocket.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
Expand Down Expand Up @@ -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) => {
Expand Down

1 comment on commit a421eb5

@lpinca
Copy link
Member Author

@lpinca lpinca commented on a421eb5 Jul 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are typos in commit message:

  • Websocket#addEventListener() -> WebSocket#addEventListener().
  • Websocket.prototype.addEventListener() -> WebSocket.prototype.addEventListener().

Please sign in to comment.