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

Adds support for Rust/wasm assets via wasm-pack #4970

Open
wants to merge 42 commits into
base: v2
Choose a base branch
from

Conversation

mysterycommand
Copy link

@mysterycommand mysterycommand commented Aug 3, 2020

↪️ Pull Request

This PR is a first implementation of support for Rust/wasm assets via wasm-pack in Parcel 2. It's based on parcel-plugin-wasm-pack for Parcel 1 (which I also wrote). It has been discussed in #4422 (comment) and in #3365.

💻 Examples

There are examples in packages/examples/wasm-single-crate and packages/examples/wasm-multiple-crates. They both use an index.html as the entry for parcel build/serve. The html adds a js script file which does something like import {run} from '../Cargo.toml';.

I updated packages/configs/default/index.json so that imports of Cargo.toml flow through a new WasmPackTransformer which runs wasm-pack and generates a loader and some glue code to wire the wasm module and it's imports object together.

Probably the most "controversial" part of this PR are some changes to JSPackager that are based on the original plugin implementation's WasmPackPackager which registered itself to handle JS files in the plugin implementation. I wasn't sure if (now that I'm contributing to core) there was a better way/place to make these changes, but this seems to work at least … as a starting point.

🚨 Test instructions

I am very open to ways of making the integration test better/more meaningful … it's really just a scaffold right now that asserts the names of the generated bundle's assets. Weirdly, it appears that one of the bundles includes the JSRuntime asset twice … is that normal/expected? Seems weird to me.

That said, both the examples mentioned above work, and navigating into one of those folders and running yarn build or yarn start will generate a valid bundle that logs something to the console from the wasm module.

✔️ PR Todo

  • Added/updated unit tests for this change <-- need help!
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

… them from transform, now just gotta wire it all together
…re like integration tests just for comparison
@height
Copy link

height bot commented Aug 3, 2020

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

💡Tip: You can also use "Close T-X" to automatically close a task when the pull request is merged.

@devongovett
Copy link
Member

This looks great! Thanks for working on it. Will look more in depth soon. 😄

@mischnic
Copy link
Member

mischnic commented Aug 5, 2020

Which of the problems that you ran into and mentioned in the various other issues are still relevant?

Weirdly, it appears that one of the bundles includes the JSRuntime asset twice … is that normal/expected? Seems weird to me.

You mean according to --detailed-report (or something similar)? That is normal, i.e. every dynamic import is replaced with a small asset that always has JSRuntime.js as it's path:

filePath: __filename,

@mysterycommand
Copy link
Author

mysterycommand commented Aug 7, 2020

Which of the problems that you ran into and mentioned in the various other issues are still relevant?

The issues described here: #3365 (comment) (apart from item 1) are largely unaddressed because I find myself agreeing with @qwerty2501's conclusion here: #3365 (comment) (mainly that, as an MVP it should be a thin wrapper around "vanilla" wasm-pack behavior).

@Pauan's comments here: #3365 (comment) are mostly addressed with the exceptions of the first and last items there. In the case of the first, I think I prefer to use wasm-pack's verbose output in Parcel's logger.progress when running wasm-pack build over a "crate".

In the case of the last, I think unless we can target ES Modules we need to do some work on wasm-pack's output to fetch and instantiate the wasm module, and to create the imports object … indeed removing this boilerplate appears to be one of the key motivations behind the WebAssembly/ESM integration spec, but I'm pretty sure it's not yet widely supported.

You mean according to --detailed-report (or something similar)? That is normal, i.e. every dynamic import is replaced with a small asset that always has JSRuntime.js as it's path:

According to the integration test … I do like this:

await assertBundles(b, [
  {
    assets: [
      'browser-loader.js',
      'bundle-manifest.js',
      'bundle-url.js',
      'Cargo.toml',
      'index.js',
      'JSRuntime.js', // <-- one JSRuntime
      'JSRuntime.js', // <-- two JSRuntimes
      'relative-path.js',
      'single_bg.js',
    ],
  },
  {
    assets: ['single_bg.wasm'],
  },
]);

As far as what's left goes, I think just getting that integration test a little more robust? Possibly adding a NodeJS-targeted integration test and a multi-crate integration test?

Any feedback on the changes to JSPackager.js would be welcome as well.

Also, it looks to me like the test failures are coming from unrelated tests … obv. should figure out what's going on there.

@Pauan
Copy link

Pauan commented Aug 7, 2020

@mysterycommand It is not necessary to have ESM integration, instead you can use --target web. This is what the Rollup plugin does. In that case the boilerplate, fetching, and imports object are all handled by wasm-pack.

If you use --target web then you would no longer need the packages/transformers/wasm-pack/src/loaders/browser-loader.js or packages/transformers/wasm-pack/src/loaders/node-loader.js files.

@mysterycommand
Copy link
Author

mysterycommand commented Aug 7, 2020

@Pauan super interesting, thanks! You're reminding me of a couple of things that may or may not be issues for using --target web in the wasm-pack build args. In a first draft of parcel-plugin-wasm-pack I actually split the wasm-pack --target (bundler, web, nodejs, or no-modules) based on the parcel --target (browser, electron, or node).

If we could split it up this way we wouldn't need the loaders (you're right … in fact, even in the case of the Rollup plugin I think using wasm-pack --target nodejs would let you avoid adding the loadFile function to the asset prelude since the wasm-pack-generated entry does that for you as well in that case, though synchronously).

