Skip to content

Fix RTCPeerConnection override #966

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

Merged
merged 2 commits into from
Feb 23, 2021
Merged

Fix RTCPeerConnection override #966

merged 2 commits into from
Feb 23, 2021

Conversation

yousefamar
Copy link
Contributor

The signature for RTCPeerConnection.setLocalDescription has been overriden, as of #386, to have a mandatory argument. This is no longer the case; see specs here:

https://w3c.github.io/webrtc-pc/#dom-peerconnection-setlocaldescription
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/setLocalDescription#Parameters

This PR makes it optional, as it should be.

Resolves #875
Resolves microsoft/TypeScript#41939
Resolves microsoft/TypeScript#39142

Props to @jcc10 and @simonwep for finding this bug before me, but trying to fix it by modifying the generated declaration files, instead of here (as per these instructions). I think this should solve the problem for good and finally stop TypeScript from complaining about the argument count?

That being said, I still think that webidl2 needs to be looked at, as @simonwep said in #875, because in this file after line 44565 there should be "optional": 1, (like #386, this is all from 2018) but I deliberatiely didn't touch that since I'm thinking it needs to be fixed upstream.

@yousefamar yousefamar requested a review from sandersn as a code owner December 22, 2020 15:45
@ghost
Copy link

ghost commented Dec 22, 2020

CLA assistant check
All CLA requirements met.

@orta
Copy link
Contributor

orta commented Feb 23, 2021

Thanks, @yousefamar - I agree that this change matches the docs and we should take it 👍🏻

I guess we'll see what happens with the webidl change too

@orta orta merged commit c6d98e4 into microsoft:master Feb 23, 2021
@yousefamar yousefamar deleted the patch-1 branch February 23, 2021 17:42
@patelnav
Copy link

patelnav commented Mar 5, 2021

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