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

Fix Webpack warning caused by dynamic require #234

Merged

Conversation

mjhanninen
Copy link
Contributor

This PR reverts the changes in commit 120a1d7 partially . In particular, it changes the wasm-bindgen binding of module.requirement back to the older form. As a result the generated Javascript binding code looks now like:

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

As a consequence Webpack does not detect this as a package import and, thus, does not attempt to bundle it which solves problem. It is safe skip the bundling of the crypto package because (1) it is imported only on Node and (2) it comes with Node.

Relevant issues:

Webpack supports dynamic importing only for some special cases in which
it is able to narrow down the set of packages to bundled.  In the
general case it just produces an empty (Webpack) context plus the
warning stating that "the request of a dependency is an expression."

Apparently the commit 120a1d7 changed the Javascript generated by
wasm-bindgen so that the binding for the `require` became:

```
module.require(getStringFromWasm0(arg0, arg1))
```

when it used to be:

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

In the latter case Webpack did not even realize that this code imported
a package and, hence, did not try to bundle it.  The new code triggers
the bundling and because the dependency is fully dynamic Webpack has
problems with it.

This commit reverts partially the commit 120a1d7 so that the generated
binding obfuscates the `require` call enough to hide it from Webpack
again.
Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member

Sorry for the delay. I was hoping there would eventually be a better solution to #224 but nobody has chimed in, so I'll just merge this (as it does seem to work).

@josephlr josephlr merged commit e6e7dd6 into rust-random:master Jan 14, 2022
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.

2 participants