From 8ce4bbc332562a1bdd7df74215285057f7500c1d Mon Sep 17 00:00:00 2001 From: ritave Date: Thu, 2 Jun 2022 16:42:01 +0200 Subject: [PATCH 1/2] Fix #242 --- src/websocket.js | 57 +++++++++++++++++++------ tests/issues/242.test.js | 89 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 12 deletions(-) create mode 100644 tests/issues/242.test.js diff --git a/src/websocket.js b/src/websocket.js index ae4d2447..5b8df302 100644 --- a/src/websocket.js +++ b/src/websocket.js @@ -21,6 +21,11 @@ class WebSocket extends EventTarget { constructor(url, protocols) { super(); + this.onopenCb = null; + this.onmessageCb = null; + this.onerrorCb = null; + this.oncloseCb = null; + this.url = urlVerification(url); protocols = protocolVerification(protocols); this.protocol = protocols[0] || ''; @@ -94,39 +99,67 @@ class WebSocket extends EventTarget { } get onopen() { - return this.listeners.open; + return this.onopenCb; } get onmessage() { - return this.listeners.message; + return this.onmessageCb; } get onclose() { - return this.listeners.close; + return this.oncloseCb; } get onerror() { - return this.listeners.error; + return this.onerrorCb; } set onopen(listener) { - delete this.listeners.open; - this.addEventListener('open', listener); + if (listener === null) { + if (this.onopenCb !== null) { + this.removeEventListener('open', this.onopenCb); + } + } else { + this.onopen = null; + this.addEventListener('open', listener); + } + this.onopenCb = listener; } set onmessage(listener) { - delete this.listeners.message; - this.addEventListener('message', listener); + if (listener === null) { + if (this.onmessageCb !== null) { + this.removeEventListener('message', this.onmessageCb); + } + } else { + this.onmessage = null; + this.addEventListener('message', listener); + } + this.onmessageCb = listener; } set onclose(listener) { - delete this.listeners.close; - this.addEventListener('close', listener); + if (listener === null) { + if (this.oncloseCb !== null) { + this.removeEventListener('close', this.oncloseCb); + } + } else { + this.onclose = null; + this.addEventListener('close', listener); + } + this.oncloseCb = listener; } set onerror(listener) { - delete this.listeners.error; - this.addEventListener('error', listener); + if (listener === null) { + if (this.onerrorCb !== null) { + this.removeEventListener('error', this.onerrorCb); + } + } else { + this.onerror = null; + this.addEventListener('error', listener); + } + this.onerrorCb = listener; } send(data) { diff --git a/tests/issues/242.test.js b/tests/issues/242.test.js new file mode 100644 index 00000000..2ce044ed --- /dev/null +++ b/tests/issues/242.test.js @@ -0,0 +1,89 @@ +import test from 'ava'; +import Server from '../../src/server'; +import WebSocket from '../../src/websocket'; + +test('websocket on* methods family returns a single listener', t => { + const socketUrl = 'ws://localhost:8080'; + const mockServer = new Server(socketUrl); + const mockSocket = new WebSocket(socketUrl); + + const listener = () => { + /* do nothing */ + }; + + mockSocket.onopen = listener; + mockSocket.onmessage = listener; + mockSocket.onerror = listener; + mockSocket.onclose = listener; + + t.is(mockSocket.onopen, listener); + t.is(mockSocket.onmessage, listener); + t.is(mockSocket.onerror, listener); + t.is(mockSocket.onclose, listener); + + mockServer.close(); +}); + +test("websocket on* methods family doesn't delete other listeners", async t => { + const socketUrl = 'ws://localhost:8080'; + const mockServer = new Server(socketUrl); + const mockSocket = new WebSocket(socketUrl); + + mockServer.on('connection', socket => { + socket.send('test message'); + }); + + let onOpenCalled = 0; + let onMessageCalled = 0; + let onErrorCalled = 0; + + let onCloseEventResolve; + let onCloseResolve; + const allClosed = Promise.all([ + new Promise(r => { + onCloseEventResolve = r; + }), + new Promise(r => { + onCloseResolve = r; + }) + ]); + + mockSocket.addEventListener('open', () => { + onOpenCalled += 1; + }); + mockSocket.addEventListener('message', () => { + onMessageCalled += 1; + mockSocket.dispatchEvent(new Event('error')); + }); + mockSocket.addEventListener('error', () => { + onErrorCalled += 1; + mockSocket.close(); + }); + mockSocket.addEventListener('close', () => onCloseEventResolve()); + + const throwCb = () => { + throw new Error('this call should have been replaced'); + }; + mockSocket.onopen = throwCb; + mockSocket.onopen = () => { + onOpenCalled += 1; + }; + mockSocket.onmessage = throwCb; + mockSocket.onmessage = () => { + onMessageCalled += 1; + }; + mockSocket.onerror = throwCb; + mockSocket.onerror = () => { + onErrorCalled += 1; + }; + mockSocket.onclose = throwCb; + mockSocket.onclose = () => onCloseResolve(); + + await allClosed; + + t.is(onOpenCalled, 2); + t.is(onMessageCalled, 2); + t.is(onErrorCalled, 2); + + mockServer.close(); +}); From ba3f78e9ad3de1c2f58bf6253ac96f9393364bff Mon Sep 17 00:00:00 2001 From: ritave Date: Fri, 3 Jun 2022 14:26:01 +0200 Subject: [PATCH 2/2] Changes after code review --- .eslintrc | 3 +- src/websocket.js | 64 +++++++++++++--------------------------- tests/issues/242.test.js | 1 + 3 files changed, 23 insertions(+), 45 deletions(-) diff --git a/.eslintrc b/.eslintrc index e4db4868..a2fc9ca5 100644 --- a/.eslintrc +++ b/.eslintrc @@ -13,6 +13,7 @@ "no-param-reassign": 0, "comma-dangle": ["error", "never"], "arrow-parens": 0, - "import/no-extraneous-dependencies": 0 // all deps are resolved with "node-resolve" + "import/no-extraneous-dependencies": 0, // all deps are resolved with "node-resolve" + "no-underscore-dangle": ["error", { "allowAfterThis": true }] } } diff --git a/src/websocket.js b/src/websocket.js index 5b8df302..112193c9 100644 --- a/src/websocket.js +++ b/src/websocket.js @@ -21,10 +21,10 @@ class WebSocket extends EventTarget { constructor(url, protocols) { super(); - this.onopenCb = null; - this.onmessageCb = null; - this.onerrorCb = null; - this.oncloseCb = null; + this._onopen = null; + this._onmessage = null; + this._onerror = null; + this._onclose = null; this.url = urlVerification(url); protocols = protocolVerification(protocols); @@ -99,67 +99,43 @@ class WebSocket extends EventTarget { } get onopen() { - return this.onopenCb; + return this._onopen; } get onmessage() { - return this.onmessageCb; + return this._onmessage; } get onclose() { - return this.oncloseCb; + return this._onclose; } get onerror() { - return this.onerrorCb; + return this._onerror; } set onopen(listener) { - if (listener === null) { - if (this.onopenCb !== null) { - this.removeEventListener('open', this.onopenCb); - } - } else { - this.onopen = null; - this.addEventListener('open', listener); - } - this.onopenCb = listener; + this.removeEventListener('open', this._onopen); + this._onopen = listener; + this.addEventListener('open', listener); } set onmessage(listener) { - if (listener === null) { - if (this.onmessageCb !== null) { - this.removeEventListener('message', this.onmessageCb); - } - } else { - this.onmessage = null; - this.addEventListener('message', listener); - } - this.onmessageCb = listener; + this.removeEventListener('message', this._onmessage); + this._onmessage = listener; + this.addEventListener('message', listener); } set onclose(listener) { - if (listener === null) { - if (this.oncloseCb !== null) { - this.removeEventListener('close', this.oncloseCb); - } - } else { - this.onclose = null; - this.addEventListener('close', listener); - } - this.oncloseCb = listener; + this.removeEventListener('close', this._onclose); + this._onclose = listener; + this.addEventListener('close', listener); } set onerror(listener) { - if (listener === null) { - if (this.onerrorCb !== null) { - this.removeEventListener('error', this.onerrorCb); - } - } else { - this.onerror = null; - this.addEventListener('error', listener); - } - this.onerrorCb = listener; + this.removeEventListener('error', this._onerror); + this._onerror = listener; + this.addEventListener('error', listener); } send(data) { diff --git a/tests/issues/242.test.js b/tests/issues/242.test.js index 2ce044ed..647a706c 100644 --- a/tests/issues/242.test.js +++ b/tests/issues/242.test.js @@ -1,6 +1,7 @@ import test from 'ava'; import Server from '../../src/server'; import WebSocket from '../../src/websocket'; +import Event from '../../src/event/event'; test('websocket on* methods family returns a single listener', t => { const socketUrl = 'ws://localhost:8080';