-
-
Notifications
You must be signed in to change notification settings - Fork 72
fix: use binaryType correctly #348
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
fix: use binaryType correctly #348
Conversation
e1266f1 to
f492b05
Compare
|
it's one of the things i fixed in ##324 ;-; |
|
|
Promise.withResolvers is in node 22. What version are you using? |
|
Local 20 |
|
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. |
|
Hello @achingbrain , |
|
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. Yes, it is possible to use node v22 for development and target node version >= 18, but I think it will complicate the process. We can write a simple workaround like this; What do you think? |
|
Also, could you please run the prettier command for formatting? |
bdee61e to
8de55cd
Compare
|
I have removed the node upgrade, added the polyfill and run the formatter. |
|
Thank you @achingbrain When I run the test Shouldn't it be a string? |
|
No, it should be an See - https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/binaryType
The valid values are 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 |
|
I am comparing with Chrome; Chrome output: Our Output: |
The `.binaryType` field on the `RTCDataChannel` class defines what type the `.data` field on emitted `MessageEvent`s 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.
c06237a to
a3474a1
Compare
|
Ah, you are right - I've updated the PR to retain string typing when strings are sent as before. |
|
Thanks! |
|
@murat-dogan thanks for merging this, any idea when it will make it into a release? |
|
Hello @achingbrain , v0.29.0 has been released. |
The
.binaryTypefield on theRTCDataChannelclass defines what type the.datafield on emittedMessageEvents will be.It's either
"arraybuffer"(the default) or"blob".Docs: https://developer.mozilla.org/en-US/docs/Web/API/RTCDataChannel/binaryType
arraybufferbinaryTypeI'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
Blobis 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.