However, I think there's a couple things I'm not so jazzed about with this approach:

  1. In Parcel, I believe JS is passed through Babel and I think it still needs a plugin for import.meta (which is used by the init function generated by wasm-pack --target web … I guess you don't have this issue in Rollup since it has a more "ESM first" approach?
    • if we want to do it this way, maybe we can just auto-install that plugin?
  2. In the case of multiple crates being compiled to wasm and imported separately, the wasm-pack-generated module includes a (duplicate) loader for each module. This is probably not a huge deal, just a personal annoyance of mine.
  3. The usage I'd like to achieve involves loading the wasm, and swapping them out in the bundle cache before running the entry (the "controversial" stuff in JSPackager) … all this to make it so that someone working with Rust/wasm can do like (for example):
    import { greet } from './Cargo.toml';
    
    greet();
    
    … or in the case of multiple crates like:
    import { hello } from './crates/hello/Cargo.toml';
    import { world } from './crates/world/Cargo.toml';
    
    hello();
    world();
    

Super open to feedback on all of this … my use case/desired usage might not be in exactly in line with Parcel's? I'm really not sure what other users/authors might want or expect either.

@mischnic
Copy link
Member

mischnic commented Aug 7, 2020

I think it still needs a plugin for import.meta

That's enabled by default since Babel 7.10. Though we probably need to handle it in Parcel itself: #3269

@Pauan
Copy link

Pauan commented Aug 8, 2020

@mysterycommand I personally would not recommend using --target nodejs, because it is synchronous. Instead you can use --target web with NodeJS, you just need to pass a Promise<Buffer> to the init function:

init(promisify(readFile)(wasmPath))

This works because the init function accepts a string or a Buffer/ArrayBuffer/Uint8Array (or a Promise which resolves to one of those).

This allows you to load the .wasm however you wish, while still having the initialization/imports object handled by wasm-bindgen.

In the case of multiple crates being compiled to wasm and imported separately, the wasm-pack-generated module includes a (duplicate) loader for each module. This is probably not a huge deal, just a personal annoyance of mine.

That's unavoidable, since the imports object, JS heap, and linear memory must be different for each crate.

So the only things which could be deduplicated are a handful of small utility functions like cachedTextDecoder, cachedTextEncoder, and load. So the amount of duplicated code is small, just ~1 KB (which will become much smaller with compression).

Any sort of deduplication would have to be done in wasm-bindgen, I don't think this plugin is the right place to do it.

The usage I'd like to achieve involves loading the wasm, and swapping them out in the bundle cache before running the entry (the "controversial" stuff in JSPackager) … all this to make it so that someone working with Rust/wasm can do like (for example)

I don't know how Parcel works internally, and I don't know what Parcel plugins are capable of doing, so I don't know if that's possible without ESM integration or top-level await.

Rollup does not work with top-level await (it simply relies on the browser's implementation of top-level await), so that's why the Rust Rollup plugin needed to use a less convenient API. Whereas Webpack does transform top-level await, so it has great support for .wasm.

Comment on lines +7 to +10
[profile.release]
# Tell `rustc` to optimize for small code size.
# @see: https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections
opt-level = "s"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add more describes and lto example.

Suggested change
[profile.release]
# Tell `rustc` to optimize for small code size.
# @see: https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections
opt-level = "s"
# Customizing compiler settings when release build.
# @see: https://doc.rust-lang.org/cargo/reference/manifest.html#the-profile-sections
[profile.release]
# Tell `rustc` to optimize for small code size.
# @see: https://rustwasm.github.io/docs/book/reference/code-size.html#tell-llvm-to-optimize-for-size-instead-of-speed
opt-level = "s"
# Compiling with `link time optimizations`. It is more optimize and small code size. But compile time to be longer.
# @see: https://rustwasm.github.io/docs/book/reference/code-size.html#compiling-with-link-time-optimizations-lto
lto = true

@aminya
Copy link
Contributor

aminya commented Jan 6, 2021

Does this PR add anything reusable for #5598?

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.

6 participants