denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.42k stars 5.18k forks source link

upgradeWebSocket could return a WebSocketStream #14064

Open MarkTiedemann opened 2 years ago

MarkTiedemann commented 2 years ago

Currently, Deno.upgradeWebSocket returns a WebSocket. It could return a WebSocketStream, too, for example:

const conn = await Deno.listen({ port: 80 });
const httpConn = Deno.serveHttp(await conn.accept());
const e = await httpConn.nextRequest();
if (e) {
  const { stream, response } = Deno.upgradeWebSocket(e.request);
  const [_, { writable, readable }] = await Promise.all([
    e.respondWith(response),
    stream.connection,
  ]);
  // Use writable and/or readable
}
crowlKats commented 2 years ago

Yes, this is the plan longterm, and was actually the plan originally to some degree (for both statements, its rather not alongside, but instead of), however, the idea is that the server side and client side use the same API for a smoother experience, but since browsers do not have WebSocketStream yet, this isnt exactly the case

danopia commented 1 year ago

I'm looking forward to this feature as well 😃 I have Deno programs as both the client and the server, so I get to do the nice W3C streams stuff on the client side, just to deal with the classic WebSocket callbacks on the server side.

bartlomieju commented 1 year ago

I discussed it with @crowlKats recently and the fact that there's still no spec makes it quite hard to ship in Deno.

@crowlKats anything changed since we spoke about it?

crowlKats commented 1 year ago

Sadly no, there has been no progress from what I am aware

korkje commented 10 months ago

Could it make sense to, at least while WebSocketStream is getting all spec'ed and ready, provide an experimental/unstable upgradeWebSocketStream alternative alongside upgradeWebSocket? Would prevent any breaking changes for users, while also letting users choose their preferred API.

korkje commented 10 months ago

Tried my hand at a PR taking some inspiration from, and trying to improve upon, the earlier PR (https://github.com/denoland/deno/pull/16732) by @crowlKats.

masx200 commented 5 months ago
export class WebSocketStream {
    public socket: WebSocket;
    public readable: ReadableStream<Uint8Array>;
    public writable: WritableStream<Uint8Array>;

    constructor(socket: WebSocket) {
        this.socket = socket;
        this.readable = new ReadableStream({
            start(controller) {
                socket.onmessage = function ({ data }) {
                    // console.log(data);
                    return controller.enqueue(new Uint8Array(data));
                };
                socket.onerror = (e) => controller.error(e);
                socket.onclose = (/* e */) => controller.close(/* e */);
            },
            cancel(/* reason */) {
                socket.close();
            },
        });
        this.writable = new WritableStream({
            start(controller) {
                socket.onerror = (e) => controller.error(e);
            },
            write(chunk) {
                // console.log(chunk);
                socket.send(chunk);
            },
            close() {
                socket.close();
            },
            abort(e) {
                console.error(e);
                socket.close();
            },
        });
    }
}
rikka0w0 commented 4 months ago
export class WebSocketStream {

One important feature of the WebSocketStream is the backpressure handling, and this simple wrapper does not support backpressure at all. It is possible to implement backpressure on the outgoing stream via WebSocket: bufferedAmount, but not possible on the incoming stream. Missing the backpressure support can overwhelm the receiver and potentially cause out-of-memory.