websockets / ws

Simple to use, blazing fast and thoroughly tested WebSocket client and server for Node.js
MIT License
21.34k stars 2.3k forks source link

Support sending Blob #2206

Closed kettanaito closed 6 days ago

kettanaito commented 3 months ago

Is there an existing issue for this?

Description

Currently, ws doesn't accept Blob as an argument to the .send() function:

wss.on('connection', (ws) => ws.send(new Blob(['hello']))

I believe this is mainly for historic reasons because there used to be no Blob in Node.js. There is now, and it would be great to align the sending (and receiving also) with the WHATWG WebSocket standard.

ws version

8.16.0

Node.js Version

v20.11.0

System

Irrelevant.

Expected result

ws supports sending Blob from the server.

Actual result

TypeError: The first argument must be of type string or an instance of Buffer, ArrayBuffer, or Array or an Array-like Object. Received an instance of Blob

Attachments

No response

lpinca commented 3 months ago

It is not for historic reasons, see https://github.com/nodejs/undici/pull/1795#discussion_r1038928224.

kettanaito commented 3 months ago

This is an important context, thanks for sharing! So, Blobs being written first is a Node.js thing?

This is also interesting:

In practice, even though it requires explicitly setting binaryType, 97% of messages are received as ArrayBuffers.

Especially given that the default binaryType value for blob. I believe Undici did end up supporting sending Blob from the client (at least, I seem to be able to do that in tests).

lpinca commented 3 months ago

So, Blobs being written first is a Node.js thing?

Yes, because there is no official API to get an ArrayBuffer synchronously from a Blob and streams do not support Blobs natively.

I believe Undici did end up supporting sending Blob from the client (at least, I seem to be able to do that in tests).

I think it is affected by the issue I wrote in that comment.

lpinca commented 3 months ago

See also https://github.com/nodejs/undici/pull/1795#issuecomment-1353535714.

kettanaito commented 3 months ago

Got it. Thanks for the great references! Do you consider this proposal to be out of scope? We should close it then.

lpinca commented 3 months ago

It is doable but in my opinion it is not worth it. Added complexity for little to no benefit.

kettanaito commented 3 months ago

What about supporting this only on the surface level? Accepting the Blob but converting it internally to nodebuffer and proceeding as ws does now? If that makes any sense, of course (would love to learn if it doesn't).

My main use case is showcasing ws for testing, and it's not great to omit sending/receiving Blob data.

lpinca commented 3 months ago

Accepting the Blob but converting it internally to nodebuffer and proceeding as ws does now?

It is not trivial because again, converting a Blob to an ArrayBuffer/Buffer synchronously is not supported by Node.js. We could pause the Sender like we do when compressing data but I'm not very happy with it. Also what about support for Node.js versions where Blob is not supported?

It is a lot easier to convert the Blob before calling websocket.send().

websocket.send(await blob.arrayBuffer());
kettanaito commented 3 months ago

It is not trivial because again, converting a Blob to an ArrayBuffer/Buffer synchronously is not supported by Node.js.

I keep missing this bit. Got it.

Also what about support for Node.js versions where Blob is not supported?

Blob is supported since Node.js 18, which by itself reaches the maintenance mode in a few months. Does ws support EOL versions of Node.js (16 and older)? If anything, I see this as a motivation to drop those old versions but you would know far better than me here.

lpinca commented 3 months ago

Yes, ws still supports Node.js 10.

kettanaito commented 3 months ago

It is a lot easier to convert the Blob before calling websocket.send().

I understand this. What I'm proposing is not a matter of convenience but expectations. As in, valid WebSocket usage as per spec should work. Sending a Blob is a valid usage. Not supporting it is a limitation of ws (as the immediate agent; the issue runs deeper as we've discussed) so it may be a good thing to try to find a solution/compromise here.

I also understand that ws doesn't promise full WHATWG compliance as it's a server library and the spec has nothing about servers, afaik. I'm coming purely from the client usage. If a WebSocket client, like Undici, sends a Blob, it's okay if ws receives and handles it as something else. The issue is that the client will never receive a Blob, even if its binaryType = 'blob' (I will double-check on this but I recall that being the outcome of my testing).

lpinca commented 3 months ago

Strictly following the WHATWG specification is a non-goal for ws.

lpinca commented 3 months ago

The issue is that the client will never receive a Blob, even if its binaryType = 'blob' (I will double-check on this but I recall that being the outcome of my testing)

That is correct for ws. That binary type is not supported. Receiving a Blob is easier as it can be created synchronously from an ArrayBuffer but if we allow receiving it we should also allow sending it.

kettanaito commented 5 days ago

Thank you for your work on this! 💪