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

Replace sinkId attribute with getSinkId method #2531

Closed
beaufortfrancois opened this issue Nov 30, 2022 · 3 comments
Closed

Replace sinkId attribute with getSinkId method #2531

beaufortfrancois opened this issue Nov 30, 2022 · 3 comments
Labels
Needs Discussion The issue needs more discussion before it can be fixed.

Comments

@beaufortfrancois
Copy link

TLDR; I believe we should not expose a new AudioSinkInfo interface to the web platform when we can "simply" use a dictionary for this purpose. By replacing the sinkId attribute with a getSinkId() method, we can reuse the existing AudioSinkOptions dictionary.

I've been playing with AudioContext.sinkId recently and I really wish we'd have readonly attribute (DOMString or AudioSinkOptions) sinkId; instead of readonly attribute (DOMString or AudioSinkInfo) sinkId;. From what I understand, it is what was planned from the beginning, but because of the WebIDL rule "Dictionaries must not be used as the type of an attribute or constant." the design was updated, and eventually resulted in a new AudioSinkInfo interface added to the spec.

I think using a dictionary is better here because:

  1. it doesn't pollute the global scope. See Classes (WebIDL interfaces) should only be used when you have both data and behavior w3ctag/design-principles#11
  2. stringification comes for free.

I am sympathetic to the fact we're trying to achieve symmetry with the existing AudioElement.sinkId. In that specific case though, sinkId is not a DOMString only anymore. It can be both a string and a dictionary, which means enhancing AudioElement.sinkId to return a (DOMString or AudioSinkInfo) in the future would certainly break existing web apps as they expect only a String.

I would propose then we use a method named getSinkId() that returns (DOMString or AudioSinkOptions) instead of a sinkId attribute.

IDL Proposal:

- interface AudioSinkInfo {
-   readonly attribute AudioSinkType type;
- };

  interface AudioContext : BaseAudioContext {
-   [SecureContext] readonly attribute (DOMString or AudioSinkInfo) sinkId;
+   [SecureContext] (DOMString or AudioSinkOptions) getSinkId ();

For background, this WebIDL rule has bitten some people already. See WICG/sanitizer-api#107 for instance. The resolution in that case was also to use a method instead of an attribute.

Addendum: For info, here's what the spec would look like with this proposed change: https://github.com/WebAudio/web-audio-api/pull/2529/files

@hoch
Copy link
Member

hoch commented Nov 30, 2022

I am sympathetic to the fact we're trying to achieve symmetry with the existing AudioElement.sinkId. In that specific case though, sinkId is not a DOMString only anymore. It can be both a string and a dictionary, which means enhancing AudioElement.sinkId to return a (DOMString or AudioSinkInfo) in the future would certainly break existing web apps as they expect only a String.

The existing app will continue to work because the sinkId property only returns the AudioSinkInfo object when it is explicitly given by the user. If the user decides to use it only with String like before, it will continue to work. If the user decided to work with the new way (AudioSinkOptions), then it's sensible to use the sinkId property accordingly.

My preference is API consistency and ergonomics over other principles. If that was not the goal of this project, I would have proposed a completely different API that is not based on AudioElement.setSinkId(). If we are losing that benefit, then perhaps we might want to revisit this design to make a fundamental change.

@hoch
Copy link
Member

hoch commented Nov 30, 2022

For background, this WebIDL rule has bitten some people already. See WICG/sanitizer-api#107 for instance. The resolution in that case was also to use a method instead of an attribute.

This is a slightly different context - I don't think the problem in discussion needs to maintain consistency with other APIs. In that case, using a method or a different approach makes sense.

@hoch hoch added the Needs Discussion The issue needs more discussion before it can be fixed. label Nov 30, 2022
@mdjp
Copy link
Member

mdjp commented Jan 12, 2023

Discussed and agreed to close

@mdjp mdjp closed this as completed Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion The issue needs more discussion before it can be fixed.
Projects
None yet
Development

No branches or pull requests

3 participants