-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
PoC for inline target #4065
base: main
Are you sure you want to change the base?
PoC for inline target #4065
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.
See my PR #4027 for the Node target and #2176/#3990 for the Deno target in regards to tests and other target-addition changes.
I was going to refactor the target generation a bit, but this change is small enough where it doesn't matter whether this PR or mine gets merged first. I'll ping you once I open it in either case.
crates/cli/src/bin/wasm-bindgen.rs
Outdated
@@ -110,6 +110,7 @@ fn rmain(args: &Args) -> Result<(), Error> { | |||
"nodejs" => b.nodejs(true)?, | |||
"deno" => b.deno(true)?, | |||
"experimental-nodejs-module" => b.nodejs_module(true)?, | |||
"isomorphic" => b.isomorphic(true)?, |
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.
Since this is a proof-of-concept I think experimental-isomorphic
for the name makes more sense.
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.
- change cli option
|
||
for (id, js) in crate::sorted_iter(&self.wasm_import_definitions) { | ||
let import = self.module.imports.get_mut(*id); | ||
footer.push_str("\nmodule.exports."); |
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 use-cases all expect CommonJS?
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.
Good point. Technically it also makes sense for ecmascript modules but I can't think of a use case.
- check whether and how to support ecmascript modules
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.
I decided not to use ecmascript modules for now as I don't see a use case for this. The right way of mixing ecmascript modules with wasm should be importing wasm as esm.
crates/cli-support/src/js/mod.rs
Outdated
@@ -142,7 +142,7 @@ impl<'a> Context<'a> { | |||
self.globals.push_str(c); | |||
} | |||
let global = match self.config.mode { | |||
OutputMode::Node { module: false } => { | |||
OutputMode::Isomorphic | OutputMode::Node { module: false } => { |
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.
Why the name Isomorphic
?
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.
It was introduced in rustwasm/wasm-pack#1334 and I didn't question it, it was just a catchy name. The idea seems to be that the code should run unchanged (without polyfiils or other modification) in any context (node, browser, ...). But this goal is beyond what I am doing here. Here I just inline the assets that need (and effectively cannot atm) to be processed by a bundler.
I would rather call it node-universal
. Basically because that's basically what it should do: You can build your Rust code with wasm-pack
and include it in your typescript application (e.g. react, aws lambda, node express) without worrying about the bundling.
What do you think?
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.
Sorry for disturbing you here.
I previously facilitated the sharing the same WebAssembly file across of cjs/esm/bundler through manual adjustments in this JohnnyMorganz/StyLua#848 .
I also shared my concept in #3790, which supports various environments, including browsers, Node, Deno, Bun, and bundlers.
I'm not certain about your reference to include it in your typescript application (e.g. react,
but include CJS into the web project is challenging. We faced this issue in swc-project/swc-playground#42, and finallay, we had to create a separate esm to fulfill the requirements.
Inline WASM is viable, but I don't believe it can serve as a direct alternative to the current web/node/deno/bundler option. Instead, this option should complement them, being usable independently and applicable across all environments.
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.
Sorry for disturbing you here.
Thank you for your valuable insights!
I previously facilitated the sharing the same WebAssembly file across of cjs/esm/bundler through manual adjustments in this JohnnyMorganz/StyLua#848 . I also shared my concept in #3790, which supports various environments, including browsers, Node, Deno, Bun, and bundlers.
I'm not certain about your reference to
include it in your typescript application (e.g. react,
but include CJS into the web project is challenging. We faced this issue in swc-project/swc-playground#42, and finallay, we had to create a separate esm to fulfill the requirements.
Maybe I am missing something here (I am actually using wasm only in AWS Lambdas for now). Is there an issue with using the npm package generated by the Isomorphic
target in an arbitrary react app?
Inline WASM is viable, but I don't believe it can serve as a direct alternative to the current web/node/deno/bundler option. Instead, this option should complement them, being usable independently and applicable across all environments.
I totally agree. My focus is also on the inline part not on the "usability across targets". I have been watching the asset handling issue in esbuild
, webpack
, rollup
, wasm-pack
, wasm-bindgen
and never came closer to the solution than writing custom plugins plus a bunch of scripts. So as an intermediate solution I would propose inline wasm until every target just speaks wasm as esm module either directly or with any bundler of my choice without even having to change configuration.
Summing this up, I think I should just focus on an "inline node" target in this PR, moving the portability issues to the future when someone actually needs this.
216acf3
to
f9f1eff
Compare
I'm sorry this isn't a very substantial contribution, but the name "isomorphic" is rather confusing to me. If I understand, the idea is to make the code identical for browsers and other JS engines (which I'd really love to see), whereas "isomorphic" refers in general to things that are equivalent but not necessarily identical? Would it make more sense to call the code something more like "cross-compatible"? |
"cross-compatible" is not very specific. I would prefer not to invent a new industry term. This behavior is generally known within the developer community as isomorphic builds. Consider it a term of art. |
Hmm, it seems you're right: https://en.wikipedia.org/wiki/Isomorphic_JavaScript That also states:
I certainly think "universal" is much less of a misnomer, but thanks for the context! |
f9f1eff
to
c57b5cf
Compare
I changed the target name to "inline"
It would be great to have an "universal" target (or if you want to call it like this: isomorphic). But the required changes would be much bigger than this tiny pull request. |
ed3294f
to
cb8a1f1
Compare
Out of curiosity I also checked browser compatibility: after a tiny fix basic functionality (meaning you can go the happy path and npm i a wasm project in your react app) seems to be there in browsers as well. A PoC is here: https://github.com/torfmaster/inline-wasm-react. |
crates/cli-support/src/js/mod.rs
Outdated
let mut serialized = "const bytes = new Uint8Array([".to_string(); | ||
let (last, bytes) = buf.split_last().unwrap(); | ||
for byte in bytes { | ||
serialized.push_str(&format!("{},", byte)); | ||
} |
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.
I know that this PR is a PoC, so please take this comment as me throwing around ideas.
I wonder if there's a better way to store the WASM bytes inline.
Especially in the web context, the time it takes to parse a JS file is significant. JS is an interpreted language, so the total time it takes to run a script is parse time + exec time
. JS is a complex language and JS parsers are equally complex. This is why often times, storing JSON data inside JS files as
const data = {...}
is slower than
const data = JSON.parse('{...}')
JSON is a simpler language and so it can be parsed much faster than JS. Basically, the time it takes to parse a JS object literal with all of JS's complexity is often more than the time it takes to parse a JS string literal and then parsing the string with a JSON parser.
So I believe that using new UInt8Array(JSON.parse("[1,2,3,...]"))
would be faster than the current approach. However, benchmarks need to be done.
There might also be more efficient ways (both in terms of runtime and data compression). Base64 was my first thought, but I'm not aware of any fast and compatible way to do that in pure JS. NodeJs has Buffer.from("...", "base64")
, but then generated files would be truly nodejs-only.
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.
I just found this, so base64 might be an option.
wasm-bindgen/crates/cli-support/src/wasm2es6js.rs
Lines 235 to 246 in 4765448
format!( | |
" | |
let bytes; | |
const base64 = \"{base64}\"; | |
if (typeof Buffer === 'undefined') {{ | |
bytes = Uint8Array.from(atob(base64), c => c.charCodeAt(0)); | |
}} else {{ | |
bytes = Buffer.from(base64, 'base64'); | |
}} | |
", | |
base64 = BASE64_STANDARD.encode(&wasm) | |
), |
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.
Thank you for your reply. I agree with your observation and with your fix. Hence, I would
- write a polyfill for base64 decode
- use this to decode the wasm binary blob
It shouldn't be to hard to accomplish and I will write the fix as soon as I find the time (essentially the time for testing).
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.
I implemented the base64 alternative (re-using the existing code). It works well in nodejs as in the browser. I don't know yet how to easily measure the startup performance as for usual wasm payloads the contribution of parsing the js code seems to be below noise. Maybe, I'll do some experiments with larger (non-wasm) payloads.
323f2fa
to
118319b
Compare
118319b
to
006b373
Compare
This PR adds an "isomorphic" or "inline" target next to the nodejs target as a proof of concept. It targets simple use cases as using small snippets of Rust Code in larger typescript codebases. In detail it allows (with a proposal that I will create for wasm-pack) creating npm packages that ship wasm modules inline.
Background
Using Rust code via wasm in existing nodejs typescript applications is challenging. It usually involves changing the bundler configuration and currently involves handling wasm artifacts.
The motivating example for a typescript lambda built via nx and bundled via esbuild (which is the de-facto standard for AWS Lambda in CDK). After a lot of experimentation (involving using different bundlers and writing plugins) I found no obvious way for my rather easy use case (a few lines of Rust Code embedded via wasm). Hence I
Relation to other issues
This touches at least #2265, rustwasm/wasm-pack#1334, rustwasm/wasm-pack#831.
Testing
I currently only did some rough manual testing. Once I know that this PR is somehow useful I can invest more.