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

Avoid require an expression (wasm-bindgen) #138

Closed
wants to merge 1 commit into from
Closed

Avoid require an expression (wasm-bindgen) #138

wants to merge 1 commit into from

Conversation

IcanDivideBy0
Copy link

@IcanDivideBy0 IcanDivideBy0 commented Apr 16, 2020

When using getrandom with Webpack, there is a warning (which is a bit annoying) about using an expression in a require call

Critical dependency: the request of a dependency is an expression

It turns out this can be avoided by using the module attribute on the wasm_bindgen macro. Also the resulting code is a bit simpler and more correct (randomFillSync is not a method of some crypto class, just a function attached to the module.exports object, unlike the browser version, where the getRandomValues function is declared in the prototype of the crypto object)

@IcanDivideBy0
Copy link
Author

IcanDivideBy0 commented Apr 16, 2020

Travis build failure seems unrelated:

$ cargo web test --target wasm32-unknown-emscripten --no-run
   Compiling libc v0.2.69
   Compiling getrandom v0.1.14 (/home/travis/build/rust-random/getrandom)
   Compiling cfg-if v0.1.10
    Finished test [unoptimized + debuginfo] target(s) in 1.35s
   Compiling getrandom v0.1.14 (/home/travis/build/rust-random/getrandom)
    Finished test [unoptimized + debuginfo] target(s) in 1m 01s
thread 'main' panicked at 'internal error: wasm doesn't exist where I expected it to be', src/cargo_shim/mod.rs:905:29

this PR isn't related to the stdweb build :/

@josephlr josephlr mentioned this pull request Apr 25, 2020
@josephlr
Copy link
Member

I prefer this approach (if it works) to the one in #137

Once I fix the CI I'll try it out. As mentioned in that issue, this fix should be submitted against the 0.2 branch.

@IcanDivideBy0
Copy link
Author

Freshly rebased on 0.2 branch :)

@josephlr josephlr changed the base branch from master to 0.2 April 25, 2020 07:47
@josephlr
Copy link
Member

So I tried this out locally w/ Firefox, and unfortunately this isn't going to work. When running on Firefox, we get TypeError: Error resolving module specifier: crypto. So it looks like the NodeJS module is being looked up unconditionally (even when not running under NodeJS).

For context, this code should work in the browser and in Node by doing runtime detection. wasm-bindgen support is already in its own crate, so we could split it into wasm-bindgen-web and wasm-bindgen-node, but that seems unnecessary given that it's possible to check the implementation at runtime.

@IcanDivideBy0
Copy link
Author

Ok feel free to close this then!

@josephlr josephlr closed this Apr 25, 2020
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

2 participants