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

Build with a .wasm file instead of minified+compressed+inlined wasm #92

Open
miunau opened this issue Aug 28, 2023 · 6 comments
Open

Build with a .wasm file instead of minified+compressed+inlined wasm #92

miunau opened this issue Aug 28, 2023 · 6 comments

Comments

@miunau
Copy link

miunau commented Aug 28, 2023

Hi,
I'm working with Cloudflare Workers, and in that environment you can't dynamically load wasm from base64 or gzipped data- just the import wasmContent from './wasmfile.wasm' style of importing. I've mangled a local version of the build to work with this but I was wondering if it'd make sense to try to work this upstream into your repo somehow, maybe as a different export inside each package? Thanks!

@eshaz
Copy link
Owner

eshaz commented Aug 28, 2023

That might be a good feature, but the built in wasm code is heavily integrated throughout this library. What's the limitation with Cloudflare Workers that doesn't allow this? If there's some error, it might be easier to fix that than to make this change.

@miunau
Copy link
Author

miunau commented Aug 28, 2023

Yes, I realize it would require an entirely different build script, that's what I've done locally. The workers environment simply does not allow instantiating from an arbitrary byte source- they want their compiler to evaluate the .wasm file. I think some other hosts might have this limitation, possibly Fastly.

One solution might be to make it possible to pass in the .wasm file when instantiating a Decoder, and use that instead of the inlined one. The asm exports would have to be exactly the same, though, so it would be beneficial to have a build script that produces both artifacts (or rather, doesn't delete the original .wasm file after inlining). That way I could import wasmFile from flac-decoder/wasm and feed that into the decoder constructor.

@miunau
Copy link
Author

miunau commented Aug 28, 2023

Ah, there's another problem that I just remembered that is related to CF: There is (ironically) no Worker global so I've had to remove the worker exports from each local package. Would it make sense to split these off into their own exports so tree shaking can take care of it? I can make an example fork and another issue if you'd like to see how I'd structure it.

@eshaz
Copy link
Owner

eshaz commented Aug 28, 2023

The workers environment simply does not allow instantiating from an arbitrary byte source- they want their compiler to evaluate the .wasm file.

Is this environment using NodeJS or something else? Do you have a link to a doc on how they expect WASM to compile? This seems very unusual to me. Either way you're supplying the same WASM code to the compiler when calling WebAssembly.compile() regardless of where it comes from...

One solution might be to make it possible to pass in the .wasm file when instantiating a Decoder

If you want to put together a fork or example with this that would be helpful. I think your idea of another export would work. I don't want to increase the export size of the package unless at a last resort, so being able to tree shake this out would be ideal.

There is (ironically) no Worker global so I've had to remove the worker exports

I have some code that toggles on a NodeJS Worker polyfill if it doesn't exist on the globalThis object, i.e. where it is on browsers. Could you test this code out in your Cloudflare Worker environment and let me know where the error is?

const getWorker = () => globalThis.Worker || NodeWorker;

@miunau
Copy link
Author

miunau commented Aug 28, 2023

Is this environment using NodeJS or something else? Do you have a link to a doc on how they expect WASM to compile? This seems very unusual to me. Either way you're supplying the same WASM code to the compiler when calling WebAssembly.compile() regardless of where it comes from...

This is the environment: https://github.com/cloudflare/workerd

They don't really document the limitation anywhere. The error it gives is CompileError: WebAssembly.instantiate(): Wasm code generation disallowed by embedder, but it also blocks eval() so I imagine it's some kind of security guard.

One solution might be to make it possible to pass in the .wasm file when instantiating a Decoder

If you want to put together a fork or example with this that would be helpful. I think your idea of another export would work. I don't want to increase the export size of the package unless at a last resort, so being able to tree shake this out would be ideal.

Yes, using separate exports in package.json would achieve this. I'll put together a fork.

I have some code that toggles on a NodeJS Worker polyfill if it doesn't exist on the globalThis object, i.e. where it is on browsers. Could you test this code out in your Cloudflare Worker environment and let me know where the error is?

const getWorker = () => globalThis.Worker || NodeWorker;

Unfortunately no, it seems to choke even at the mention of any global scope objects that don't exist, so this kind of feature sniffing doesn't work. The separate export I think is the only way.

@eshaz
Copy link
Owner

eshaz commented Aug 28, 2023

They don't really document the limitation anywhere. The error it gives is CompileError: WebAssembly.instantiate(): Wasm code generation disallowed by embedder, but it also blocks eval() so I imagine it's some kind of security guard

I think there should be an issue in their repo for this. Either way, the developer provides the same code whether in a file or as a byte array, so I don't see what this would protect from a security standpoint. At the minimum, they should update their docs :) It looks like that project is still in beta, so now would be a good time to get this fixed.

it seems to choke even at the mention of any global scope objects that don't exist

A try / catch might work here. If you want to try that, and put in a PR if it works, that would be awesome.

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

No branches or pull requests

2 participants