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

loader: allow importing wasm modules #18972

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ module.exports = {
'node-core/no-unescaped-regexp-dot': 'error',
},
globals: {
WebAssembly: false,
COUNTER_HTTP_CLIENT_REQUEST: false,
COUNTER_HTTP_CLIENT_RESPONSE: false,
COUNTER_HTTP_SERVER_REQUEST: false,
Expand Down
24 changes: 18 additions & 6 deletions lib/internal/loader/CreateDynamicModule.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@ const { ModuleWrap } = internalBinding('module_wrap');
const debug = require('util').debuglog('esm');
const ArrayJoin = Function.call.bind(Array.prototype.join);
const ArrayMap = Function.call.bind(Array.prototype.map);
const { Buffer } = require('buffer');

const createDynamicModule = (exports, url = '', evaluate) => {
const createDynamicModule = (exports, url = '', evaluate, imports = []) => {
debug(
`creating ESM facade for ${url} with exports: ${ArrayJoin(exports, ', ')}`
);
Expand All @@ -32,28 +33,39 @@ const createDynamicModule = (exports, url = '', evaluate) => {
const reflectiveModule = new ModuleWrap(src, `cjs-facade:${url}`);
reflectiveModule.instantiate();
const { setExecutor, reflect } = reflectiveModule.evaluate()();

const importLookup = {};
for (const name of imports)
importLookup[name] = `$${Buffer.from(name).toString('hex')}`;

// public exposed ESM
const reexports = `
import {
executor,
${ArrayMap(names, (name) => `$${name}`)}
} from "";
${ArrayJoin(ArrayMap(imports, (i) =>
`import * as ${importLookup[i]} from '${i}';`), '\n')}
export {
${ArrayJoin(ArrayMap(names, (name) => `$${name} as ${name}`), ', ')}
}
if (typeof executor === "function") {
// add await to this later if top level await comes along
executor()
executor({ ${
ArrayJoin(ArrayMap(imports, (i) => `'${i}': ${importLookup[i]}, `), '\n')}
});
}`;
if (typeof evaluate === 'function') {
setExecutor(() => evaluate(reflect));
setExecutor((imports) => {
reflect.imports = imports;
evaluate(reflect);
});
}
const module = new ModuleWrap(reexports, `${url}`);
module.link(async () => reflectiveModule);
module.instantiate();
return {
module,
reflect
reflect,
reflectiveModule,
};
};

Expand Down
3 changes: 2 additions & 1 deletion lib/internal/loader/DefaultResolve.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ const extensionFormatMap = {
'.mjs': 'esm',
'.json': 'json',
'.node': 'addon',
'.js': 'cjs'
'.js': 'cjs',
'.wasm': 'wasm',
};

function resolve(specifier, parentURL) {
Expand Down
17 changes: 12 additions & 5 deletions lib/internal/loader/ModuleJob.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ class ModuleJob {
this.modulePromise = moduleProvider(url);
this.module = undefined;
this.reflect = undefined;
let reflectiveModule;

// Wait for the ModuleWrap instance being linked with all dependencies.
const link = async () => {
({ module: this.module,
reflect: this.reflect } = await this.modulePromise);
reflect: this.reflect,
reflectiveModule } = await this.modulePromise);
if (inspectBrk) {
const initWrapper = process.binding('inspector').callAndPauseOnStart;
initWrapper(this.module.instantiate, this.module);
Expand All @@ -34,6 +36,11 @@ class ModuleJob {

const dependencyJobs = [];
const promises = this.module.link(async (specifier) => {
if (specifier === '' && reflectiveModule !== undefined) {
const r = reflectiveModule;
reflectiveModule = undefined;
return r;
}
const jobPromise = this.loader.getModuleJob(specifier, url);
dependencyJobs.push(jobPromise);
return (await (await jobPromise).modulePromise).module;
Expand All @@ -53,10 +60,10 @@ class ModuleJob {
}

async instantiate() {
if (this.instantiated) {
return this.instantiated;
}
return this.instantiated = this._instantiate();
if (!this.instantiated)
this.instantiated = this._instantiate();
await this.instantiated;
return this.module;
}

// This method instantiates the module associated with this job and its
Expand Down
13 changes: 13 additions & 0 deletions lib/internal/loader/Translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,16 @@ translators.set('json', async (url) => {
}
});
});

translators.set('wasm', async (url) => {
debug(`Translating WASMModule ${url}`);
const bytes = await readFileAsync(new URL(url));
const module = await WebAssembly.compile(bytes);
const imports = WebAssembly.Module.imports(module).map((i) => i.module);
const exports = WebAssembly.Module.exports(module).map((e) => e.name);
return createDynamicModule(exports, url, (reflect) => {
const instance = new WebAssembly.Instance(module, reflect.imports);
for (const name of exports)
reflect.exports[name].set(instance.exports[name]);
}, imports);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the error thrown in case of a bad or non-existent wasm file good enough? Does it include, at least, the path to the problematic module? I would definitely also add a test to verify that an exception is thrown for a bad or non-existent module.

Copy link
Member Author

@devsnek devsnek Feb 24, 2018

Choose a reason for hiding this comment

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

non-existant -> DefaultResolve won't resolve the file and MODULE_NOT_FOUND

bad -> WebAssembly.compile rejects and goes up the chain

i'll add another tests for errors

5 changes: 5 additions & 0 deletions test/common/index.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ export function leakedGlobals() {
// Turn this off if the test should not check for global leaks.
export let globalCheck = true; // eslint-disable-line

export function crashOnUnhandledRejection() {
process.on('unhandledRejection',
(err) => process.nextTick(() => { throw err; }));
}

process.on('exit', function() {
if (!globalCheck) return;
const leaked = leakedGlobals();
Expand Down
32 changes: 32 additions & 0 deletions test/es-module/test-esm-import-wasm-errors.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Flags: --experimental-modules

import { crashOnUnhandledRejection } from '../common';
import assert from 'assert';

crashOnUnhandledRejection();

async function expectsRejection(fn, settings) {
// Retain async context.
const storedError = new Error('Thrown from:');
try {
await fn();
} catch (err) {
try {
assert(err instanceof settings.type,
`${err.name} is not instance of ${settings.type.name}`);
assert.strictEqual(err.name, settings.type.name);
} catch (validationError) {
console.error(validationError);
console.error('Original error:');
console.error(err);
throw storedError;
}
return;
}
assert.fail('Missing expected exception');
}

expectsRejection(() =>
import('../fixtures/es-modules/invalid.wasm'), {
type: WebAssembly.CompileError,
});
15 changes: 15 additions & 0 deletions test/es-module/test-esm-import-wasm.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Flags: --experimental-modules --harmony-dynamic-import

import { crashOnUnhandledRejection } from '../common';
import assert from 'assert';

crashOnUnhandledRejection();

import { add } from '../fixtures/es-modules/add.wasm';

assert.strictEqual(add(2, 3), 5);

import('../fixtures/es-modules/add.wasm').then((ns) => {
Copy link
Contributor

@giltayar giltayar Feb 24, 2018

Choose a reason for hiding this comment

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

I'm guessing that this dynamic import will get the module from the cache and thus not instantiate it. This is good, as it checks wasm and cache together, but unfortunately it doesn't check dynamic importing a wasm module.

Perhaps add another import('../fixtures/es-modules/add.wasm?cache-avoider').then(...)?

Copy link
Member Author

Choose a reason for hiding this comment

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

i'm not checking dynamic instantiation (we know that works), i'm checking the namespace and default exports

assert.strictEqual(ns.add(2, 3), 5);
assert.strictEqual(ns.default(2), 12);
});
1 change: 1 addition & 0 deletions test/fixtures/es-modules/add.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const add = (a, b) => a + b;
Binary file added test/fixtures/es-modules/add.wasm
Binary file not shown.
18 changes: 18 additions & 0 deletions test/fixtures/es-modules/add.wat
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
;; source of add.wasm

(module
(import "./add.mjs" "add" (func $add_i32 (param i32 i32) (result i32)))

(func $add (param $p0 i32) (param $p1 i32) (result i32)
get_local $p0
get_local $p1
call $add_i32)

(func $add_10 (param $p0 i32) (result i32)
get_local $p0
i32.const 10
i32.add)

(export "add" (func $add))
(export "default" (func $add_10)))

1 change: 1 addition & 0 deletions test/fixtures/es-modules/invalid.wasm
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
a