-
Notifications
You must be signed in to change notification settings - Fork 5.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
feat(runtime/http): websocket support #10359
Conversation
# Conflicts: # op_crates/websocket/lib.rs
runtime/js/40_http.js
Outdated
|
||
return { request, respondWith }; | ||
return { request, respondWith, upgradeWebsocket }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use upgrade
instead of upgradeWebsocket
? a simple API can get better dev experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, upgrading isn't something exclusive to websockets. that would end up being misleading and confusing.
9 more characters in a function name used in a single place wont affect dev experience.
Of course that would make sense if we would support upgrading other kind of conns out of the box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course that would make sense if we would support upgrading other kind of conns out of the box
Other hypothetical upgrades would have different return types, so they should just have individual names anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this actually working? Would be very nice to have a test in cli/tests/unit/http_test.ts if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs regression tests but LGTM apart from that, thanks!
@crowlKats I'm a bit confused about how this depends on #10365... I haven't dived into this PR but I'm not sure why we would need to expose WebSocketStream in order to hook up the hyper-tungstenite bindings? I don't care so much about WebSocketStream support - since it's still a tentative API - but I'm rather keen to land this PR. |
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
# Conflicts: # extensions/websocket/lib.rs # runtime/ops/http.rs # runtime/ops/websocket.rs
Co-authored-by: Nayeem Rahman <nayeemrmn99@gmail.com>
Blocked on hyperium/hyper#2587 |
# Conflicts: # Cargo.lock # Cargo.toml # cli/dts/lib.deno.unstable.d.ts # extensions/net/03_http.js # extensions/net/ops_http.rs # runtime/Cargo.toml
# Conflicts: # Cargo.lock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for this great feature!
We need to add some documentation for this to https://deno.land/manual@v1.11.5/runtime/http_server_apis in a follow up commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh actually upgradeWebsocket
is still async. That needs to become synchronous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now. Thanks!
Closes #10211