- Notifications
You must be signed in to change notification settings - Fork18
The WebSocketStream Interface#48
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Lamzin left a comment
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.
Hi Adam,
Thanks for the PR! Looks good in general. However, I left a few comments.
Thanks,
Oleh
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
ricea commentedJan 28, 2024
The algorithms are complete now. @nidhijaju@domenic PTAL The PR Preview is unhappy for some reason, but it builds locally. |
Temporarily force-link "writable stream writer" until it can be exportedfrom the streams standard.
This feature is not worth the complexity so it is being removed.
…cancel()"Put back mention of passing close code or reason to abort() or cancel(),in preparation for updating it to use WebSocketError.
Some algorithms that are common to WebSocket and WebSocketStream havebeen moved to a new section to be shared.
dead-claudia commentedJan 29, 2024
Quick question: wouldn't it be better to model it this way?
WebIDL: dictionary WebSocketStreamOptions { sequence<USVString> protocols; AbortSignalsignal;};interfaceWebSocketStream {constructor(USVString url, optional WebSocketStreamOptions options); readonly attribute ReadableStream readable; readonly attribute WritableStream writable; readonly attribute sequence<DOMString> extensions; readonly attribute DOMString protocol; readonly attribute Promise<undefined> opened; readonly attribute Promise<undefined> closed; [Clamp] attributeunsignedshort closeCode; attribute USVString closeReason;};This has a few other added benefits as well:
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
domenic left a comment
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.
Only got a little ways through before I had to wrap up for the day.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Also apply backpressure on connection, and linkify "exists".
ricea commentedJan 30, 2024
I think always throwing an exception on close would lead to poor ergonomics. For example, even a simple function like constwss=newWebSocketStream('/messages');forawait(constmessageofwss.readable){constdiv=newHTMLDivElement();div.textContent=message;body.appendChild(div);} would have to be wrapped in a
This is what WebTransport does with datagrams, but it adds a lot of complexity to the standard and implementation. In the case of WebSockets, no messages can be sent until the handshake response has been received anyway, so there's no performance benefit in queuing them early. I consciously chose not to give out the streams until they were useful to avoid the need to think about these issues.
This means you have to pass around the writer to any place that needs to be able to close the WebSocket. I have the impression that most users of WebSockets don't bother with the close codes but for those who do it feels awkward: wss.closeCode=1000;wss.closeReason='Thank you';writer.close();
I'm not comfortable that they can be changed asynchronously outside of script:
In the case of HTTP, it's necessary to send the request body first, so it's not an exact parallel.
It's a bit easier to implement, yes, but in my opinion it's harder to use. Developers won't have to think about the various dictionaries in my design, they can just copy the calling patterns from examples. Implementing the construction of a
You can actually pass a
I'm not sure of the usefulness of the URL field, but it's very easy to implement, and maybe if it saves someone from needing to use a |
domenic left a comment
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.
Didn't yet get to the last two sections, but it's all looking quite nice so far; no substantial comments.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
domenic left a comment
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 with nits, nice work!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
Co-authored-by: Domenic Denicola <d@domenic.me>
Don't nest a link inside another link.
jhugard commentedMar 22, 2024
Any thoughts on taking this as an opportunity to facilitate adding custom HTTP UPGRADE request headers? For example, web clients currently must provide any auth token as a query parameter on the URL. This conflicts with security best practice, which recommend using an Authorization header to avoid accidental logging of security related info. |
ricea commentedMar 25, 2024
I'm not going to add it in the initial PR. I've considered adding it initially for WebSocketStream so we can experiment with a smaller and more adaptable user base. But we'd have to add it to the old WebSocket API too eventually. This is getting off-topic for this PR so please take future discussion to#16 (or don't -- that issue has already reached a length where most people won't read the whole thing). |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
WebIDL requires explicit defaults for optional arguments.Co-authored-by: Philip Jägenstedt <philip@foolip.org>
dontcallmedom-bot commentedMay 22, 2024
This issue was mentioned inWEBRTCWG-2024-05-21 (Page 38) |
lucacasonato commentedSep 16, 2024
Chrome has shipped this since 124 in stable. Deno is interested in shipping too. Is there any interest in this API from Gecko or WebKit? cc@annevk and@saschanaz |
saschanaz commentedSep 16, 2024
This interface is quite similar to WebTransport but still is different in a few ways, I wonder it could be nicer if it was more similar so that potential migration to webtransport would be easier for devs? (e.g. have createBidirectionalStream to get the stream, even though there will only be a single pair of streams. Too hacky maybe? Also kinda interesting that WT somehow doesn't have the url attribute.) The name is misleading btw, WebSocketStream is not really a stream but an interface to eventually get one. The official position will be announced inmozilla/standards-positions#970. |
annevk commentedSep 17, 2024
dead-claudia commentedSep 17, 2024
Okay, I revisited#48 (comment), addressed the feedback, and came up with a replacement that also uses streams a bit better.
Here's the WebIDL: dictionary WebSocketStreamOptions { sequence<USVString> protocols = []; AbortSignalsignal;};[Exposed=(Window,Worker)]interfaceWebSocketReadableStream : ReadableStream { readonly attribute Promise<void> negotiated; readonly attribute USVString protocol; readonly attribute USVString extensions; readonly attributeshort closeCode; readonly attribute USVString closeReason;};[Exposed=(Window,Worker)]interfaceWebSocketWritableStream : WritableStream { readonly attribute Promise<void> negotiated; readonly attribute USVString protocol; readonly attribute USVString extensions; [Clamp] attributeshort closeCode; attribute USVString closeReason;};[Exposed=(Window,Worker)]interfaceWebSocketStream {constructor(USVString url, optional WebSocketStreamOptions options = {}); readonly attribute WebSocketReadableStream readable; readonly attribute WebSocketWritableStream writable;};This fully separates readers and writers, so you don't need to pass the parent duplex stream around at all to set close codes. The reader and writer operate entirely independently, to the point you can even send them to different threads (and browsers may be able to optimize that to not require an intermediate controlling thread). For high-throughput web sockets, this could offer a major speed-up opportunity. |
Uh oh!
There was an error while loading.Please reload this page.
Specify the WebSocketStream API. This exposes backpressure to
JavaScript, avoiding pathological behavior when one side of the
connection is not keeping up. It also provides a modern API using
Streams and Promises to interoperate cleanly with the modern web
ecosystem.
See the explainer at
https://github.com/ricea/websocketstream-explainer/blob/master/README.md
for examples and motivation.
(SeeWHATWG Working Mode: Changes for more details.)
Preview |Diff