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.

Sign up for GitHub

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

Add tests of EXPORT_ES6 bundling with webpack, esbuild, and rollup #19086

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add tests of EXPORT_ES6 bundling with webpack, esbuild, and rollup #19086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a8d359
test: add EXPORT_ES6 version of webpack test
dhdaines Mar 28, 2023
66cd457
test: add esbuild (with import meta plugin) test
dhdaines Mar 28, 2023
873da44
test: add test with rollup
dhdaines Mar 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
{
"private": true,
"devDependencies": {
"@chialab/esbuild-plugin-meta-url": "^0.17.5",
"@web/rollup-plugin-import-meta-assets": "^1.0.7",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@dhdaines dhdaines Mar 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(example of using copy-webpack-plugin: https://github.com/dhdaines/webpack-wasm-test/blob/main/webpack.config.cjs.js)

"es-check": "^6.2.1",
"esbuild": "^0.17.14",
"eslint": "^8.16.0",
"eslint-config-prettier": "^8.5.0",
"prettier": "^2.7.1",
"rollup": "^3.20.2",
"source-map": "0.7.4",
"webpack": "^5.76.1",
"webpack-cli": "^5.0.1",
Expand Down
11 changes: 11 additions & 0 deletions test/esbuild/build.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import metaUrlPlugin from '@chialab/esbuild-plugin-meta-url';
import * as esbuild from 'esbuild';

await esbuild.build({
entryPoints: ['src/index.mjs'],
bundle: true,
outdir: 'dist',
format: 'esm',
logLevel: 'info',
plugins: [metaUrlPlugin()],
});
6 changes: 6 additions & 0 deletions test/esbuild/dist/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<html>
<body>
<hr><div id='output'></div><hr>
<script type="module" src="index.js"></script>
</body>
</html>
18 changes: 18 additions & 0 deletions test/esbuild/src/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import createModule from "./hello.mjs";

var params = {
print: (function() {
var element = document.getElementById('output');
return function(text) {
console.log(text);
element.innerHTML += text.replace('\n', '<br>', 'g') + '<br>';
};
})(),
canvas: document.getElementById('canvas'),
};

params.print("testing..");

createModule(params).then((instance) => {
console.log('loaded');
});
6 changes: 6 additions & 0 deletions test/rollup/dist/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<html>
<body>
<hr><div id='output'></div><hr>
<script type="module" src="index.js"></script>
</body>
</html>
10 changes: 10 additions & 0 deletions test/rollup/rollup.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { importMetaAssets } from '@web/rollup-plugin-import-meta-assets';

export default {
input: 'src/index.mjs',
output: {
dir: 'dist',
format: 'es',
},
plugins: [importMetaAssets()],
};
18 changes: 18 additions & 0 deletions test/rollup/src/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import createModule from "./hello.mjs";

var params = {
print: (function() {
var element = document.getElementById('output');
return function(text) {
console.log(text);
element.innerHTML += text.replace('\n', '<br>', 'g') + '<br>';
};
})(),
canvas: document.getElementById('canvas'),
};

params.print("testing..");

createModule(params).then((instance) => {
console.log('loaded');
});
25 changes: 25 additions & 0 deletions test/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
from tools import shared
from tools import ports
from tools import utils
from tools import config
from tools.shared import EMCC, WINDOWS, FILE_PACKAGER, PIPE
from tools.utils import delete_file, delete_dir

Expand Down Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, probably renaming it to test_webpack_cjs is a good idea!

shutil.copytree(test_file('webpack'), 'webpack')
with utils.chdir('webpack'):
self.compile_btest([test_file('hello_world.c'), '-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web',
'-sEXPORT_ES6=1', '-o', 'src/hello.mjs'])
self.run_process(shared.get_npm_cmd('webpack') + ['--mode=development', '--no-devtool', './src/index.mjs'])
self.run_browser('webpack/dist/index.html', '/report_result?exit:0')

def test_esbuild(self):
shutil.copytree(test_file('esbuild'), 'esbuild')
with utils.chdir('esbuild'):
self.compile_btest([test_file('hello_world.c'), '-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web',
'-sEXPORT_ES6=1', '-o', 'src/hello.mjs'])
self.run_process(config.NODE_JS + ['build.mjs'])
self.run_browser('esbuild/dist/index.html', '/report_result?exit:0')

def test_rollup(self):
shutil.copytree(test_file('rollup'), 'rollup')
with utils.chdir('rollup'):
self.compile_btest([test_file('hello_world.c'), '-sEXIT_RUNTIME', '-sMODULARIZE', '-sENVIRONMENT=web',
'-sEXPORT_ES6=1', '-o', 'src/hello.mjs'])
self.run_process(shared.get_npm_cmd('rollup') + ['-c', 'rollup.config.mjs'])
self.run_browser('rollup/dist/index.html', '/report_result?exit:0')


class emrun(RunnerCore):
def test_emrun_info(self):
Expand Down
18 changes: 18 additions & 0 deletions test/webpack/src/index.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import createModule from "./hello.mjs";

var params = {
print: (function() {
var element = document.getElementById('output');
return function(text) {
console.log(text);
element.innerHTML += text.replace('\n', '<br>', 'g') + '<br>';
};
})(),
canvas: document.getElementById('canvas'),
};

params.print("testing..");

createModule(params).then((instance) => {
console.log('loaded');
});