Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

pimterry
Copy link
Contributor

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).

This notably makes it possible to create a WebSocket over _any_ duplex
stream, allowing use of WS in all sorts of other weird environments,
eventually including the use of WebSockets over HTTP/2 streams.
@lpinca
Copy link
Member

lpinca commented Apr 12, 2024

See #1944.

@pimterry
Copy link
Contributor Author

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
Copy link
Member

lpinca commented Apr 15, 2024

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

@lpinca lpinca merged commit 53a8888 into websockets:master Apr 15, 2024
56 checks passed
@pimterry
Copy link
Contributor Author

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 👍

@lpinca
Copy link
Member

lpinca commented Apr 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants