-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Conversation
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.
See #1944. |
Ah, interesting! I hadn't seen the 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. |
I don't remember why but not allowing a custom |
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 For my case at least, this route is very nice and easy 👍 |
No, I can't remember, but hopefully it was something that is no longer needed. |
This adds support for a new
createConnection
option on client websockets, with the exact same functionality as the same option onhttp.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).