node-datachannel
node-datachannel copied to clipboard
fix: use binaryType correctly
The .binaryType field on the RTCDataChannel class defines what type the .data field on emitted MessageEvents will be.
It's either "arraybuffer" (the default) or "blob".
Docs: https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/binaryType
- Switches the default to
arraybuffer - Converts incoming data to the correct type based on the
binaryType
I've done my best to assert the right types/values in the "P2P test" in polyfill.test.ts.
Because getting the binary data out of a Blob is an async operation I had to make the test async, but it doesn't fit very well with the current nature of the test.
TBH that test is very complicated and would be much better off split into multiple smaller tests, each testing one thing.
it's one of the things i fixed in ##324 ;-;
- I am getting an error on tests TypeError: Promise.withResolvers is not a function
- As I said in the other PR, we need to stop extending the tests :)
Promise.withResolvers is in node 22. What version are you using?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers
Local 20 But the base reference still 18. Actions, tests etc.
Any progress on this?
No, sorry. I'm still waiting for @paullouisageneau to perform a release of libdatachannel with the memory leak fixes before spending more time here.
https://github.com/murat-dogan/node-datachannel/issues/349#issuecomment-2857545150
Hello @achingbrain , What is the plan for this PR? And also please check this PR? #359 I could not check it yet but I thought could be related.
It's up to date and the build is happy now so it's ready for review/merging.
The linked PR seems unrelated.
As I see, you updated the build versions to node 22. Since we are still supporting the node >= 18, I'm not sure if this is a good idea.
Yes, it is possible to use node v22 for development and target node version >= 18, but I think it will complicate the process.
If we really need that, of course we can, but only for Promise.withResolvers? Is it worth?
We can write a simple workaround like this;
// Polyfill for Promise.withResolvers for Node < 20
if (!Promise.withResolvers) {
(Promise as any).withResolvers = function <T>() {
let resolve: (value: T | PromiseLike<T>) => void;
let reject: (reason?: any) => void;
const promise = new Promise<T>((res, rej) => {
resolve = res;
reject = rej;
});
return { promise, resolve: resolve!, reject: reject! };
};
}
What do you think?
Also, could you please run the prettier command for formatting?
npx prettier -w .
I have removed the node upgrade, added the polyfill and run the formatter.
Thank you @achingbrain
When I run the test npx ts-node test/polyfill-connectivity.ts , I get an output like
Peer1 Received Msg: [object ArrayBuffer]
Peer2 Received Msg: [object ArrayBuffer]
Shouldn't it be a string?
No, it should be an ArrayBuffer because that's the default value of binaryType.
See - https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/binaryType
When a binary message is received on the data channel, the resulting message event's MessageEvent.data property is an object of the type specified by the binaryType.
The valid values are "arraybuffer" and "blob" - "string" is not allowed.
You can decode it with:
dc1.onmessage = (msg): void => {
console.log('Peer1 Received Msg:', new TextDecoder().decode(msg.data));
};
I'm guessing it printed a string before because msg.data.toString() was used - if msg.data was a Node.js Buffer then toString() would attempt to UTF-8 decode it's contents. Node.js Buffer's are not ArrayBuffers though, so it's not compatible with the spec to use them.
I am comparing with Chrome;
dc2.onmessage = (msg) => {
console.log('Peer2 Received Msg:', msg.data);
console.log(msg);
};
Chrome output:
Peer2 Received Msg: Hello from Peer1
{
"isTrusted": true,
...
"data": "Hello from Peer1",
...
"type": "message"
}
Our Output:
Peer2 Received Msg: ArrayBuffer {
[Uint8Contents]: <48 65 6c 6c 6f 20 66 72 6f 6d 20 50 65 65 72 31>,
byteLength: 16
}
MessageEvent {
type: 'message',
defaultPrevented: false,
cancelable: false,
timeStamp: 2424.882424
}
Ah, you are right - I've updated the PR to retain string typing when strings are sent as before.
Thanks! There are small formatting problems, but I can handle them.
@murat-dogan thanks for merging this, any idea when it will make it into a release?
Hello @achingbrain ,
v0.29.0 has been released. https://github.com/murat-dogan/node-datachannel/releases/tag/v0.29.0