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

Emit bundler-friendly URL locators #13571

Closed
hardfist opened this issue Feb 27, 2021 · 18 comments · Fixed by #14135
Closed

Emit bundler-friendly URL locators #13571

hardfist opened this issue Feb 27, 2021 · 18 comments · Fixed by #14135

Comments

@hardfist
Copy link

according to https://twitter.com/RReverser/status/1365342246653018115 ,wasm-bindgen already support emit code import wasm using import, which is very helpful for bundler, I wonder whether emscripten could support the same feature?

@RReverser
Copy link
Collaborator

To elaborate: in EXPORT_ES6 mode, we should be able to use new URL('...', import.meta.url) pattern as the default Wasm location.

This pattern is increasingly commonly recognised by tooling, including major bundlers, and would allow to bundle Emscripten-generated JS more effortlessly.

It's, of course, also a valid runtime expression to get a URL of the Wasm file relatively to the module, so it works directly in browsers too.

@RReverser RReverser changed the title support load wasm using import in js glue code Emit bundler-friendly URL locators Mar 1, 2021
@RReverser
Copy link
Collaborator

I tried to take a stab at this, but ran into a blocker on the Closure side :( google/closure-compiler#3707

@RReverser
Copy link
Collaborator

No, wait, I think it's on Terser side...

@RReverser
Copy link
Collaborator

Ah it's both.

matthewp added a commit to FredKSchott/snowpack that referenced this issue Mar 2, 2021
This adds the import meta rollup extension which will copy assets that use this pattern:

```js
new URL('./compiler.wasm', import.meta.url);
```

Among other things, this helps support for wasm when the above pattern is used.

* Rust now uses this pattern, so bundlers can include the wasm file. https://twitter.com/RReverser/status/1365342246653018115
* Emscripten (compiles C/C++) is working on support: emscripten-core/emscripten#13571 (comment)

This will also help with other types of static assets loaded this way, such as images.
matthewp added a commit to FredKSchott/snowpack that referenced this issue Mar 2, 2021
This adds the import meta rollup extension which will copy assets that use this pattern:

```js
new URL('./compiler.wasm', import.meta.url);
```

Among other things, this helps support for wasm when the above pattern is used.

* Rust now uses this pattern, so bundlers can include the wasm file. https://twitter.com/RReverser/status/1365342246653018115
* Emscripten (compiles C/C++) is working on support: emscripten-core/emscripten#13571 (comment)

This will also help with other types of static assets loaded this way, such as images.
@kripken
Copy link
Member

kripken commented Mar 2, 2021

Perhaps this could be implemented in -O0/-O1 at least, which don't run terser or closure? Then we can expand later when those projects fix things.

@RReverser
Copy link
Collaborator

@kripken I actually worked around that issue, but then ran into scriptDirectory issues on EXPORT_ES6 + Node.js from #11792, then tried to fix those but got very confused around all the scriptDirectory handling and nested #ifdefs (there seems to be quite a bit of duplication) and that's where I'm at now 😅

Maybe if someone wants to tackle #11792, I'm happy to do just this one on top.

@curiousdannii
Copy link
Contributor

Yes, unfortunately despite being there for years, EXPORT_ES6 is still fundamentally broken. :(

@matthewp
Copy link

matthewp commented Mar 4, 2021

I worked around the minifier support bug myself by instead using IMPORT_META_URL and then replacing that with import.meta.url in a post-build transform. Not sure if a hacky solution like that is worthy of being in the compiler.

@RReverser
Copy link
Collaborator

@matthewp Yeah I did something similar in Emscripten patch too; the Node issue / scriptDirectory still needs to be fixed though.

@matthewp
Copy link

matthewp commented Mar 4, 2021

I haven't ran into the scriptDirectory issue, probably because I'm using a custom locateFile. the worst issue I ran into is that in Node it depends on synchronous requires that can't just be replaced with dynamic import. So I wound up prepending static imports for fs/path, which meant having separate modules for Node and browser.

@RReverser
Copy link
Collaborator

@matthewp Yeah that issue was also mentioned in #11792.

@matthewp
Copy link

matthewp commented Mar 4, 2021

@RReverser Ah yeah, I now see what you mean about the nested #ifs being confusing. I wonder if that can be replaced with JS ifs and then let closure worry about DCE-ing unused conditions. Probably for another issue. I'll follow up in that other issue.

@hardfist
Copy link
Author

hardfist commented Mar 5, 2021

Perhaps this could be implemented in -O0/-O1 at least, which don't run terser or closure? Then we can expand later when those projects fix things.

Yes, We use esbuild to bundle&minify wasm|js in production, which does not rely on terser or google closure compiler, I think wasm bundle strategy should be left to user land(so many strategies could be token),and do not think bound with terser or Closure is an good idea.

@RReverser
Copy link
Collaborator

RReverser commented Mar 5, 2021

@hardfist Not really; running Closure on practice is still useful regardless of the final minifier, because Closure can do a lot better minification using Emscripten-emitted annotations, whereas any regular minifier would just ignore those comments and would have less leeway in optimizing code, removing unused pieces, renaming properties and so on.

@hardfist
Copy link
Author

any one working on this ? It's really annoying to deploy an universal wasm library without this feature. and we're ok with the unminify version of glue js.

@kripken
Copy link
Member

kripken commented May 7, 2021

It looks like closure fixed this meanwhile. Perhaps terser has too? If so maybe we can update.

If terser is a blocker here, we could consider adding an option to not run it even when optimizing. The results would be less optimal (e.g. metadce would not run), but it would unblock people?

@RReverser
Copy link
Collaborator

RReverser commented May 8, 2021

Yes, there is more than Closure though - I've actually made quite a bit of progress on this (again) today and can make a PR next week that will cover most common scenarios.

(For now I've decided to treat Node.js + EXPORT_ES6 as simply unsupported combo, since it's already broken in other ways and it allows to unblock this issue otherwise.)

RReverser added a commit that referenced this issue May 10, 2021
This PR changes loading of main Wasm binary as well as helper Worker used by PThread integration to use a URL expression that can be both used directly by browsers as well as statically detected by bundlers like Webpack.

The main caveats are:
1. Emscripten is very configurable, so some of the new conditions might look odd but are necessary to keep backward compatibility and allow overriding bundler-friendly URL with a custom one during runtime.
2. None of Closure, our fork of Terser, and `eval` (which is used by Emscripten's JS library preprocessing) support `import.meta` expressions without more work. While Closure seems to have _just_ implemented such support, and it wouldn't be too hard to add it to our Terser too, `eval` usage would still require a string replacement before execution (or complete revamp).
  To keep implementation simple, for now I went with just string replacement that covers all tools - this way, we replace `import.meta` -> `EMSCRIPTEN$IMPORT$META` only once per library when JS is added before any of this tooling is executed, and then replace back after everything is done right before the final emit.
  We might want to revisit this in future, but for now this works well and covers all the tooling incompatibilities together.
3. Webpack assumes that all modules are strict mode, so I updated `worker.js` correspondingly to avoid usages of global `this` (which is `undefined` in strict mode and breaks in bundled code) and instead using `self`; I've also updated the Node.js adapter code to satisfy strict requirements too and to be a bit simpler.
4. This won't work in Node.js, since it's not compatible with `EXPORT_ES6` in general yet.
5. I've only updated places for loading main Wasm binary and PThread code. This should cover majority of use-cases, but other external files like side modules, proxy-to-pthread, proxy-to-worker, external memory loading etc. are not covered by this PR and need to be updated separately if someone wants them to work with bundlers out of the box too.

Fixes #13571.
@RReverser
Copy link
Collaborator

Sent a PR at #14135.

RReverser added a commit that referenced this issue May 10, 2021
This PR changes loading of main Wasm binary as well as helper Worker used by PThread integration to use a URL expression that can be both used directly by browsers as well as statically detected by bundlers like Webpack.

The main caveats are:
1. Emscripten is very configurable, so some of the new conditions might look odd but are necessary to keep backward compatibility and allow overriding bundler-friendly URL with a custom one during runtime.
2. None of Closure, our fork of Terser, and `eval` (which is used by Emscripten's JS library preprocessing) support `import.meta` expressions without more work. While Closure seems to have _just_ implemented such support, and it wouldn't be too hard to add it to our Terser too, `eval` usage would still require a string replacement before execution (or complete revamp).
  To keep implementation simple, for now I went with just string replacement that covers all tools - this way, we replace `import.meta` -> `EMSCRIPTEN$IMPORT$META` only once per library when JS is added before any of this tooling is executed, and then replace back after everything is done right before the final emit.
  We might want to revisit this in future, but for now this works well and covers all the tooling incompatibilities together.
3. Webpack assumes that all modules are strict mode, so I updated `worker.js` correspondingly to avoid usages of global `this` (which is `undefined` in strict mode and breaks in bundled code) and instead using `self`; I've also updated the Node.js adapter code to satisfy strict requirements too and to be a bit simpler.
4. This won't work in Node.js, since it's not compatible with `EXPORT_ES6` in general yet.
5. I've only updated places for loading main Wasm binary and PThread code. This should cover majority of use-cases, but other external files like side modules, proxy-to-pthread, proxy-to-worker, external memory loading etc. are not covered by this PR and need to be updated separately if someone wants them to work with bundlers out of the box too.

Fixes #13571.
RReverser added a commit that referenced this issue May 11, 2021
This PR changes loading of main Wasm binary as well as helper Worker used by PThread integration to use a URL expression that can be both used directly by browsers as well as statically detected by bundlers like Webpack.

The main caveats are:
1. Emscripten is very configurable, so some of the new conditions might look odd but are necessary to keep backward compatibility and allow overriding bundler-friendly URL with a custom one during runtime.
2. None of Closure, our fork of Terser, or even `eval` (which is used by Emscripten's JS library preprocessing) support `import.meta` expressions without more work. While Closure seems to have _just_ implemented such support, and it wouldn't be too hard to add it to our Terser too, `eval` usage would still require a string replacement before execution (or complete revamp).
  To keep implementation simple, for now I went with just string replacement that covers all tools - this way, we replace `import.meta` -> `EMSCRIPTEN$IMPORT$META` only once when JS is added before any of this tooling is executed, and then replace back after everything is done right before the final emit.
  We might want to revisit this in future, but for now this works well and covers all the tooling incompatibilities together.
3. This won't work in Node.js, since it's not compatible with `EXPORT_ES6` in general yet.
4. I've only updated places for loading main Wasm binary and PThread code. This should cover majority of use-cases, but other external files like side modules, proxy-to-pthread, proxy-to-worker, external memory loading etc. are not covered by this PR and need to be updated separately if someone wants them to work with bundlers out of the box too.

Fixes #13571.
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.

5 participants