dart-lang / web

Lightweight browser API bindings built around JS static interop.
https://pub.dev/packages/web
BSD 3-Clause "New" or "Revised" License
126 stars 21 forks source link

is RTCLocalSessionDescriptionInit needed? #178

Open jarrodcolburn opened 7 months ago

jarrodcolburn commented 7 months ago

Problem: RTCLocalSessionDescriptionInit class for "local" seems unneeded. I think RTCSessionDescriptionInit would suffice. Please see the screenshot where inconsistent between local and remote is highlighted

image


Recommendation A. Get rid of RTCLocalSessionDescriptionInit

Recommendation B (less good)... If RTCLocalSessionDescriptionInit is needed... it (and possible a new "remote" class RTCRemoteSessionDescriptionInit) should extend RTCSessionDescriptionInit.

jarrodcolburn commented 7 months ago

for more context, please see https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/createOffer specifically see their comment.

myPeerConnection
  .createOffer()
  .then((offer) => myPeerConnection.setLocalDescription(offer))

This Dart equivalent of this js won't work because in Dart createOffer() returns a "non-local" description, but Dart's setLocalDescription requires "local" description.

jarrodcolburn commented 7 months ago

Current implementation and links:

extension type RTCSessionDescriptionInit._(JSObject _) implements JSObject {
  external factory RTCSessionDescriptionInit({
    required RTCSdpType type,
    String sdp,
  });
extension type RTCLocalSessionDescriptionInit._(JSObject _) implements JSObject {
  external factory RTCLocalSessionDescriptionInit({
    RTCSdpType type,
    String sdp,
  });

RTCSessionDescriptionInit

RTCLocalSessionDescriptionInit

jarrodcolburn commented 7 months ago

Opened a ticket on the webref repo https://github.com/w3c/webref/issues/1155

kevmoo commented 7 months ago

Opened a ticket on the webref repo w3c/webref#1155

I'd be SO HAPPY if our efforts here help validate and improve the "source of truth" in this space. Thanks for being a good and proactive web citizen. 🤘

jarrodcolburn commented 7 months ago

Looks like the change needs to be made in the webrtc spec.

I opened https://github.com/w3c/webrtc-pc/issues/2940 there. If you could help me articulate need for issue there, I'd appreciate it @kevmoo

jarrodcolburn commented 6 months ago

@kevmoo did you see response in webrtc repo and do you have any thoughts.

For context, the MDN example RTCPeerConnection.setLocalDescription() providing_your_own_offer_or_answer see below with comments.

async function handleNegotiationNeededEvent() {
  try {
    const offer = await pc.createOffer(); // <= offer is (not "local") RTCSessionDescriptionInit
    pc.setLocalDescription(offer); // In Dart, offer can't be used for setLocalDecription 
    signalRemotePeer({ description: pc.localDescription });
  } catch (err) {
    reportError(err);
  }
}
kevmoo commented 6 months ago

@jarrodcolburn – can you include a link to the response here? I didn't see it

kevmoo commented 6 months ago

Found it! - https://github.com/w3c/webrtc-pc/issues/2940#issuecomment-1965488897

jarrodcolburn commented 6 months ago

He outlines the specific case for RTCLocalSessionDescriptionInit, then clarifies...

about the IDL...

WebIDL dictionaries are not classes or interfaces: "an operation that accepts a dictionary as an argument will perform a one-time conversion from the given JavaScript value into the dictionary, based on the current properties of the JavaScript object"

about member-compatibility...

IOW member-compatible inputs are valid, making RTCSessionDescriptionInit valid input to a method expecting RTCLocalSessionDescriptionInit.

and ultimately, about ingesting those IDL members...

If the Dart Language web bindings cannot express this then that seems like a limitation of those bindings.

kevmoo commented 6 months ago

@srujzs @sigmundch – thoughts here?

srujzs commented 6 months ago

I think their point is that APIs that expect one dictionary is also compatible with another dictionary that contains the necessary members, essentially defining dictionaries by their shape rather than their name. While true, this restricts how we can represent these types in Dart.

The closest feature to that description is records, but there's still a limitation because records with more parameters are not a subtype of records with fewer parameters e.g. (int a, int b) is not a subtype of (int a). There's another tradeoff with records too as we'll need to convert when it's passed to a JS API instead of when it's initialized, which may or may not be more expensive.

Ultimately, since the object does contain the necessary members, you can just cast to the right type here (RTCLocalSessionDescriptionInit <-> RTCSessionDescriptionInit) and everything should work fine, but it's admittedly not great that a cast is needed.

jarrodcolburn commented 6 months ago

I agree that...

Ultimately, since the object does contain the necessary members, you can just cast to the right type here (RTCLocalSessionDescriptionInit <-> RTCSessionDescriptionInit) and everything should work fine, but it's admittedly not great that a cast is needed.

...fixes my use case. But is this workaround an isolated in the web package?

As a dev, I kinda expect to be able to follow the MDN examples and implement them with little modification in Dart. Since the irony is that I'm not a JavaScript pro, or fan for that matter. And I'm trying to get out of JavaScript, and it's nuances, and back to my beloved Dart as soon as possible. But I get where I don't know if my code is failing because of my fondness of writing buggy code, or if I'm getting stuck on some weird JavaScript oddity (like this is not this, it's that).