From dcb85e902d129b2d1a94943b4f6d471532f70dc9 Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Thu, 29 Apr 2021 11:49:17 +0200 Subject: [PATCH 1/3] feat: add the "closeOnBeforeunload" option Since [1], the socket is now closed when receiving the "beforeunload" event in the browser. This change was meant to fix a discrepancy between Chrome and Firefox when the user reloads/closes a browser tab: Firefox would close the connection (and emit a "disconnect" event, at the Socket.IO level), but not Chrome (see [2]). But it also closes the connection when there is another "beforeunload" handler, for example when the user is prompted "are you sure you want to leave this page?". Note: calling "stopImmediatePropagation()" was a possible workaround: ```js window.addEventListener('beforeunload', (event) => { event.preventDefault(); event.stopImmediatePropagation(); event.returnValue = 'are you sure you want to leave this page?'; }); ``` This commit adds a "closeOnBeforeunload" option, which controls whether a handler is registered for the "beforeunload" event. Syntax: ```js const socket = require('engine.io-client')('ws://localhost', { closeOnBeforeunload: false // defaults to true }); ``` [1]: https://github.com/socketio/engine.io-client/commit/ed48b5dc3407e5ded45072606b3bb0eafa49c01f [2]: https://github.com/socketio/socket.io/issues/3639 Related: - https://github.com/socketio/engine.io-client/issues/661 - https://github.com/socketio/engine.io-client/issues/658 - https://github.com/socketio/socket.io-client/issues/1451 Reference: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event --- README.md | 1 + lib/socket.js | 30 ++++++++++++++++++------------ 2 files changed, 19 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index c85dc87e4..247439328 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,7 @@ Exposed as `eio` in the browser standalone build. - `requestTimeout` (`Number`): Timeout for xhr-polling requests in milliseconds (`0`) - **Websocket-only options** - `protocols` (`Array`): a list of subprotocols (see [MDN reference](https://developer.mozilla.org/en-US/docs/Web/API/WebSockets_API/Writing_WebSocket_servers#Subprotocols)) + - `closeOnBeforeunload` (`Boolean`): whether to silently close the connection when the [`beforeunload`](https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event) event is emitted in the browser (defaults to `true`) - `send` - Sends a message to the server - **Parameters** diff --git a/lib/socket.js b/lib/socket.js index 7f939b31e..996a8c611 100644 --- a/lib/socket.js +++ b/lib/socket.js @@ -70,7 +70,8 @@ class Socket extends Emitter { perMessageDeflate: { threshold: 1024 }, - transportOptions: {} + transportOptions: {}, + closeOnBeforeunload: true }, opts ); @@ -91,17 +92,22 @@ class Socket extends Emitter { this.pingTimeoutTimer = null; if (typeof addEventListener === "function") { - addEventListener( - "beforeunload", - () => { - if (this.transport) { - // silently close the transport - this.transport.removeAllListeners(); - this.transport.close(); - } - }, - false - ); + if (this.opts.closeOnBeforeunload) { + // Firefox closes the connection when the "beforeunload" event is emitted but not Chrome. This event listener + // ensures every browser behaves the same (no "disconnect" event at the Socket.IO level when the page is + // closed/reloaded) + addEventListener( + "beforeunload", + () => { + if (this.transport) { + // silently close the transport + this.transport.removeAllListeners(); + this.transport.close(); + } + }, + false + ); + } if (this.hostname !== "localhost") { this.offlineEventListener = () => { this.onClose("transport close"); From c46611ce44897ef27f111f34151455d57193588b Mon Sep 17 00:00:00 2001 From: Damien Arrachequesne Date: Tue, 4 May 2021 09:28:21 +0200 Subject: [PATCH 2/3] refactor: remove "self" references --- lib/socket.js | 111 ++++++++++++++------------------ lib/transports/polling-jsonp.js | 33 ++++------ lib/transports/polling-xhr.js | 35 +++++----- lib/transports/polling.js | 27 ++++---- lib/transports/websocket.js | 89 ++++++++++++------------- 5 files changed, 127 insertions(+), 168 deletions(-) diff --git a/lib/socket.js b/lib/socket.js index 996a8c611..91346b970 100644 --- a/lib/socket.js +++ b/lib/socket.js @@ -172,9 +172,8 @@ class Socket extends Emitter { transport = "websocket"; } else if (0 === this.transports.length) { // Emit error on next tick so it can be listened to - const self = this; - setTimeout(function() { - self.emit("error", "No transports available"); + setTimeout(() => { + this.emit("error", "No transports available"); }, 0); return; } else { @@ -203,7 +202,6 @@ class Socket extends Emitter { */ setTransport(transport) { debug("setting transport %s", transport.name); - const self = this; if (this.transport) { debug("clearing existing transport %s", this.transport.name); @@ -215,17 +213,11 @@ class Socket extends Emitter { // set up transport listeners transport - .on("drain", function() { - self.onDrain(); - }) - .on("packet", function(packet) { - self.onPacket(packet); - }) - .on("error", function(e) { - self.onError(e); - }) - .on("close", function() { - self.onClose("transport close"); + .on("drain", this.onDrain.bind(this)) + .on("packet", this.onPacket.bind(this)) + .on("error", this.onError.bind(this)) + .on("close", () => { + this.onClose("transport close"); }); } @@ -239,52 +231,46 @@ class Socket extends Emitter { debug('probing transport "%s"', name); let transport = this.createTransport(name, { probe: 1 }); let failed = false; - const self = this; Socket.priorWebsocketSuccess = false; - function onTransportOpen() { - if (self.onlyBinaryUpgrades) { - const upgradeLosesBinary = - !this.supportsBinary && self.transport.supportsBinary; - failed = failed || upgradeLosesBinary; - } + const onTransportOpen = () => { if (failed) return; debug('probe transport "%s" opened', name); transport.send([{ type: "ping", data: "probe" }]); - transport.once("packet", function(msg) { + transport.once("packet", msg => { if (failed) return; if ("pong" === msg.type && "probe" === msg.data) { debug('probe transport "%s" pong', name); - self.upgrading = true; - self.emit("upgrading", transport); + this.upgrading = true; + this.emit("upgrading", transport); if (!transport) return; Socket.priorWebsocketSuccess = "websocket" === transport.name; - debug('pausing current transport "%s"', self.transport.name); - self.transport.pause(function() { + debug('pausing current transport "%s"', this.transport.name); + this.transport.pause(() => { if (failed) return; - if ("closed" === self.readyState) return; + if ("closed" === this.readyState) return; debug("changing transport and sending upgrade packet"); cleanup(); - self.setTransport(transport); + this.setTransport(transport); transport.send([{ type: "upgrade" }]); - self.emit("upgrade", transport); + this.emit("upgrade", transport); transport = null; - self.upgrading = false; - self.flush(); + this.upgrading = false; + this.flush(); }); } else { debug('probe transport "%s" failed', name); const err = new Error("probe error"); err.transport = transport.name; - self.emit("upgradeError", err); + this.emit("upgradeError", err); } }); - } + }; function freezeTransport() { if (failed) return; @@ -299,7 +285,7 @@ class Socket extends Emitter { } // Handle any error that happens while probing - function onerror(err) { + const onerror = err => { const error = new Error("probe error: " + err); error.transport = transport.name; @@ -307,8 +293,8 @@ class Socket extends Emitter { debug('probe transport "%s" failed because of error: %s', name, err); - self.emit("upgradeError", error); - } + this.emit("upgradeError", error); + }; function onTransportClose() { onerror("transport closed"); @@ -328,13 +314,13 @@ class Socket extends Emitter { } // Remove all listeners on the transport and on self - function cleanup() { + const cleanup = () => { transport.removeListener("open", onTransportOpen); transport.removeListener("error", onerror); transport.removeListener("close", onTransportClose); - self.removeListener("close", onclose); - self.removeListener("upgrading", onupgrade); - } + this.removeListener("close", onclose); + this.removeListener("upgrading", onupgrade); + }; transport.once("open", onTransportOpen); transport.once("error", onerror); @@ -557,13 +543,29 @@ class Socket extends Emitter { * @api private */ close() { - const self = this; + const close = () => { + this.onClose("forced close"); + debug("socket closing - telling transport to close"); + this.transport.close(); + }; + + const cleanupAndClose = () => { + this.removeListener("upgrade", cleanupAndClose); + this.removeListener("upgradeError", cleanupAndClose); + close(); + }; + + const waitForUpgrade = () => { + // wait for upgrade to finish since we can't send packets while pausing a transport + this.once("upgrade", cleanupAndClose); + this.once("upgradeError", cleanupAndClose); + }; if ("opening" === this.readyState || "open" === this.readyState) { this.readyState = "closing"; if (this.writeBuffer.length) { - this.once("drain", function() { + this.once("drain", () => { if (this.upgrading) { waitForUpgrade(); } else { @@ -577,24 +579,6 @@ class Socket extends Emitter { } } - function close() { - self.onClose("forced close"); - debug("socket closing - telling transport to close"); - self.transport.close(); - } - - function cleanupAndClose() { - self.removeListener("upgrade", cleanupAndClose); - self.removeListener("upgradeError", cleanupAndClose); - close(); - } - - function waitForUpgrade() { - // wait for upgrade to finish since we can't send packets while pausing a transport - self.once("upgrade", cleanupAndClose); - self.once("upgradeError", cleanupAndClose); - } - return this; } @@ -622,7 +606,6 @@ class Socket extends Emitter { "closing" === this.readyState ) { debug('socket close with reason: "%s"', reason); - const self = this; // clear timers clearTimeout(this.pingIntervalTimer); @@ -652,8 +635,8 @@ class Socket extends Emitter { // clean buffers after, so users can still // grab the buffers on `close` event - self.writeBuffer = []; - self.prevBufferLen = 0; + this.writeBuffer = []; + this.prevBufferLen = 0; } } diff --git a/lib/transports/polling-jsonp.js b/lib/transports/polling-jsonp.js index 3674dfbb8..a739d5458 100644 --- a/lib/transports/polling-jsonp.js +++ b/lib/transports/polling-jsonp.js @@ -33,10 +33,7 @@ class JSONPPolling extends Polling { this.index = callbacks.length; // add callback to jsonp global - const self = this; - callbacks.push(function(msg) { - self.onData(msg); - }); + callbacks.push(this.onData.bind(this)); // append to query string this.query.j = this.index; @@ -77,7 +74,6 @@ class JSONPPolling extends Polling { * @api private */ doPoll() { - const self = this; const script = document.createElement("script"); if (this.script) { @@ -87,8 +83,8 @@ class JSONPPolling extends Polling { script.async = true; script.src = this.uri(); - script.onerror = function(e) { - self.onError("jsonp poll error", e); + script.onerror = e => { + this.onError("jsonp poll error", e); }; const insertAt = document.getElementsByTagName("script")[0]; @@ -119,7 +115,6 @@ class JSONPPolling extends Polling { * @api private */ doWrite(data, fn) { - const self = this; let iframe; if (!this.form) { @@ -149,30 +144,30 @@ class JSONPPolling extends Polling { fn(); } - function initIframe() { - if (self.iframe) { + const initIframe = () => { + if (this.iframe) { try { - self.form.removeChild(self.iframe); + this.form.removeChild(this.iframe); } catch (e) { - self.onError("jsonp polling iframe removal error", e); + this.onError("jsonp polling iframe removal error", e); } } try { // ie6 dynamic iframes with target="" support (thanks Chris Lambacher) - const html = '