Add tests of EXPORT_ES6 bundling with webpack, esbuild, and rollup #19086
Conversation
| @@ -5546,6 +5547,30 @@ def test_webpack(self): | |||
| shutil.copyfile('webpack/src/hello.wasm', 'webpack/dist/hello.wasm') | |||
| self.run_browser('webpack/dist/index.html', '/report_result?exit:0') | |||
|
|
|||
| def test_webpack_es6(self): | |||
There was a problem hiding this comment.
Should we rename the above test test_webpack_cjs maybe?
It looks like the main difference here is that you pass src/index.mjs to the webpack command. Does webpack default to src/index.js otherwise?
There was a problem hiding this comment.
Hi, yes, webpack will default to src/index.js otherwise. I am not entirely sure if it will also expect CommonJS module syntax in that case, I figure it is best to be safe and use .mjs since there is no package.json to tell Node that it is an ESM.
There was a problem hiding this comment.
yes, probably renaming it to test_webpack_cjs is a good idea!
| @@ -1,10 +1,14 @@ | |||
| { | |||
| "private": true, | |||
| "devDependencies": { | |||
| "@chialab/esbuild-plugin-meta-url": "^0.17.5", | |||
| "@web/rollup-plugin-import-meta-assets": "^1.0.7", | |||
There was a problem hiding this comment.
Can you explain in a little more detail why these plugins are needed?
What breaks if they are not included? Is this something we should be looking to fix in emscripten?
There was a problem hiding this comment.
Unlike webpack, esbuild and rollup require a plugin to handle referencing assets with new URL(..., import.meta.url). These ensure that the .wasm file gets copied to the bundle directory and that the Emscripten runtime code is rewritten to point to it. Of course, you can also do this manually as is done in the CommonJS webpack test, but it's probably good to take advantage of the asset bundling (and also know if it breaks for some reason).
As I mentioned in the issue, esbuild may add built-in support for import.meta.url assets, but the current implementation doesn't work properly for WebAssembly (whereas the plugin works, at least for simple cases like this).
There's nothing that needs to be fixed in emscripten for the moment, aside from adding a leading ./ to the path to the .wasm file (which isn't strictly necessary but will help ensure compatibility)
There was a problem hiding this comment.
When you say "you can also do this manually as is done in the CommonJS webpack test", does that mean these new tests are actually testing more than the existing test and the the existing test doesn't correctly bundle the wasm file? Should the existing test be changed in some way to bundle the wasm file?
Also, just to confirm these plugins relate the bundling of the wasm and therefore would not be needed in the case of -sSINGLE_FILE?
There was a problem hiding this comment.
The existing test is fine, it's just that webpack is unable to automatically bundle wasm files referenced from CommonJS modules unless you require them, which is not a good idea since it makes the resulting module webpack-specific. Usually I use copy-webpack-plugin to copy it into place as part of the webpack build.
When you use ES6 modules, you can use import.meta.url which allows the code to reference the accompanying wasm file in the same way from the browser, node, and webpack.
Yes, none of this is needed for -sSINGLE_FILE - which reminds me that I had some other issues with -sSINGLE_FILE that I could submit a PR for!
There was a problem hiding this comment.
(example of using copy-webpack-plugin: https://github.com/dhdaines/webpack-wasm-test/blob/main/webpack.config.cjs.js)
This follows on the suggestion in #18969 and adds tests/examples for webpack, esbuild, and rollup using
MODULARIZE=1,EXPORT_ES6=1andENVIRONMENT=web.Note that if you don't specify
ENVIRONMENT=webthen they will fail, and there is not always a workaround for this.The esbuild and rollup ones require plugins, and esbuild may soon have this functionality built-in (though it may or may not work for WebAssembly).
They all reuse the same HTML from the webpack test, so feel free to merge that if you prefer.