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

Javascript code does not work in ESM context #256

Closed
josephg opened this issue Mar 28, 2022 · 11 comments · Fixed by #284
Closed

Javascript code does not work in ESM context #256

josephg opened this issue Mar 28, 2022 · 11 comments · Fixed by #284

Comments

@josephg
Copy link

josephg commented Mar 28, 2022

Currently wasm-pack has two common build options:

  • nodejs produces a commonjs module for node
  • web produces an es6 module for the web

I'd like to use the same wasm module (published in npm) isomorphically for both nodejs and the web. It almost works - but unfortunately getrandom (run from nodejs) doesn't work at all in an es6 context. It produces this code in a esm file:

    imports.wbg.__wbg_modulerequire_0a83c0c31d12d2c7 = function() { return handleError(function (arg0, arg1) {
        var ret = module.require(getStringFromWasm0(arg0, arg1));
        return addHeapObject(ret);
    }, arguments) };

... Which errors, because module is not defined in this context.

I'm not sure how getrandom should work around this. Any thoughts?

@josephlr
Copy link
Member

josephlr commented Jun 7, 2022

We have run into a bunch of issues like this before (see #214 #224 #234). If you (or anyone else) have an improvement for our code, we would be happy to accept a PR. The current logic for selecting the implementation is here:

fn getrandom_init() -> Result<RngSource, Error> {

We have CI tests that make sure our code works w/ Web and NodeJS, so I'm confused as to why you are having issues. The code you have above will only be run in a Node environment. That function will not be called if you are running on a web browser.

@josephlr
Copy link
Member

josephlr commented Jun 7, 2022

Oh wait I think this issue was fixed in #234 as the generated JS bindings should now look like:

getObject(arg0).require(getStringFromWasm0(arg1, arg2))

not

module.require(getStringFromWasm0(arg0, arg1))

Can you make sure you are using the latest version of getrandom and see if your issue still persists?

@josephg
Copy link
Author

josephg commented Jun 9, 2022

We have CI tests that make sure our code works w/ Web and NodeJS, so I'm confused as to why you are having issues.

The code works in web and nodejs, so long as I compile separate wasm modules for each context. And thus, publish separate npm packages for the web and for nodejs. But I hate that - javascript code works on the web and nodejs (thats kind of the whole point of nodejs). Ideally I'd like to be able to just compile my wasm code once and run it in both a nodejs and web context.

And I almost can do that - but I just can't get getrandom to work in both contexts. I'm not sure what the right answer is here from a library design perspective.

The current steps I'm taking:

lib.rs:

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn foo() -> u32 {
    console_error_panic_hook::set_once();
    
    let mut bytes = [0; 4];
    getrandom::getrandom(&mut bytes[..]).expect("getrandom failed");
    u32::from_be_bytes(bytes)
}

Then:

wasm-pack build --dev --target web

Unfortunately I need to add "type": "module" into the generated package.json. Once I've done that I can load the wasm module from nodejs in a kinda manual, hacky way:

foo.mjs:

import {default as init} from "./pkg/rand_wasm.js"
import {readFileSync} from 'fs'

;(async () => {
    let bytes = readFileSync("pkg/rand_wasm_bg.wasm")

    let module = await init(bytes)
    console.log(module.foo())
})()

All my rust code runs fine - but the getrandom call fails:

$ node foo.mjs 
panicked at 'getrandom failed: Error { internal_code: 2147483660, description: "Node.js crypto module is unavailable" }', src/lib.rs:8:42
(stack follows...)

The reason is that the nodejs codepath is trying to use require(), which is unavailable when using es modules.

For now I've given up, and I'm just compiling my wasm module twice (once for node, once for the web). But ideally I'd like to get this fixed. Having a single wasm module which works everywhere would be lovely.

Any ideas on how we can make this work?

@josephg
Copy link
Author

josephg commented Jun 9, 2022

Maybe in the long term I could target WASI instead, which provides an API for random. Thats probably a long ways out though :/

Another approach is to detect nodejs then use a dynamic import if we're in ESM mode -

let crypto = await import('node:crypto').webcrypto
crypto.getRandomValues(buffer)

... But dynamic imports return a promise, and thats really hard to cope with. :/

@tjjfvi
Copy link

tjjfvi commented Jun 11, 2022

A cheap workaround is to add this to the end of the generated bindings file:

// workaround for https://github.com/rust-random/getrandom/issues/256
import * as crypto from "crypto"
const module = {
    require: (string) => {
        if(string !== "crypto") throw new Error("Unexpected require " + string)
        return crypto
    }
}

@josephlr
Copy link
Member

josephlr commented Jun 17, 2022

So it seems like we cannot support both ES6 modules running on Node.js and wasm_bindgen's --target nodejs which uses CommonJS modules. I'll try to get some concrete code examples together this week/weekend.

Basically, as the object returned by import() is a Promise and we have no way to "run" this Promise, we can't actually use import() as a function call. We can convert between Promises and Futures
but there's not a way to execute them. We could get around this by using top-level await but that doesn't work with CommonJS. This would also mean losing support for many older environments which currently work.

So if we want to support this use-case, we would have to add an additional js_modules Cargo feature that:

  • Allows getrandom use features exclusive to ES6 modules
  • Node.js + ES6 now works
  • Makes getrandom no longer work in older JS environments
  • Makes getrandom incompatible with wasm_bindgen's --target nodejs and --target no-modules
  • Only --target web and --target bundler would be supported

The code works in web and nodejs, so long as I compile separate wasm modules for each context. And thus, publish separate npm packages for the web and for nodejs. But I hate that - javascript code works on the web and nodejs (thats kind of the whole point of nodejs). Ideally I'd like to be able to just compile my wasm code once and run it in both a nodejs and web context.

The main issue here is that Node and the Web really are different targets, even if they are superficially similar. We've had a ton of issues targeting Node.js in general (targeting the Web has been bad but not as bad). This library attempts to support every Rust target possible, and Node.js has easily has the highest maintenance cost, due to:

  • A lower level of support in wasm_bindgen
  • The fractured module ecosystem (CommonJS modules vs ES6 modules vs whatever was before ES6)
  • global.foo and foo not being being the same (or one will exist and the other will not)
  • We need to conditionally import a Node.js library (unlike the Web which needs no library imports)
  • Node.js conditional import story is half-backed:
    • CommonJS require() is only synchronous
    • ES6 import() is only asynchronous
    • Node.js's only ES6-compatible way to do blocking imports requires importing another Node.js specific library, making this "solution" circular.

@josephg
Copy link
Author

josephg commented Jun 21, 2022

I've had a bit of a think about it, and I want to suggest another approach:

So right now, the logic is something like this:

if (process.versions.node != null) {
  require('crypto').randomFillSync(...)
} else {
  globalThis.crypto.getRandomValues(...)
}

There are essentially 3 different environments:

  1. Nodejs in commonjs mode (supported)
  2. Nodejs in esm mode (unsupported - require is not available)
  3. Web browsers in any mode (supported)

If we invert the logic to do capability detection instead, it would look like this:

if (globalThis.crypto.getRandomValues != null) {
  globalThis.crypto.getRandomValues(...)
} else if (globalThis.require != null) { // Or keep the existing check for process.version.node
  require('crypto').randomFillSync(...)
}

Then I could make my code work with a simple shim:

import {webcrypto} from 'crypto' // only available from node 16+, but thats not a problem for me.
globalThis.crypto = webcrypto

// ...
wasm.init()

This would also work in deno, and any other javascript context which follows web standards. And if nodejs ever implements the web's JS crypto standard, everything will just work.

josephlr added a commit that referenced this issue Sep 13, 2022
Now we look for the standard Web Cryptography API before attempting to
check for Node.js support. This allows Node.js ES6 module users to add
a polyfill like:
```js
import {webcrypto} from 'crypto'
globalThis.crypto = webcrypto
```
as described in #256 (comment)

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit that referenced this issue Sep 13, 2022
Now we look for the standard Web Cryptography API before attempting to
check for Node.js support. This allows Node.js ES6 module users to add
a polyfill like:
```js
import {webcrypto} from 'crypto'
globalThis.crypto = webcrypto
```
as described in #256 (comment)

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member

I've opened #284 which tires to address some of the issues discussed here and implements the detection logic in #256 (comment)

@josephg can take a look and let me know if that addresses your conerns?

@josephg
Copy link
Author

josephg commented Sep 13, 2022

Fantastic thanks! I'll take a look in the next couple days

@josephg
Copy link
Author

josephg commented Sep 14, 2022

Looks great - lovely work. I really enjoy the updates to the documentation.

Everything looks great when building using wasm-pack in web & nodejs modes. I'm very happy with the change!

For completeness with wasm-pack, I tried building in "bundler" mode. When I do that, it tries to execute this method:

export function __wbg_require_b3fbb8db36a15c55() { return logError(function () {
    const ret = module.require; // <--- this crashes
    return addHeapObject(ret);
}, arguments) };

And this fails with an error:

$ node --experimental-wasm-modules foo.mjs
(node:391210) ExperimentalWarning: Importing WebAssembly modules is an experimental feature. This feature could change at any time
wasm-bindgen: imported JS function that was not marked as `catch` threw an error: module is not defined

Stack:
ReferenceError: module is not defined
    at file:///home/seph/src/rand-wasm/pkg/rand_wasm_bg.js:201:17
...

Personally I'm not concerned, since I fail to see the point of wasm-pack 's "bundler" mode. It feels like a halfbaked kludge. To reach this point I:

  1. Build using bundler mode (wasm-pack build --target bundler)
  2. Change the resulting package.json to "type": "module" (this is a long standing bug in wasm-pack)
  3. Run nodejs with --experimental-wasm-modules, and pull in the resulting package.

I'd be happy to ship the change as-is. Thanks for giving this some attention!

@josephg
Copy link
Author

josephg commented Sep 14, 2022

As a follow-up, I just tested the library in wasm-pack's 'web' mode in deno, and it works great. Much cleaner than nodejs, because deno supports the webcrypto API directly, and supports file:/// URLs with fetch.

Rust:

use wasm_bindgen::prelude::*;

#[wasm_bindgen]
pub fn foo() -> u32 {
    let mut bytes = [0; 4];
    getrandom::getrandom(&mut bytes[..]).expect("getrandom failed");
    u32::from_be_bytes(bytes)
}

Built with wasm-pack build --target web

foo.js:

import {default as init, foo} from './pkg/rand_wasm.js'

;(async () => {
    let module = await init()    
    console.log(module.foo()) // Works great
    console.log(foo()) // Also works
})()
$ deno run --allow-read foo.mjs
module {
  memory: WebAssembly.Memory {},
  foo: [Function: 69],
  __wbindgen_exn_store: [Function: 132],
  __wbindgen_free: [Function: 126],
  __wbindgen_malloc: [Function: 95],
  __wbindgen_realloc: [Function: 108]
}
479281192
3637154827

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 a pull request may close this issue.

3 participants