GoogleChromeLabs / audioworklet-polyfill

🔊 Polyfill AudioWorklet using the legacy ScriptProcessor API.
https://googlechromelabs.github.io/audioworklet-polyfill/
Apache License 2.0
195 stars 20 forks source link

Make AudioWorkletProcessor a class (fixes #24) #25

Closed hgcummings closed 4 years ago

hgcummings commented 4 years ago

Fix for issue #24

googlebot commented 4 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

hgcummings commented 4 years ago

@googlebot I signed it!

googlebot commented 4 years ago

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

developit commented 4 years ago

Thanks for the PR! We might need to use a hack here to fall back to a function in older browsers. Currently I think the class will actually end up being transpiled to a function by this project's build script, but I can tweak that.

hgcummings commented 4 years ago

Ah, on closer inspection I think this PR is unnecessary...

Functions would have a prototype object anyway, so a plain old function should be fine.

I saw this error when working from source to implement #26 , but I wasn't being consistent with how you're transpiling your distributed package, so my build chain was leaving this line as-is, which I think results in a named lambda (which has no prototype property) rather than a plain old JS function (which has a prototype property and works just fine).

Re-tested on iOS Safari 12. Nothing to do here.

charlieroberts commented 4 years ago

@hgcummings I also had this problem. I fixed it by making a function and assigning an empty object to its.prototype property, and then assigning this function to the AudioWorkletProcessor member of the context. Your fix would have saved me a lot of debugging... are you sure this should be closed? I don't quite follow what you mentioned about transpiling.

hgcummings commented 4 years ago

@charlieroberts are you referencing the distributed minified version (e.g. from npm) or the raw source (e.g. from GitHub)? I only had this problem with the latter.

The issue was the following expression:

          AudioWorkletProcessor () {
            this.port = nextPort;
          }

The transpiler/bundler used to generate the distributed version of audioworklet-polyfill (microbundle) turns the above expression into a plain-old JS function, which has a prototype property. The transpiler/bundler I was using (rollup) turned the above expression into a lambda function (which doesn't have a prototype property).

The above expression is pretty weird-looking to me. I don't recognise it as valid syntax for declaring a function, class, or lambda. Which might explain why different transpilers interpret it differently.

My change to explicitly declare a class was unnecessarily disruptive (materially altered the output of microbundle). A change like yours to explicitly declare a function would be safer (wouldn't materially alter the output of microbundle, and should make other transpilers match it more closely), and is probably still worth submitting as a PR.

charlieroberts commented 4 years ago

Yes, I was using the raw code, and I got this error even without any modifications from my bundler (Browserify). At this point my version is getting kinda far from the polyfill (for example, supporting variable buffer sizes to mimic the AudioContext.latencyHint behavior) and I'm not familiar with microbundle (looks nice!) so it might take some time to create a tested pull request, but I'll add it to the list :) Thanks for the explanation @hgcummings !

hgcummings commented 4 years ago

Yeah, I think that matches what I saw. When I said "my bundler turns this expression into a lambda function" I think actually my bundler wasn't touching this expression at all, and Safari was interpreting it as a lambda declaration. Like I say, I don't recognise it as valid pure ES, at least not in older ES versions, so I'm not surprised it gets interpreted a bit inconsistently.