-
Notifications
You must be signed in to change notification settings - Fork 182
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
Fixing Webpack require warning when using wasm-bindgen #137
Conversation
Thank's for the quick fix! Could someone take a look at it, maybe @newpavlov? |
Looks good to me, but I am not familiar with webpack and Node, so I would prefer for someone else to take a look at it. CI failures are not caused by this PR, but I think ideally we should first fix CI, then rebase this PR and merge it after successful CI passes. Also could you please submit these changes to v0.2 branch as a separate PR? In it wasm code is moved to separate crates. |
Oops, I didn't realized there's already a PR for that. I just submitted another one with a better solution IMO #138 |
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.
This looks good to me (modulo nits). I really wish we could have got the wasm-bindgen
module
attribute to work, but as I noted in #138, that attribute makes things break on the Web (why? I have no idea).
#141 fixes the CI, so we should be able to test this PR if you rebase on top of it (or you can just wait for that to be merged).
#[derive(Clone, Debug)] | ||
type NodeCrypto; | ||
|
||
#[wasm_bindgen(method, js_name = randomFillSync, structural)] | ||
fn random_fill_sync(me: &NodeCrypto, buf: &mut [u8]); | ||
} | ||
|
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.
Formatting nit: could we just put all of the Node.js declarations in their own block. Something like:
#[wasm_bindgen]
extern "C" {
type NodeModule;
#[wasm_bindgen(js_name = module)]
static MODULE: NodeModule;
#[derive(Clone, Debug)]
type NodeCrypto;
#[wasm_bindgen(method)]
fn require(this: &NodeModule, s: &str) -> NodeCrypto;
#[wasm_bindgen(method, js_name = randomFillSync)]
fn random_fill_sync(this: &NodeCrypto, buf: &mut [u8]);
}
@josephlr In order to conditionally import a module, you have to use dynamic So the only option is to use the NodeJS-specific |
924a488
to
645e5d3
Compare
I just rebased this onto master (now that #142 has been merged) |
Still have this issue when I use [dependencies]
getrandom = { version = "0.2.3", features = ["js"] }
wasm-bindgen = {version = "0.2.76", features = ["serde-serialize"]} Webpack version: webpack 5.51.1
|
@huacnlee do you have any more specific reproduction instructions? Do you know why the above fix is only just now not working? If so, can you open a new issue about this? |
@josephlr I just add a example, please checkout it. I found that when I use Webpack 4 with JavaScript it is work will, but when I upgrade Webpack 5 + TypeScript it's can not work. |
@josephlr I'm experiencing this issue as well using getrandom with Webpack 5 and came across this while trying to resolve it. It looks like Webpack 5 now warns about export function __wbg_modulerequire_3440a4bcf44437db() { return handleError(function (arg0, arg1) {
var ret = module.require(getStringFromWasm0(arg0, arg1)); // <-- Critical dependency: the request of a dependency is an expression
return addHeapObject(ret);
}, arguments) }; |
When using the getrandom library with wasm-bindgen + Webpack it uses
require("crypto")
in order to load the NodeJS crypto module. However, Webpack does not like that, so it gives a warning.This PR fixes that warning by instead using
module.require("crypto")
.You can see more details here:
rustwasm/wasm-pack#822
webpack/webpack#8826