-
-
Notifications
You must be signed in to change notification settings - Fork 588
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 wasm plugin work with service workers #334
Conversation
The code generated by @rollup/plugin-wasm breaks when executed in a service worker because window is undefined.
Thanks for taking this on. We'll leave this open and pending until you give the go-ahead to review. |
Instead of checking whether window is defined, we can just use globalThis.
@shellscape I added a test and refactored test.js because I needed to pass the code generated by rollup into a Note: The test is a bit fragile because I have to fake the browser environment ( The test can only run on node versions where
|
Thanks for the PR! @ColinEberhardt and @jamen would you two be willing to take a look at this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The worker test does seem a bit hacky, but I don't have a better idea, so its probably fine. The change affecting the other tests is functionally the same, so that's fine.
It should probably be added in a major version because there is a gap of support between WebAssembly and globalThis according to caniuse (1, 2).
Looks okay to me. 👍
@jamen The gap of support seems to show that WebAssembly is less supported than |
What I mean is the browser versions. WebAssembly came first and globalThis came later, so there are versions where WebAssembly exists but globalThis doesn't. I believe that is a breaking change, because the build would be supported in a more limited range of versions than WebAssembly itself. I don't think it needs changed, but it should be in a major version so it wouldn't surprise existing builds. I haven't been very active in web lately though, so someone else can make that call. But also, a build concerned about this should be using something like |
Thanks everyone! |
BREAKING CHANGES: This change leverages `globalThis` and will fail in older environments where `globalThis` is not implemented. See https://caniuse.com/#search=WebAssembly and https://caniuse.com/#search=globalThis. If your environment doesn't support `globalThis`, please use an earlier version of this plugin. * Make wasm plugin work with service workers The code generated by @rollup/plugin-wasm breaks when executed in a service worker because window is undefined. * Add test for worker scenario and simplify the fix Instead of checking whether window is defined, we can just use globalThis. * Only run worker test if worker_threads supported
The "target environment browser" test invoked the bundled output, which leads to an undefined error on node 14 as [`atob` was introduced in node 16][0]. This commit uses the same atob polyfill that was introduced in rollup#334 [0]: https://developer.mozilla.org/en-US/docs/Web/API/atob#browser_compatibility
The "target environment browser" test invoked the bundled output, which leads to an undefined error on node 14 as [`atob` was introduced in node 16][0]. This commit uses the same atob polyfill that was introduced in #334 [0]: https://developer.mozilla.org/en-US/docs/Web/API/atob#browser_compatibility
The code generated by @rollup/plugin-wasm breaks when executed in a
service worker because window is undefined.
Rollup Plugin Name:
wasm
This PR contains:
Are tests included?
This is a draft PR because I'm not sure how to write a test that runs code inside a service worker. Once I figure that out, I will add a test and update the above checkbox.Breaking Changes?
List any relevant issue numbers: #333
Description
This PR adds a check for whetherThis PR makes the loader function usewindow
is undefined.globalThis
instead ofwindow
. Prior to this,@rollup/plugin-wasm
would break when the resulting generated code was run in a service worker, because it assumedwindow
was defined.