Extract handshake generation & validation into composable replaceable parts#2313
Extract handshake generation & validation into composable replaceable parts#2313pimterry wants to merge 1 commit intowebsockets:masterfrom
Conversation
| WebSocket.HandshakeRequest = require('./lib/handshake-request'); | ||
| WebSocket.HandshakeValidator = require('./lib/handshake-validator'); |
There was a problem hiding this comment.
I've only looked at this and no, no more spurious keys to the exported WebSocket.
|
I understand the concept but I don't like the code at all.
I think that if this is the alternative I prefer to add options. |
|
I think my digging into this has mostly convinced me that my use case needs something like this approach. Medium-term I'm going to want to make significant changes to the handshake behaviour details (custom subprotocol validation yes, but also direct key control, raw headers instead of object headers, lots of groundwork for WS over HTTP/2 and 3, and I suspect plenty of other bits that won't immediately make sense here). At the same time, I still want to use reuse all the existing WS logic for frame parsing & generation etc - I'm really just messing with the handshake before the stream is set up. Big handshake changes mean being able to replace quite large chunks of the implementation completely with custom code though, which isn't currently possible. Options only offer a limited alternative, and much more hassle for you to support now and ongoing. I think the "90% of that stuff should not be customizable at all" is the key point here - in my experience, there are use cases where you really do want to customize crazy things - WS powers https://testserver.host/ for example, which acts as a public server intentionally simulating invalid websocket behaviour for testing cases (for example, it does have an How about a different approach: what if it was possible to extract the handshake outside WS completely? So I can do the upgrade etc totally manually, and then create a client websocket directly from a stream, ready to start reading & writing frames. That would reduce the API surface of the resulting change significantly, and make the interactions much clearer. It'd require a bit more work on the handshake-overriding side, but that's fine. If you think you'd be open to that approach let me know and I can put together a demo so we can see the tradeoffs and discuss more specifically. |
It is already possible. const ws = WebSocket(null, undefined, options);
ws._isServer = false;
ws.setSocket(duplex, Buffer.alloc(0), options); |
|
Ah, neat! Yes, something like that would work great and seems much simpler. Looks like setSocket and _isServer are undocumented private APIs though. At a quick skim, it also looks like that has some other small issues like not setting _closeTimeout, but otherwise it's extremely close. Would you be happy to expose & document that as a public API? I think that would cover my use cases nicely. |
|
I would prefer not to document them but they are not going away (at least not in a minor or patch release). |
|
Ok, understood. I'm going to migrate to using that API instead. I would prefer it to be documented and managed like the rest of the public API. It sounds like you're trying to discourage usage of it, but I suspect that will just send users in the wrong (worse) directions like forking the library or adding more options - if I had seen that this was possible in advance, I wouldn't have explored these more complicated approaches like we've discussed. Anyway, I'll leave that up to you. For now I'm happy to build on those informal support guarantees, and this approach solves my issues nicely, so I'll close the PR. Thanks for your help 😃 |
|
Improvements to make the API less cluttered are welcome, but yes, using these properties is not something the common user should do. Most of them should be read only. Another reason for having them "private" is that they do not exist in the WHATWG spec (yes again: the entire public API was modeled after it) or they behave differently. I think the biggest pain in your case (replacing the handshake) is dealing with the permessage-deflate extension, but it should be doable. |
From a quick look it seems that it is currently not possible to use the |
Detailsdiff --git a/index.js b/index.js
index 41edb3b..d03bd88 100644
--- a/index.js
+++ b/index.js
@@ -1,13 +1,20 @@
'use strict';
+const createWebSocketStream = require('./lib/stream');
+const PerMessageDeflate = require('./lib/permessage-deflate');
+const Receiver = require('./lib/receiver');
+const Sender = require('./lib/sender');
const WebSocket = require('./lib/websocket');
+const WebSocketServer = require('./lib/websocket-server');
+const extension = require('./lib/extension');
-WebSocket.createWebSocketStream = require('./lib/stream');
-WebSocket.Server = require('./lib/websocket-server');
-WebSocket.Receiver = require('./lib/receiver');
-WebSocket.Sender = require('./lib/sender');
-
+WebSocket.createWebSocketStream = createWebSocketStream;
+WebSocket.extension = extension;
+WebSocket.PerMessageDeflate = PerMessageDeflate;
+WebSocket.Receiver = Receiver;
+WebSocket.Sender = Sender;
+WebSocket.Server = WebSocketServer;
WebSocket.WebSocket = WebSocket;
-WebSocket.WebSocketServer = WebSocket.Server;
+WebSocket.WebSocketServer = WebSocketServer;
module.exports = WebSocket;
diff --git a/wrapper.mjs b/wrapper.mjs
index 7245ad1..fd3be48 100644
--- a/wrapper.mjs
+++ b/wrapper.mjs
@@ -1,8 +1,19 @@
import createWebSocketStream from './lib/stream.js';
+import PerMessageDeflate from './lib/permessage-deflate.js';
import Receiver from './lib/receiver.js';
import Sender from './lib/sender.js';
import WebSocket from './lib/websocket.js';
import WebSocketServer from './lib/websocket-server.js';
+import extension from './lib/extension.js';
+
+export {
+ createWebSocketStream,
+ extension,
+ PerMessageDeflate,
+ Receiver,
+ Sender,
+ WebSocket,
+ WebSocketServer
+};
-export { createWebSocketStream, Receiver, Sender, WebSocket, WebSocketServer };
export default WebSocket;
Do you have better ideas for
|
|
Perhaps something like this Detailsdiff --git a/lib/index.js b/lib/index.js
new file mode 100644
index 0000000..70c6531
--- /dev/null
+++ b/lib/index.js
@@ -0,0 +1,17 @@
+'use strict';
+
+const createWebSocketStream = require('./stream');
+const extension = require('./extension');
+const PerMessageDeflate = require('./permessage-deflate');
+const Receiver = require('./receiver');
+const Sender = require('./sender');
+const subprotocol = require('./subprotocol');
+
+module.exports = {
+ createWebSocketStream,
+ extension,
+ PerMessageDeflate,
+ Receiver,
+ Sender,
+ subprotocol
+};
diff --git a/lib/wrapper.mjs b/lib/wrapper.mjs
new file mode 100644
index 0000000..1347978
--- /dev/null
+++ b/lib/wrapper.mjs
@@ -0,0 +1,15 @@
+import createWebSocketStream from './stream.js';
+import extension from './extension.js';
+import PerMessageDeflate from './permessage-deflate.js';
+import Receiver from './receiver.js';
+import Sender from './sender.js';
+import subprotocol from './subprotocol.js';
+
+export {
+ createWebSocketStream,
+ extension,
+ PerMessageDeflate,
+ Receiver,
+ Sender,
+ subprotocol
+};
diff --git a/package.json b/package.json
index 5428144..79d5c19 100644
--- a/package.json
+++ b/package.json
@@ -25,6 +25,10 @@
"import": "./wrapper.mjs",
"require": "./index.js"
},
+ "./lib": {
+ "import": "./lib/wrapper.mjs",
+ "require": "./lib/index.js"
+ },
"./package.json": "./package.json"
},
"browser": "browser.js",
@@ -35,6 +39,7 @@
"browser.js",
"index.js",
"lib/*.js",
+ "lib/wrapper.mjs",
"wrapper.mjs"
],
"scripts": {
|
|
Yes, I've just run into that exact issue. Otherwise it all seems to be working OK. Exposing a I expect eventually for a future v9 breaking change it'd be best to have just one entrypoint exposing everything, but if that means either ESM-only or changing the CJS export to a named |
|
@pimterry what do think about this? diff --git a/index.js b/index.js
index 41edb3b..280093a 100644
--- a/index.js
+++ b/index.js
@@ -1,13 +1,40 @@
'use strict';
+const createWebSocketStream = require('./lib/stream');
+const extension = require('./lib/extension');
+const PerMessageDeflate = require('./lib/permessage-deflate');
+const Receiver = require('./lib/receiver');
+const Sender = require('./lib/sender');
+const subprotocol = require('./lib/subprotocol');
const WebSocket = require('./lib/websocket');
+const WebSocketServer = require('./lib/websocket-server');
-WebSocket.createWebSocketStream = require('./lib/stream');
-WebSocket.Server = require('./lib/websocket-server');
-WebSocket.Receiver = require('./lib/receiver');
-WebSocket.Sender = require('./lib/sender');
+const map = {
+ createWebSocketStream,
+ extension,
+ PerMessageDeflate,
+ Receiver,
+ Sender,
+ subprotocol,
+ Server: WebSocketServer,
+ WebSocket,
+ WebSocketServer,
+ __proto__: null
+};
-WebSocket.WebSocket = WebSocket;
-WebSocket.WebSocketServer = WebSocket.Server;
+const proxy = new Proxy(WebSocket, {
+ get(target, property, receiver) {
+ const value = map[property];
-module.exports = WebSocket;
+ if (
+ value !== undefined &&
+ !Object.prototype.hasOwnProperty.call(target, property)
+ ) {
+ return property === 'WebSocket' ? proxy : value;
+ }
+
+ return Reflect.get(target, property, receiver);
+ }
+});
+
+module.exports = proxy;
diff --git a/wrapper.mjs b/wrapper.mjs
index 7245ad1..dd14e89 100644
--- a/wrapper.mjs
+++ b/wrapper.mjs
@@ -1,8 +1,21 @@
import createWebSocketStream from './lib/stream.js';
+import extension from './lib/extension.js';
+import PerMessageDeflate from './lib/permessage-deflate.js';
import Receiver from './lib/receiver.js';
import Sender from './lib/sender.js';
-import WebSocket from './lib/websocket.js';
+import subprotocol from './lib/subprotocol.js';
+import WebSocket from './index.js';
import WebSocketServer from './lib/websocket-server.js';
-export { createWebSocketStream, Receiver, Sender, WebSocket, WebSocketServer };
+export {
+ createWebSocketStream,
+ extension,
+ PerMessageDeflate,
+ Receiver,
+ Sender,
+ subprotocol,
+ WebSocket,
+ WebSocketServer
+};
+
export default WebSocket;
|
|
Hmm, what's the practical goal here? I see this avoids literally attaching new properties to the WebSocket constructor, but the proxy emulates all of them instead, so you get the same API. That seems like very nearly the same result, and the benefits of the extra proxy logic are quite subtle... In most cases I suspect downstream usage will largely end up going through this proxy, so it won't make any difference there. Totally agree it's a bit messy having the properties on the class directly, tidying up seems mildly nice if we can, but I'm not sure it's worth the extra complexity here for a whole proxy layer that doesn't change much visibly. Am I missing some practical benefits of this? Does seem like it'd work fine of course though, so if that's worthwhile to you then fair enough, no strong objections from my side certainly. |
|
The goal is to not add spurious properties to the |
|
Personally, I don't think it's worth it. This is neat but I don't think it'd change anything significantly and it confuses & complicates things a bit. I'd stick with the current approach even if it's a bit messy, and aim towards a proper breaking cleanup for an eventual v9 where everything will be a separate named export. |
Export the `PerMessageDeflate` class, the parser and serializer for the `Sec-WebSocket-Extensions` header, and the parser for the `Sec-WebSocket-Protocol` header. Documentation is intentionally omitted as these utilities are primarily intended for niche use cases rather than general consumption. Refs: #2313
Here's an alternative approach to our discussion from #1862: how about instead of endlessly adding more handshake options, the core handshake steps were separated into composable parts, and you could just provide your own implementation instead, while still using all the rest of ws as normal.
This PR is a proof-of-concept for the idea.
It passes all the existing tests and preserves backward compatibility (it's the same logic, just extracted) but you can now freely implement whatever crazy things you want to do in the handshake - custom subprotocol validation (#1862), set custom websocket keys (#2048), use flat header arrays (another extension on my list, for raw header control), and generally fully control all headers & request options directly. All while preserving sensible defaults, and making it clear that you're taking responsibility for your part of the handshake if you do this (because you have to replace the implementation entirely - although in practice I imagine most cases will be subclasses or tiny forks of the existing implementation).
It also makes
initAsClientquite a bit simpler, and makes all the existing logic easier to test in isolation.This does add two options, but hopefully removes all future option discussion around handshakes. You could plausibly make the Sender & Receiver classes replaceable if you want to generally solve the 'many options' problem for post-handshake as well. With that ws becomes more like a websocket toolkit, providing default implementations of each part of the protocol that work together correctly, but which you can modify & swap out, without needing ws to explicitly support every possible behaviour.
If this were merged, in a later breaking change you could potentially remove the
finishRequestoption, since this basically replaces that with a more general solution. I'd suggest also dropping theprotocolVersionoption as well - I suspect it's very rarely used, and can easily be reimplemented through this API by anybody who needs it (and that would simplify the behaviour oforiginen route). Arguably this also replaces the'upgrade'option too - if you want to see the response headers, you can now do so with a custom handshake validator.Code itself mostly written by claude, but I've done a good bit of review & iteration myself and I think it looks like a reasonable start - this is primarily a base for discussion though, happy to change the details of the approach if you're on board with the concept.