WebAudio / web-audio-api

The Web Audio API v1.0, developed by the W3C Audio WG
https://webaudio.github.io/web-audio-api/
Other
1.04k stars 166 forks source link

AudioWorklet string name collisions #839

Closed cwilso closed 8 years ago

cwilso commented 8 years ago

From an IM chat with @hoch:

I'm really worried about the registration and reference of AudioWorklets being purely a string reference name. Seems like to cause a lot of conflicts, as devs build apps that include multiple Worklet libraries. It seems like we should give an optional prepend label to the "import" call, or resolving the import() with some form of factory to create Processors from just that Worklet library.

The use case, for example, is if I (an app author) want to use and compare two different custom node libraries, both of which define a "BitCrusher"), and do an A/B test. With the current design, if both vendors happen to name their node "BitCrusher", I can't ever do this - because I'm not in control of what string token they put inside their library.

@hoch said "This is how Worklet infrastructure works, so I didn't want to diverge from there." That just means now I'm concerned about other Worklet cases, too, because this seems like it doesn't scale well. (cc @bfgeek)

@hoch said "But we have same problems like this with npm or bower. They operate the whole system based on a string." - but my response is that npm and bower have - by design - a global registry. This forcibly eliminates name conflicts up front. The problem isn't that there's a string token, per se - it's that the registry isn't globally organized (like npm or bower), and simple string tokens are most likely.

To fix this, I can see a couple of options - optionally supply a string prepend to the token (e.g. window.audioWorklet.import('bitcrusher.js', "vendor1").then()) and access via new AudioWorkletNode(context, "vendor1BitCrusher") (the label could be optional, of course, in which case it looks just like current design.

or, have the audioworklet.import return a factory for nodes, or a token you pass in to the AudioWorkletNode creator.

Regardless, should try to rationalize against general pattern of Worklets.

padenot commented 8 years ago

Yeah, we've mentioned this a couple times, including last time in Atlanta, but I can't remember why we did not do something about it.

Your concerns are certainly valid.

hoch commented 8 years ago

Actually CustomElements API has a similar issue, and it tries to resolve this by having a constraint on the custom name: the name must contain a dash (-). I wonder how/why the working group of CE reached the solution.

In terms of the exposed surface and the order of scale, I believe it is fair to say that CE is a lot bigger than WebAudio, but their solution seems to be reasonable to many developers. Perhaps we can do a similar approach?

bfgeek commented 8 years ago

Custom elements actually have this restriction so that the platform can add native html elements later without fear of breaking sites. I.e. <circle> <!-- Illegal as no '-' -->

I wouldn't worry about this and just educate libraries to prefix their custom nodes.

This is pretty common on the platform today with:

If you are particularly worried about this the suggestion from @hoch seems good, just require a '-' in the name to encourage prefixing.

cwilso commented 8 years ago

But these aren't the same in usage. The closest one is custom elements; and the original design did, IIRC, enable you to work around this potential problem. (I think you might have this problem now.) But custom elements have that restriction in order to future-proof the HTML namespace (i.e. you couldn't have a popular library take over the tagname "circle" and prevent HTML 6.3 from adding an element named "circle").

CSS variables and CustomEvents are likewise much less likely to have conflicting objects defined by multiple libraries both intended to be used in a single application.

It just seems super-likely that eventually audio apps will want to plug in multiple libraries of custom nodes, and offer users a choice between similar nodes. "Use a complex name" seems like less than ideal guidance. It would seem easy enough to return a factory or token from import() that was specific to this library.

On Fri, Jun 10, 2016 at 11:22 AM, Ian Kilpatrick notifications@github.com wrote:

Custom elements actually have this restriction so that the platform can add native html elements later without fear of breaking sites. I.e.

