node-datachannel icon indicating copy to clipboard operation
node-datachannel copied to clipboard

fix: use binaryType correctly

Open achingbrain opened this issue 7 months ago • 14 comments

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

  1. Switches the default to arraybuffer
  2. 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.

achingbrain avatar Apr 15 '25 18:04 achingbrain

it's one of the things i fixed in ##324 ;-;

ThaUnknown avatar Apr 24 '25 18:04 ThaUnknown

  • 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 :)

murat-dogan avatar May 09 '25 14:05 murat-dogan

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

achingbrain avatar May 12 '25 20:05 achingbrain

Local 20 But the base reference still 18. Actions, tests etc.

murat-dogan avatar May 12 '25 20:05 murat-dogan

Any progress on this?

murat-dogan avatar May 24 '25 13:05 murat-dogan

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

achingbrain avatar May 24 '25 15:05 achingbrain

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.

murat-dogan avatar Jun 18 '25 18:06 murat-dogan

It's up to date and the build is happy now so it's ready for review/merging.

The linked PR seems unrelated.

achingbrain avatar Jun 19 '25 05:06 achingbrain

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?

murat-dogan avatar Jun 20 '25 09:06 murat-dogan

Also, could you please run the prettier command for formatting?

npx prettier -w .

murat-dogan avatar Jun 20 '25 10:06 murat-dogan

I have removed the node upgrade, added the polyfill and run the formatter.

achingbrain avatar Jun 25 '25 16:06 achingbrain

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?

murat-dogan avatar Jun 26 '25 15:06 murat-dogan

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.

achingbrain avatar Jun 26 '25 15:06 achingbrain

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
}

murat-dogan avatar Jun 26 '25 16:06 murat-dogan

Ah, you are right - I've updated the PR to retain string typing when strings are sent as before.

achingbrain avatar Jul 04 '25 08:07 achingbrain

Thanks! There are small formatting problems, but I can handle them.

murat-dogan avatar Jul 04 '25 11:07 murat-dogan

@murat-dogan thanks for merging this, any idea when it will make it into a release?

achingbrain avatar Jul 28 '25 08:07 achingbrain

Hello @achingbrain ,

v0.29.0 has been released. https://github.com/murat-dogan/node-datachannel/releases/tag/v0.29.0

murat-dogan avatar Aug 01 '25 14:08 murat-dogan