search

websockets / ws

Simple to use, blazing fast and thoroughly tested WebSocket client and server for Node.js
MIT License
21.33k stars 2.3k forks source link

[feature] Add createConnection option to control client socket setup #2219

Closed pimterry closed 2 months ago

pimterry commented 2 months ago

This adds support for a new createConnection option on client websockets, with the exact same functionality as the same option on http.request etc on Node.

The option is just passed straight through to http(s).request, so supports exactly the same functionality, including returning any Duplex stream as a connection, not just net sockets.

This is useful in quite a few cases. In my specific case, I need to create an outgoing WebSocket over an existing duplex stream, and this makes that possible with createConnection: () => stream. I've tested this change in my codebase and it seems to be able to do that perfectly.

Notably though, this also makes it possible to potentially create WebSockets over things like HTTP/2 streams (see #1458). It probably doesn't fully provide that, and that's not something I'm worrying about much myself right now, but it's a step in the right direction, and this would get rid of the fragile & confusing workarounds linked in that issue (https://github.com/szmarczak/http2-wrapper/blob/master/examples/ws/index.js).

lpinca commented 2 months ago

See https://github.com/websockets/ws/issues/1944.

pimterry commented 2 months ago

Ah, interesting! I hadn't seen the agent workaround, that is probably a usable workaround for me.

Given that 3x people (#1944, this PR and #2088) have now all independently suggested adding this option though (and others like the HTTP/2 example above seem to have done worse things to workaround not being obviously able to do this) maybe it's worth including? Otherwise I'm sure others make the same suggestion in future, and it's a very tiny change to pave the path. Personally I was looking for this because I use the Node HTTP API for this very frequently, while I'm not sure I've ever written a custom agent.

Up to you though! In the meantime the agent version should be manageable for me even if you don't want to merge this.

lpinca commented 2 months ago

I don't remember why but not allowing a custom options.createConnection was intentional.

pimterry commented 2 months ago

I guess you worked out why, and it's actually OK now? If there are any issues with this, do let me know, I'd definitely be interested to hear about that.

Assuming not though, thanks for merging this! That'll be very helpful. I did test the agent workaround in my code, and it does indeed work fine, but it's notably less convenient (doesn't work with Node TS types by default, requires an import of node:http that is awkward in browser+node code, generally messier code required).

For my case at least, this route is very nice and easy :+1:

lpinca commented 2 months ago

I guess you worked out why, and it's actually OK now? If there are any issues with this, do let me know, I'd definitely be interested to hear about that.

No, I can't remember, but hopefully it was something that is no longer needed.