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

Option to not inline wasm #543

Merged
merged 9 commits into from Sep 6, 2020
Merged

Conversation

bminixhofer
Copy link
Contributor

@bminixhofer bminixhofer commented Aug 16, 2020

Rollup Plugin Name: WASM

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

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 and publicPath option like in the URL plugin. E. g.

wasm({
  limit: 0,
})

would mean that no WASM files are inlined and all are copied to the destination folder and loaded via fs (in Node) or fetch(in the browser) at runtime.

Implementation

1.) Switching between fs and fetch

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 via eval('require("fs")') to avoid warnings like darionco/rollup-plugin-web-worker-loader#20. Of course eval 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:

  • I did not add an emitFiles option.
  • I did not add a fileName option, instead the output file name is always [hash].wasm.
  • I did not add destDir and sourceDir 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:

  • bundles fixtures/complex.js as CJS module.
  • asserts that the [hash].wasm file is present in the output folder.
  • Runs the output/bundle.js with its assertions.

Copy link
Collaborator

@shellscape shellscape left a 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.

packages/wasm/README.md Outdated Show resolved Hide resolved
packages/wasm/src/index.ts Outdated Show resolved Hide resolved
packages/wasm/src/index.ts Outdated Show resolved Hide resolved
@bminixhofer
Copy link
Contributor Author

Now that these initial changes are done: Do we need emitFiles, fileName, sourceDir, and destDir options like in the URL plugin? (e. g. https://github.com/rollup/plugins/tree/master/packages/url#emitfiles). Would be easy to implement but I'm not sure how useful they are.

@shellscape
Copy link
Collaborator

Do we need emitFiles, fileName, sourceDir, and destDir options like in the URL plugin?

I don't see a need right now. Those options in the url plugin came about from user feature requests. Unless there's a call from the userbase for options on where to emit the files, I think you're good to go.

Copy link
Contributor

@pnevares pnevares left a 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!

packages/wasm/src/index.ts Outdated Show resolved Hide resolved
@shellscape shellscape merged commit 0e747cd into rollup:master Sep 6, 2020
@entrptaher
Copy link

entrptaher commented May 6, 2021

The destDir is needed if we want to put the generated wasm files in seperate directory

image

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/' }),
  ],
};

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.

None yet

4 participants