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

Adding in wrapper file to fix circular dependency with Webpack 5 #2110

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Apr 29, 2020

The ESM integration spec has changed so that bindings are no longer live (they are "snapshotted" during the linking phase). Normally this isn't a big deal, but it changes the behavior for circular dependencies.

Webpack 5 implements the new ESM integration spec, whereas Webpack 4 implements the old ESM integration spec. That means that wasm-bindgen works fine in Webpack 4 but is broken in Webpack 5.

To explain more clearly why the circular dependencies are now a problem:

  1. The index.js file imports index_bg.wasm which in turn imports index.js. This is necessary because index_bg.wasm needs to use the glue code defined in index.js, and index.js needs to export the functions defined in index_bg.wasm.

  2. This situation is covered in detail in the ESM integration proposal.

    The short explanation is that when index_bg.wasm imports index.js, it immediately snapshots the bindings from index.js before the index.js file has been evaluated. That means the bindings are in a "temporal dead zone" which means they will error when they are used.

    The only way to fix this is to export function declarations (e.g. export function foo() { ... }), which means that export const foo = ...; and export class Foo { ... } will not work.

  3. The other option is to import the index_bg.wasm file first, in which case the situation is reversed: now index_bg.wasm can correctly access all of the index.js exports, but index.js cannot use any of the index_bg.wasm exports at the top level.

So this PR fixes the problem in the following way:

  1. The index.js file is renamed to index_bg.js (and index_bg.wasm imports index_bg.js).

  2. The index_bg.js file no longer calls wasm.__wbindgen_start() (because it cannot call any index_bg.wasm exports at the top level).

  3. A new index.js file is created which imports the index_bg.wasm file first, then imports the index_bg.js file, then calls wasm.__wbindgen_start():

    import * as wasm from "./index_bg.wasm";
    export * from "./index_bg.js";
    wasm.__wbindgen_start();

    The ordering is important: by importing index_bg.wasm first, it gives us the correct circular ordering.

So the end result is that we just needed a tiny index.js wrapper file in order to force the correct evaluation order, so this was actually a very small change.

@BenoitDel
Copy link

BenoitDel commented Apr 29, 2020

I am doing it by hand til now. So thanks for your help.

Did you see the node dependencies problem too ? Webpack 5 doesn't include automatically node module.
For example, for util module you need to get rid of it or provide a polyfill.

I think that typescript declaration file has to be changed too.

@sokra
Copy link

sokra commented Apr 29, 2020

export * from "./index_bg.js";

Maybe you can only reexport relevant exports. Currently all helper methods are accessible from outside. This new extra layer can be used to create a facade.

@Pauan
Copy link
Contributor Author

Pauan commented Apr 29, 2020

@sokra Sure, that's definitely possible, but I'd rather save that for a different PR, since that's a larger change.

@Pauan
Copy link
Contributor Author

Pauan commented Apr 29, 2020

@BenoitDel This PR doesn't do anything with util. But util isn't even needed for the browser, so you can just use the --browser flag, which will remove the util import.

@alexcrichton
Copy link
Contributor

Thanks for this @Pauan! I agree with @sokra that it'd be cool if we could export only the real interface, but I also agree with you that it's ok to defer that to a future PR!

@alexcrichton alexcrichton merged commit e16f7e4 into rustwasm:master Apr 29, 2020
@BenoitDel
Copy link

@Pauan about util, I can open an issue if you prefer. I checked and the util is required both with target bundler and browser. i am new to all of this so i can be wrong.

With the current implementation (I mean without this PR) only name import from wasm module work. And the typescript declaration doesn't fit. I can also open another issue about this.

Sorry if it was not the place to talk about wasm-bindgen and webpack compatibility in general. Just tell me what to do

@Pauan Pauan deleted the fixing-webpack5 branch April 29, 2020 16:10
@Pauan
Copy link
Contributor Author

Pauan commented Apr 29, 2020

@BenoitDel You would use --target bundler --browser. But I created a PR which fixes it.

I'm not sure what you mean by the declaration not fitting, it seems fine to me. Could you file an issue for that?

@BenoitDel
Copy link

Sorry i posted an issue for both before seeing your message
#2116
#2117

Maybe you want to switch on the issue for separation of concern

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.

4 participants