I wouldn't worry about this and just educate libraries to prefix their custom nodes. This is pretty common on the platform today with: - Custom elements (as mentioned) - CSS Variables (--width, no restriction apart from leading --, again just to prevent future native properties) - Custom events, (new CustomEvent('action'); no restriction) - Element attributes (all are fair game I believe; people are encouraged to use data- prefixed ones, again so that platform can add later) ... etc. If you are particularly worried about this the suggestion from @hoch https://github.com/hoch seems good, just require a '-' in the name to encourage prefixing. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/WebAudio/web-audio-api/issues/839#issuecomment-225258213, or mute the thread https://github.com/notifications/unsubscribe/AAe8eU8BENlUVcXq4q1OTzNqXKuqgkYqks5qKatVgaJpZM4Iyggx .
hoch commented 8 years ago

Perhaps we can have something like this?

/* fooNode.js: AudioWorkletGlobaScope */
registerAudioWorkletProcessor('FooNode', class extends AudioWorkletProcessor {
  // class definition here.
});

First, we can do generic constructor pattern.

/* Global Scope */
Promise.all([
  window.audioWorklet.import('fooNode.js', 'A'),
  window.audioWorklet.import('fooNode.js', 'B')
]).then(function () {
  var context = new AudioContext();
  var foo1Node = new AudioWorkletNode(context, 'FooNode-A');
  var foo2Node = new AudioWorkletNode(context, 'FooNode-B');
});

Alternatively, subclassing pattern.

/* Global Scope */
Promise.all([
  window.audioWorklet.import('fooNode.js', 'A'),
  window.audioWorklet.import('fooNode.js', 'B')
]).then(function () {

   class FooNodeA extends AudioWorkletNode {
      constructor (context, options) {
        super(context, 'FooNode-A', options);
      }
      // more definition stuffs.
   }

   class FooNodeB extends AudioWorkletNode {
      constructor (context, options) {
        super(context, 'FooNode-B', options);
      }
      // more definition stuffs.
   }

   var context = new AudioContext();
   var fooNodeA = new FooNodeA(context);
   var fooNodeB = new FooNodeB(context);
});
joeberkovitz commented 8 years ago

WG current plan is to go ahead without a solution to this problem but consult with Worklet to see how they're dealing with this for custom CSS

hoch commented 8 years ago

I've just had a brief discussion with @bfgeek and learned there is a promising solution for this issue.

Instead of registerAudioWorkletProcessor() method we can use the export keyword in the AudioWorkletGlobalScope.

// foo.js and bar.js have a definition with the same class name.
export class VUMeter extends AudioWorkletProcessor {...}

Then in the main global scope, import() method can rename the definition accordingly.

audioWorklet.import('foo.js');
audioworklet.import('bar.js', { 'VUMeter': 'BarVUMeter' });

The biggest advantage of this scheme is that it is backward compatible. We can move forward with the current design and introduce this feature later when the worklet infrastructure serves this pattern.

joeberkovitz commented 8 years ago

@hoch are you saying that we would replace registerAudioWorkletProcessor() with export... at this stage of the design? It feels rather as though this may be overloading the concept of "exporting..."

hoch commented 8 years ago

It's not replacing. You can use both ways, but developers have to use export if they want to support the dynamic class renaming. This pattern is not flattened yet and still in flux. I just wanted to update WG that something like this is being discussed for the Worklet.

bfgeek commented 8 years ago

Yeah not replace at the moment, it's a little too early to tell if this would be ergonomic / implementable.

For CSS things we are continuing down path of register* and will discuss at TPAC. We don't have to change the world for this :).

joeberkovitz commented 8 years ago

Resolved since spec now legislates that duplicate registration in the same AudioWorkletGlobalScope is disallowed.

hoch commented 6 years ago

The reference for community-driven effort on this topic: https://github.com/angular/angular-component-spec

bicknellr commented 5 years ago

Could this be reopened? The resolution given here is no longer correct.

bicknellr commented 5 years ago

Actually, it seems like I interpreted "duplicate registration in the same AudioWorkletGlobalScope is disallowed" incorrectly.