Skip to content
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

is RTCLocalSessionDescriptionInit needed? #178

Open
jarrodcolburn opened this issue Feb 19, 2024 · 12 comments
Open

is RTCLocalSessionDescriptionInit needed? #178

jarrodcolburn opened this issue Feb 19, 2024 · 12 comments

Comments

@jarrodcolburn
Copy link

jarrodcolburn commented Feb 19, 2024

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
Copy link
Author

jarrodcolburn commented Feb 19, 2024

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
Copy link
Author

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
Copy link
Author

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

@kevmoo
Copy link
Member

kevmoo commented Feb 20, 2024

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
Copy link
Author

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

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

@jarrodcolburn
Copy link
Author

@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
Copy link
Member

kevmoo commented Feb 27, 2024

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

@kevmoo
Copy link
Member

kevmoo commented Feb 27, 2024

Found it! - w3c/webrtc-pc#2940 (comment)

@jarrodcolburn
Copy link
Author

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
Copy link
Member

kevmoo commented Feb 27, 2024

@srujzs @sigmundch – thoughts here?

@srujzs
Copy link
Contributor

srujzs commented Feb 27, 2024

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
Copy link
Author

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).

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

No branches or pull requests

3 participants