-
-
Notifications
You must be signed in to change notification settings - Fork 599
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
Option to not inline wasm #543
Conversation
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.
Looks really good, thank you for stepping up to make this happen. I do have some suggestions for changes however.
Now that these initial changes are done: Do we need |
I don't see a need right now. Those options in the |
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.
One suggestion, but otherwise LGTM!
The import { nodeResolve } from '@rollup/plugin-node-resolve';
import commonjs from '@rollup/plugin-commonjs';
import { wasm } from '@rollup/plugin-wasm';
export default {
input: 'src/main.js',
output: {
file: 'dist/bundle.js',
format: 'commonjs',
},
plugins: [
nodeResolve(),
commonjs(),
wasm({ maxFileSize: 0, publicPath: 'assets/' }),
],
}; |
Rollup Plugin Name:
WASM
This PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
Discussed in #324.
Description
This PR adds support for not inlined WASM files. Currently WASM files are always base64 encoded, which increases size by > 30%. There's some discussion in #324. This PR addresses this issue by adding a
limit
andpublicPath
option like in the URL plugin. E. g.would mean that no WASM files are inlined and all are copied to the destination folder and loaded via
fs
(in Node) orfetch
(in the browser) at runtime.Implementation
1.) Switching between
fs
andfetch
I decided to include code for both and decide at runtime what to execute. The other option would be having a plugin flag for the target platform. But I opted for this since I didn't see a downside besides the negligible code size impact.
One noteworthy detail: I loaded
fs
viaeval('require("fs")')
to avoid warnings like darionco/rollup-plugin-web-worker-loader#20. Of courseeval
is typically bad but I don't see an issue here.2.) Path and name of the output WASM files
I handled file generation similarly to the URL plugin. The differences are:
emitFiles
option.fileName
option, instead the output file name is always[hash].wasm
.destDir
andsourceDir
options.Let me know if you think these are needed here as well. I didn't add them now because I wanted to make sure that my approach is valid in principle first.
Tests
I added one test which:
fixtures/complex.js
as CJS module.output/bundle.js
with its assertions.