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

RTCDataChannel.onmessage throws exception when binaryType is "arraybuffer" #303

Closed
wxyz-abcd opened this issue Nov 16, 2024 · 5 comments
Closed

Comments

@wxyz-abcd
Copy link
Contributor

Hi. I am trying to switch from @koush/wrtc to your library. I am using the polyfill in node.js environment. When I try to receive binary data from a RTCDataChannel, in this line, I encountered this error:

First argument to DataView constructor must be an ArrayBuffer

The argument is received as an Uint8Array, but then converted to a node.js Buffer by default, while it should in fact take into account the value of binaryType before the conversion.

@wxyz-abcd
Copy link
Contributor Author

wxyz-abcd commented Nov 17, 2024

I added 2 different fixes. Both work perfectly fine for me, but I prefer #305 over #304 since it makes more sense. And i have to add that i could not test receiving string data.

@murat-dogan
Copy link
Owner

Hello and thank you for the PRs

The better will be create a jest test for this.
Send a string, blob, buffer and test it like p2p test.

I can make it when I have time, or you can create one if you have time.
Then we can be sure that it will work in all cases and merge one of PRs

what you think?

@wxyz-abcd
Copy link
Contributor Author

Hello, no problem.
I think the second PR is correct, but forget about those PR's. I will create a new much bigger PR soon. I had to change some typescript definitions in order to be able to test the new code, I also added some polyfill test codes and ran them myself and I can confirm that they are running correctly without any problems, at least on my computer. Lastly, according to this link, the default value of binaryType is "blob" so I also fixed that within this PR.

wxyz-abcd added a commit to wxyz-abcd/node-datachannel that referenced this issue Nov 20, 2024
@wxyz-abcd
Copy link
Contributor Author

And sorry about the whitespace formatting... GitHub might be messing it up. :)

@murat-dogan
Copy link
Owner

Solved by #309

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