-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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(http): return WebSocketStream from upgradeWebSocket #16732
base: main
Are you sure you want to change the base?
Conversation
The WSS accessor needs to be locked behind --unstable. |
@lucacasonato done |
cli/dts/lib.deno.unstable.d.ts
Outdated
* request. | ||
* | ||
* @category Web Sockets */ | ||
export interface WebSocketUpgrade { |
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.
It seems this is not used anywhere?
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.
it overloads the same named interface as in lib.deno.ns.d.ts
Would it be possible to test this API against this issue? #17332 In Deno 1.29.2 |
# Conflicts: # runtime/ops/runtime.rs
# Conflicts: # cli/tsc/dts/lib.deno.unstable.d.ts # ext/flash/01_http.js # ext/http/01_http.js # ext/websocket/02_websocketstream.js
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.
Discussed offline, documentation should be updated to reflect that one can use either socket
or stream
.
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.
@crowlKats looks good, but I'd like to see a test that verified that either socket
or stream
can be used
@bartlomieju there is a test for that already |
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!
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 rebase.
# Conflicts: # ext/flash/01_http.js # ext/http/01_http.js # ext/websocket/02_websocketstream.js # runtime/ops/runtime.rs
Signed-off-by: Aapo Alasuutari <aapo.alasuutari@gmail.com>
Closes #14064