-
Notifications
You must be signed in to change notification settings - Fork 56
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
ES6 classes that inherits from AudioNode stopped working when the extension is enabled #73
Comments
I think the tracing logic is basically failing on the derived constructor based on "class extends" syntax. Not sure how to address this until we have a native communication via DevTool API. Perhaps there is a set of workarounds to snatch the derived construction like this, it won't be pretty. |
This is what happens in M57: const _gainNodeConstructor = GainNode.prototype.constructor;
GainNode.prototype.constructor = function () {
console.log('GainNode constructor called.');
_gainNodeConstructor(...args);
};
class VolumeNode extends GainNode {
constructor(context) {
console.log('VolumeNode constructor called.');
super(context);
}
}
let context = new AudioContext();
let vnode = new VolumeNode(context);
console.log(vnode.constructor.name);
console.log(Object.prototype.toString.call(vnode)); And the console says: "VolumeNode constructor called."
"VolumeNode"
"[object GainNode]" So the parent constructor is being invoked implicitly, basically bypassing the traced constructor. ("GainNode constructor called." is missing.) This might be the IDL/implementation bug in our constructor, or perhaps this is how it is specced. It needs further investigation. |
Ok, sorry to give you more work guys ;-) |
Per Hongchan, the current tracing logic seems to break with ES6 subclassing. The problem seems to be how the constructor overrider returns an AudioNode instead of setting properties on At the outset, I see no fixes within ES5. One solution I think would be to make closure compilation use ES6 for tracing.js. And then, make the tracing logic extend native
More investigation is indeed needed. Thank you for noticing this! More folks will start using ES6 features, so this issue is important. |
@micbuffa Thanks for catching this! |
I did some digging on how to trace ES6 classes inheritance and so far I only have more bad news.
|
Both 1 and 2 indeed seem difficult. :( I took your snippet above and played with it a lot. It seems like we can still supporting extending with ES5 if we use the "new" keyword to create the node. For instance, this works I think:
After that code runs, However, this still succumbs to how we must deal with each node type 1 by 1, which is manual, albeit I don't think I can come up with a more concise solution. Maybe it's worth it? |
In short, the problem is that this schmorgasborg of a hack isn't working. We got to use the new keyword. |
Never mind - the above won't work because calling |
Oh wait wait wait, I think using
|
Oh no, it doesn't work. None of the properties are actually defined on the node:
|
Another idea: We do not bother to trace AudioNode constructors. Instead, on The downside to this idea is that we don't track nodes that connect to nothing and have nothing connecting to them. IMO, this is tolerable because isolated don't play a role in audio graphs anyway. It might be worth doing that to support ES6, which come to think seems important. |
FWIW, Firefox's (excellent) web audio editor seems to err for The editor perpetually hangs on "Waiting for an audio context to be created ...", albeit the visualization runs correctly: Per above, it might be OK to make a compromise and avoid tracking constructors (to avoid major ES6 side effects). |
I disagree. It helps you find orphan nodes in your code. If you see something unconnected, that means the code needs to be fixed. I will bring these questions to ES6 experts. |
I can see the benefit of seeing orphan nodes. Overall, I'm not sure if we have a perfect solution. Any input from ES6 gurus would be great! |
Definitely want to see orphan nodes in the graph. Having orphans isn't an error (unless they stay that way forever). |
Got it. In that case, it seems like we ought to track AudioNode constructors. The question then becomes how we can make that jive with ES6. |
Any news? |
Unfortunately, not yet. I'm not sure how to quietly override ES6 class constructors. Maybe there is an imperfect way (with subtle side-effects), but we are still looking. |
Ok, thanks... |
@chihuahua Hey, this indeed seems to be a really tricky one. Was just wondering, why do you need to override the constructor in the first place? To be able to trace across connections, shouldn't it be sufficient to override the connect/disconnect methods with spy-ones that will track each patch and then just call the original implementation - and then keep all that state in a singleton outside of all the nodes? You shouldn't really have to deal with any instance of any Probably I should first read up on your code here, but maybe you can give me a quicker insight? That'd be nice! I am thinking of writing a webaudio graph tracer myself (not as a browser extension, but for the Tone lib) and maybe you can already tell me about my preconception mistakes ;) Thanks! |
@tchakabam, good question. I think #73 (comment) should shed some light: Overriding constructors lets us find orphan nodes. Writing a tracer for the toneJS library sounds useful. :) |
Hi! Any news about this bug? Its quite enoying, as we're subclassing AudioNode instances a lot in our code (mainly AudioWorklet but not only this one) and this breaks the extension each time, making our webapps very difficult to debug. If I can help... ? |
Hmm, Web Audio Inspector's been used for a while now. We override constructors since doing so lets us find orphan nodes. @hoch, have users found that feature useful? I wonder if it's worth avoiding overriding constructors and lazily identifying nodes during calls to |
@micbuffa Surely you can contribute. Feel free to submit a PR! @chihuahua I have no stat on the extension at all. Do you have any metric to follow? FWIW, I started to plan the native DevTool integration with WebAudio. Hopefully I will have something to demo soon. |
...was just passing through and noticed this thread. Is this what you're looking for?
Sadly this does not affect |
Hi, this example: https://codepen.io/w3devcampus/pen/GWGdjK?editors=0010
A frequency analyser I wrote, that uses an ES6 class that inherit from the AnalyserNode, stops working when the extension is activated. If I disable the extension it works (works also with FF, opera etc.)
It seems that the fact that the class inherits from a WebAudio node confuses the extension/browser, and raises erros when I try to call internal methods from the code in the class.
The text was updated successfully, but these errors were encountered: