Fix partial incompatibility with wrtc 0.0.63 #215
Conversation
…o have candidate pair with active connection when RTCDataChannel is opened (see node-webrtc/node-webrtc#339 for details)
|
@nazar-pc this looks like it would work to me; however, I guess you had to introduce the |
|
Great suggestion, I've made it even simpler now |
|
Nice! 👍 LGTM |
.gitignore
Outdated
| @@ -0,0 +1 @@ | |||
| node_modules | |||
There was a problem hiding this comment.
Please don't commit this file (git exclude).
There was a problem hiding this comment.
Removed, but why? It will be present in literally any clone and node_modules definitely shouldn't appear in repository.
index.js
Outdated
| if (item.type === 'remotecandidate' || item.type === 'remote-candidate') { | ||
| remoteCandidates[item.id] = item | ||
| function setSelectedCandidatePair (selectedCandidatePair) { | ||
| self._connecting = false |
There was a problem hiding this comment.
The use of self._connecting and self.connected like this means that self.connected will be false on a getStats error, even though the event fires. These should be left where it is, and a different variable can be used .
| self.remoteAddress = remote[0] | ||
| self.remotePort = Number(remote[1]) | ||
| self._debug('connect') | ||
| self.emit('connect') |
There was a problem hiding this comment.
This event can fire without self.connected being set to true if items is empty.
|
Applied some tweaks, does it look better now? |
|
Seems okay to me. It's unfortunate wrtc requires this hack, but I don't see it getting fixed any time soon. |
|
I don't think it will ever be fixed at all. As mentioned in linked issue latest Chrome also seems to have the same behavior and spec doesn't guarantee anything in this regard anyway. |
|
When to expect new release containing this fix? |
|
8.2.0 |
Fix partial incompatibility with wrtc 0.0.63
Fix partial incompatibility with wrtc 0.0.63 that doesn't guarantee to have candidate pair with active connection when RTCDataChannel is opened (see node-webrtc/node-webrtc#339 for details)
Test is added in order to reproduce the issue and confirm the fix. Easier to review with
?w=1is URL in order to hide code indentation changes.cc @markandrus