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

Make AudioWorkletProcessor a class (fixes #24) #25

Closed

Conversation

hgcummings
Copy link

Fix for issue #24

@googlebot
Copy link

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

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

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@developit
Copy link
Collaborator

developit commented May 16, 2020

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

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.

@hgcummings hgcummings closed this May 17, 2020
@hgcummings hgcummings deleted the AudioWorkletPrototype branch May 17, 2020 12:43
@charlieroberts
Copy link

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

hgcummings commented May 27, 2020

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

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

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.

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

Successfully merging this pull request may close these issues.

4 participants