search

cordova-rtc / cordova-plugin-iosrtc

Cordova iOS plugin exposing the WebRTC W3C API
MIT License
688 stars 340 forks source link

Description doesn't need to be an instance of RTCSessionDescription #370

Closed menelike closed 4 years ago

menelike commented 5 years ago

This error originally popped up in https://github.com/meetecho/janus-gateway/issues/1422

when they deprecated new RTCIceCandidate and new RTCSessionDescription and use the plain object instead. As a result, this plugin throws an error because of explicit checks like in

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/d46ca8f0a792fa67e06dae0f766419ba7025086d/js/RTCPeerConnection.js#L324-L335

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/d46ca8f0a792fa67e06dae0f766419ba7025086d/js/RTCPeerConnection.js#L233-L244

(please note that I have not checked the source for all occurences)

I did not test passing plain objects instead of new RTCIceCandidate but reading https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/d46ca8f0a792fa67e06dae0f766419ba7025086d/js/RTCPeerConnection.js#L416-L427 this should already be handled well.

hthetiot commented 5 years ago

We try to match WebRTC 1 spec, for me I think this should be WebRTC-adapter to deal with legacy. But I will investigate, PR welcome.

hthetiot commented 5 years ago

RTCSessionDescription is now the standard i dont think we should support plain object, this is WebRTC adapter to support old API not iosrtc. Also i dont see the point with Janus since its a server and third party component, they have to support the standard, i dont see why we would expose deprecate API to let third party use deprecated API themselves.

@saghul what do you think?

hthetiot commented 5 years ago

This is no longer necessary, however; RTCPeerConnection.setLocalDescription() and other methods which take SDP as input now directly accept an object conforming to the RTCSessionDescriptionInit dictionary, so you don't have to instantiate an RTCSessionDescription yourself.

WebRTC Spec does not show this Deprecation. But because Firefox documentation does.

menelike commented 5 years ago

I initially also wanted janus to follow the specs https://github.com/meetecho/janus-gateway/issues/1422#issuecomment-432683891, but I got corrected by https://github.com/meetecho/janus-gateway/issues/1422#issuecomment-432689394

https://w3c.github.io/webrtc-pc/#rtcsessiondescription-class clearly describes the constructor. But https://developer.mozilla.org/en-US/docs/Web/API/RTCSessionDescription/RTCSessionDescription marks that as deprecated. 🤷‍♂️