Allow `null` candidate in `RTCPeerConnection.addIceCandidate`

[silverlyra]

Add an override for addIceCandidate’s candidate parameter to allow it to be null, which is a special value indicating that ICE gathering is complete.

If no candidate object is specified, or its value is null, an end-of-candidates signal is sent to the remote peer [emphasis added]

Fixes #1738.

[github-actions[bot]]

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

[silverlyra]

@microsoft-github-policy-service agree

[saschanaz]

Passing null is same as passing undefined in this case per https://webidl.spec.whatwg.org/#js-dictionary. I'm not sure we really need a special case here?

[saschanaz]

We could instead allow null in general this case, hmm.

[silverlyra]

We could instead allow null in general this case, hmm.

I looked at the WebIDL §3.2.17 you linked to – what would you say the general fix would be? Any optional T parameter becomes nullable if T is a dictionary? Or any dictionary parameter becomes nullable if none of its members are required?

I'm not sure we really need a special case here?

It is noted as a special case not captured by WebIDL in the WebRTC specification:

Due to WebIDL processing, addIceCandidate(null) is interpreted as a call with the default dictionary present, which, in the above algorithm, indicates end-of-candidates for all media descriptions and ICE candidate generation. This is by design for legacy reasons.

I do think it’s appropriate to declare these parameters as nullable. The types right now are meaningfully incorrect – implementing WebRTC signaling is (in part) basically a process of piping icecandidate events on one peer into the addIceCandidate method of the remote peer. But TypeScript rejects this, because the event candidate property is nullable, and the method candidate parameter isn’t.

The simplest RTCPeerConnection sample just creates two peer connections in the same browser tab. If I reproduce the relevant parts of its code in TypeScript:

let pc1 = new RTCPeerConnection();
pc1.addEventListener('icecandidate', e => onIceCandidate(pc1, e));

let pc2 = new RTCPeerConnection();
pc2.addEventListener('icecandidate', e => onIceCandidate(pc2, e));

async function onIceCandidate(pc: RTCPeerConnection, event: RTCPeerConnectionIceEvent) {
  let target = pc === pc2 ? pc1 : pc2;
  await target.addIceCandidate(event.candidate);
}

it doesn’t build:

Argument of type RTCIceCandidate | null is not assignable to parameter of type RTCIceCandidateInit | undefined. Type null is not assignable to type RTCIceCandidateInit | undefined.

[nikwen]

Thanks for the fix! Would love to see it merged as I ran into the same issue.

[saschanaz]

We should make null work for dictionaries in general. That wouldn't be straightforward, so for now I'll merge this.

LGTM

[github-actions[bot]]

There was an issue merging, maybe try again saschanaz. Details

[saschanaz]

Oops, merge conflicts.

[nikwen]

Thanks, @saschanaz!

I don't think it's a merge conflict. I tried merging it locally and it worked perfectly. I think the GitHub actions bot ran into a network issue.

From the log:

Creating comments and merging
Merging (or commenting) failed:
Error: HttpError: Resource not accessible by integration
Error: Failed to merge

[saschanaz]

Local merging somehow is different from GitHub merging time to time. ~~Can you push your merge here?~~ Oops, you are not the author 😄

[saschanaz]

LGTM (retrying) but other PR worked, so if this still doesn't work then I think it's merge problem.

[github-actions[bot]]

There was an issue merging, maybe try again saschanaz. Details

[saschanaz]

Perhaps auto merge needs some permission? @jakebailey might know...

[nikwen]

Got it! ~How/where do you want me to push? (This is not my PR.)~ Ah, just saw your edit.

I did the following to merge locally:

git clone [email protected]:microsoft/TypeScript-DOM-lib-generator.git
cd TypeScript-DOM-lib-generator
git remote add silverlyra [email protected]:silverlyra/TypeScript-DOM-lib-generator.git
git fetch silverlyra
git merge silverlyra/null-ice-candidate

[jakebailey]

I tightened perms on a bunch of stuff; it's possible this broke (I will look tomorrow)

[silverlyra]

Hello! Glad to see activity here 😄 I’ve rebased

[saschanaz]

Test is failing, can you check whether it passes for you? (I just pushed #1786, so might want to merge that too)

[jakebailey]

I did nothing but https://github.com/microsoft/TypeScript-DOM-lib-generator/pull/1787 didn't fail to merge, so maybe this was a temporary problem.

[saschanaz]

LGTM

[github-actions[bot]]

Merging because @saschanaz is a code-owner of all the changes - thanks!

[saschanaz]

Huh, seems so!

[saschanaz]

Might also because @silverlyra rebased it. Worth checking with non-rebased PRs...

[nikwen]

Thanks for working on this, everyone! :raised_hands: