Overloaded operations: Throw or reject?

[adam-be]

pc.addIceCandidate(); // TypeError

The above code needs to produce a TypeError since the mandatory candidate argument is missing. We have specified two overloaded version of addIceCandidate:

// Promise version
Promise<void> addIceCandidate ((RTCIceCandidateInit or RTCIceCandidate)? candidate);

// Legacy version with callbacks
void addIceCandidate ((RTCIceCandidateInit or RTCIceCandidate) candidate, VoidFunction successCallback, RTCPeerConnectionErrorCallback failureCallback);

As specified, one should reject with TypeErrors during WebIDL argument checking, and the other should throw. What should happen in the case above?

[alvestrand]

Been through this with the createOffer and friends :-(

WebIDL doesn't allow overload of a non-promise with a promise, so the legacy version has to be:

Promise<void> addIceCandidate ((RTCIceCandidateInit or RTCIceCandidate) candidate, VoidFunction successCallback, RTCPeerConnectionErrorCallback failureCallback);

Luckily we can distinguish those two by signature, and the callback is mandatory.

The legacy function has to call the failure callback with a TypeError; it cannot throw.

[adam-be]

It's not pretty but I was leaning towards the same conclusion.

I might have worked with Promise and void return types if both versions threw on WebIDL argument checker errors. But that's not the case so.

We need patch the spec then.

@jan-ivar, any input on this?

[adam-be]

So what do we do with the promise returned by the legacy function (i.e. not the promise returned by the referenced implementation steps)? Should we fulfill/reject it after the corresponding callbacks have been run or leave it be?

[alvestrand]

As long as people don't save it (ie if they assume the return type is void()), it doesn't matter.
FWIW, @guidou says that for the legacy PeerConnection functions (including addIceCandidate), we always returned a promise resolved with undefined.

[adam-be]

A quick test in the firefox inspector gives:

> pc.setLocalDescription(null, () => {}, () => {});
Promise { <state>: "rejected", <reason>: TypeError }

[guidou]

Note that @adam-be's example fails the same way in Chrome.

pc.setLocalDescription(null, () => {}, () => {});
Uncaught (in promise) TypeError: Failed to execute 'setLocalDescription' on 'RTCPeerConnection': parameter 1 is not of type 'RTCSessionDescription'

TypeErrors are the reason all/none of the overloads have to return Promise. If some overloads returned Promise and some didn't it would be impossible to unambiguously determine the right way to return a TypeError when argument types do not match any overload.

[jan-ivar]

As @alvestrand says, always return a promise resolved with undefined when callbacks are detected, even on failure. That way we wont invoke the ire of browsers' unhandled promise detection code.

I did a test, and both Firefox and Chrome Canary pass (failure callback fires, promise resolves), though adapter.js in Chrome appears to have a bug (failure callback fires, promise rejects).

(I've removed earlier comments where I confused myself on this point).

[jan-ivar]

(my test is about errors thrown from prose in this spec. The TypeErrors from WebIDL binding code are out of our control.)

[adam-be]

Proposed fix: #618

[adam-be]

So to go into "legacy mode", both callbacks need to be provided. Otherwise we don't have an error callback to call with a possible TypeError (or some later error). Is that correct? We get a bit of a custom overload, but I guess it would work.

[alvestrand]

Exactly. That was the reason why Chrome deprecated the no-failure-callback form of the functions in M50 and removed them entirely for M51.

https://www.chromestatus.com/metrics/feature/timeline/popularity/1053 is the use counter for the no-callback form of setLocalDescription - all the other forms we measured had microscopic usage.