Skip to content
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

Support sending Blob #2206

Closed
1 task done
kettanaito opened this issue Mar 12, 2024 · 14 comments · Fixed by #2229
Closed
1 task done

Support sending Blob #2206

kettanaito opened this issue Mar 12, 2024 · 14 comments · Fixed by #2229

Comments

@kettanaito
Copy link

kettanaito commented Mar 12, 2024

Is there an existing issue for this?

  • I've searched for any related issues and avoided creating a duplicate issue.

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

  • Type error on the unknown input type to .send();
  • A runtime TypeError:
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
Copy link
Member

lpinca commented Mar 12, 2024

It is not for historic reasons, see nodejs/undici#1795 (comment).

@kettanaito
Copy link
Author

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
Copy link
Member

lpinca commented Mar 12, 2024

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
Copy link
Member

lpinca commented Mar 12, 2024

See also nodejs/undici#1795 (comment).

@kettanaito
Copy link
Author

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

@lpinca
Copy link
Member

lpinca commented Mar 13, 2024

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

@kettanaito
Copy link
Author

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
Copy link
Member

lpinca commented Mar 14, 2024

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
Copy link
Author

kettanaito commented Mar 14, 2024

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
Copy link
Member

lpinca commented Mar 14, 2024

Yes, ws still supports Node.js 10.

@kettanaito
Copy link
Author

kettanaito commented Mar 14, 2024

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
Copy link
Member

lpinca commented Mar 14, 2024

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

@lpinca
Copy link
Member

lpinca commented Mar 14, 2024

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.

lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 11, 2024
lpinca added a commit that referenced this issue Jun 13, 2024
lpinca added a commit that referenced this issue Jun 13, 2024
lpinca added a commit that referenced this issue Jul 1, 2024
@lpinca lpinca closed this as completed in 59b9629 Jul 2, 2024
@kettanaito
Copy link
Author

Thank you for your work on this! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants