Set datachannel.binaryType = "arraybuffer" in connect() in WebRtcModule

[minwidth0px]

See #24 (comment). Edit: nevermind, I didn't run npm update on my second project.

[sinclairzx81]

@minwidth0px Hi, thanks for the PR.

Ideally, I'd like to keep the binaryType: arraybuffer configuration local to the Net module if possible as other transport types may want to use the binaryType: blob configuration. The WebRTC module is primary intended to be a signaling and network interface emulation system which produces channels but should not configure or interact with channels in anyway (as that is the responsibility of the downstream transport (i.e. Net))

As for the PR I created above, I don't actually know why "arraybuffer" must be set there, but I tested it and it worked only when that was set there (I'm on 0.8.4 right now). And I don't know why it didn't need to be set for a simple example such as https://minwidth0px.github.io/Webrtc-NAT-Traversal-Proxy-Server/client.html which works on 0.8.4 but does need to be set for the example I gave above as it seems to be doing literally the exact same thing.

I think it would be good to understand why the binaryType needs to be configured as part of the WebRtc module before continuing with the PR. As far as I can tell, the only functional difference setting the binaryType in the WebRTC module means the binaryType is configured "before" the channel open event (where as it's currently being set after in 0.8.5). Also, setting the binaryType on the same JS tick as the creation of the RTCDataChannel may play a role...but will need testing.

Also, I think this functionality could be achieved by refactoring the WebRtc connect function to just return the datachannel rather than waiting for it to open (as it is technically breaking it's own rule by ascribing event handlers to the data channel (which is an interaction with the data channel)).

/** Connects to a remote peer */
public async connect(remoteAddress: string, port: number): Promise<[WebRtcPeer, RTCDataChannel]> {
  const peer = await this.#resolvePeer(await this.#resolveAddress(remoteAddress))
  const datachannel = peer.connection.createDataChannel(port.toString(), { ordered: true, maxRetransmits: 16 })
  return [peer, datachannel] // returned immediately to caller (Net module) to configure 
                             // the binaryType on same JS tick.
}

Will do some digging

[minwidth0px]

Ok. I'll close this for now as its trivial to open a new one (1 line change).

As far as I can tell, the only functional difference setting the binaryType in the WebRTC module means the binaryType is configured "before" the channel open event (where as it's currently being set after in 0.8.4).

It's worth noting that setting datachannel.binaryType = "arraybuffer" was not good enough to make the simple project work, so clearly there is some difference between setting it here and setting it in the Net module. From #24:

What is more, setting datachannel.binaryType = "arraybuffer" in WebRtcModule.connect alone does not solve it. The RTCDataChannel when logged shows that it is of binaryType "arraybuffer", but when printing the following in NetSocket.connect you can see that the messages being sent are still of type "blob"

So it definitely is strange as you say. The way you are describing it with the WebRTC module setting it on the same tick as opening the channel, it would sound like setting it there should always be more reliable, but that did not appear to be the case.

As an aside, how can I run the tests to see if this PR makes more requests succeeded? I'd like to see if this has an effect that can be observed outside of my specific setup, which as I said I don't really understand myself why it is needed there. (Although I will take another look at that again and see if I can simplify it to use a regular RemoteNetworkHub so that it is easier to reason about.)

(Also, feel free to supersede this PR if you find a cleaner solution.)

[minwidth0px]

@sinclairzx81 ok... I did end up doing some more testing to repo this issue, and I found that I actually just didn't run npm update on the test project I was working on. (I only ran it on my other project.) Sorry about that.

[sinclairzx81]

@minwidth0px Hiya,

It's worth noting that setting datachannel.binaryType = "arraybuffer" was not good enough to make the simple project work, so clearly there is some difference between setting it here and setting it in the Net module. From #24:

Yeah, it wouldn't surprise me if setting the binaryType on the same tick as the data channel create has some effect, but really need to test and assert this to be true, so better to defer for now and investigate.

As an aside, how can I run the tests to see if this PR makes more requests succeeded? I'd like to see if this has an effect that can be observed outside of my specific setup, which as I said I don't really understand myself why it is needed there. (Although I will take another look at that again and see if I can simplify it to use a regular RemoteNetworkHub so that it is easier to reason about.)

If you grab the latest in the master branch, I've just added new infrastructure to let you run the tests in the browser using a npm script command.

$ npm run test:serve # visit: http://localhost:5000 - runs the full test suite
                     #        http://localhost:5000?filter=http:listener - only run the http listener tests

If you visit localhost:5000 in the page it will run the tests. Editing the codebase will re-run the tests on save. At this stage, I don't have infrastructure to run Firefox on CI (would need to make updates to https://github.com/sinclairzx81/drift to enable that), so for now, manual testing is the order of the day, but hopeful what's there helps bring visibility to some of the Firefox issues.

Cheers

[minwidth0px]

@sinclairzx81 I realized that this was just a mistake on my part and setting datachannel.binaryType = "arraybuffer" in connect() in WebRtcModule in never necessary. I just didn't upgrade the npm package in my second project where I was running this code and I didn't realize. So I just made a stupid mistake here. That's what I meant by #27 (comment)