loader: allow importing wasm modules #18972

devsnek · 2018-02-24T07:01:10Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

loader

/cc @nodejs/modules

devsnek · 2018-02-24T07:08:01Z

CI https://ci.nodejs.org/job/node-test-pull-request/13355/

jdalton · 2018-02-24T07:18:31Z

lib/internal/loader/CreateDynamicModule.js

+  const importLookup = {};
+  for (const name of imports)
+    importLookup[name] = Buffer.from(name).toString('base64');
+


What's the name decoding 👆 about?

the names can be paths/urls which don't make very nice identifiers in js

Would you post an example, please?

https://github.com/nodejs/node/pull/18972/files#diff-b0fe7f1fb1b686a8b721a7e5b0518b13R4

Ahhhh so then it becomes

import * as someBase64Thing from "./add.mjs"

Then you capture that someBase64Thing namespace object, and then assign it to another object { "./add.mjs": someBase64Thing }, and then pass it along to get resolved, and then imported into it at
new WebAssembly.Instance(module, reflect.imports).

giltayar · 2018-02-24T08:11:56Z

test/es-module/test-esm-import-wasm.mjs

+
+assert.strictEqual(add(2, 3), 5);
+
+import('../fixtures/es-modules/add.wasm').then((ns) => {


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(...)?

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

giltayar · 2018-02-24T08:15:04Z

lib/internal/loader/Translators.js

@@ -90,3 +90,15 @@ translators.set('json', async (url) => {
    }
  });
 });
+
+translators.set('wasm', async (url) => {
+  const bytes = await readFileAsync(new URL(url));


The other translators have a "debug" line that outputs the action (i.e. "loading wasm ${url}"). I would suggest adding it here.

giltayar · 2018-02-24T08:17:25Z

lib/internal/loader/Translators.js

+    for (const name of exports)
+      reflect.exports[name].set(instance.exports[name]);
+  }, imports);
+});


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.

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

giltayar · 2018-02-24T09:44:36Z

lib/internal/loader/CreateDynamicModule.js

+
+  const importLookup = {};
+  for (const name of imports)
+    importLookup[name] = Buffer.from(name).toString('base64');


Doesn't base64 also include the characters + and / which are illegal JS identifiers? Also, a base64 string can start with a digit, which is not allowed in JS identifiers.

You can solve this by replacing + with _, / with $, and adding a letter (e.g. i) at the beginning of the identifier.

ou should also remove the trailing = signs that are used for padding base64. Or replace them also with $.

i already add $ at the beginning below, but you are right about the other stuff, a huge oversight on my part 😄

@devsnek .toString('hex')

giltayar · 2018-02-24T09:56:12Z

Pardon my ignorance, and this isn't really part of this PR, but why does CreateDynamicModule.js have to do the dance of two modules? Can't it just create one module which imports what is needed, exports some variables that it let-s (without values), and then calls the executor, allowing it to change the exported variables?

devsnek · 2018-02-24T13:11:01Z

@giltayar we need to delay that final callback until actual execution time, while still already having the reflect pre-evaluated, its a nasty situation (i've tried a few times to consolidate it)

bmeck · 2018-02-24T13:28:19Z

We should hold off on this until the WASM working group is ready and designed their exact behavior with respect to things like importing JS from WASM and live bindings.

CC: @linclark

bmeck

Needs WASM group feedback

bmeck · 2018-02-24T13:34:05Z

Ah, to note: in https://tc39.github.io/ecma262/#sec-source-text-module-records

A Source Text Module Record can exist in a module graph with other subclasses of the abstract Module Record type. However, non-source text Module Records must not participate in dependency cycles with Source Text Module Records.

Is my main concern with respect to loading JS.

and lack of liveness from our ESM facades is the other concern. Since WASM would suddenly not be able to have live exports with this.

weswigham · 2018-02-24T15:49:25Z

@bmeck do we know the reason the spec forbids circular dependencies involving both JS and non-JS modules? Is it just because non-JS execution can't be defined, so guaranteeing the "well-behaved-ness" of the execution of the circularly dependent set is hard when it includes non-JS code (even though for wasm at least it seems pretty straightforward since it has similar binding properties to esm itself)? Or was it forbidden so it could be dealt with at a later date? Or was it forbidden because cycles in general are a bad design practice? The lack of commentary in the spec makes it difficult to know the rationale for the limitation.

devsnek · 2018-02-24T16:05:42Z

@weswigham from irc:

<bradleymeck> I'll make a March agenda item to remove that paragraph from JS spec
<devsnek> `non-source text Module Records must not participate in dependency cycles with Source Text Module Records.`
<devsnek> this is the line you were referring to right
<bradleymeck> yes
<bradleymeck> that line was added after TC39 looked at CJS, but doesn't really make sense
<bradleymeck> it only relates to other module systems that are 1. synchronous 2. able to cause evaluation cycles with ESM 3. can dynamically grow their list of exported names
<bradleymeck> which are few... very few
<bradleymeck> and CJS interop we have doesn't have #2 and #3

linclark · 2018-02-26T21:53:30Z

We should hold off on this until the WASM working group is ready and designed their exact behavior with respect to things like importing JS from WASM and live bindings.

Just to update on timing, I added WASM ES modules to the agenda for the March 6 WASM community group call. I also plan to present at the WASM working group face-to-face in April.

do we know the reason the spec forbids circular dependencies involving both JS and non-JS modules?

The issue where it was introduced is here: tc39/ecma262#916 (in this commit tc39/ecma262@0f36dba). I'll be discussing the WASM ES modules work with Domenic, who introduced it, so can ask him what needs to happen for it to be removed.

weswigham · 2018-02-26T22:26:12Z

It looks like it's just a matter of the [[Status]] internal member being on the SourceTextModuleRecord and not AbstractModuleRecord (and thereby not having the correct possible values for the resolution algorithm), and the prose seems to reinforce that. An abstract class nestled between the two (perhaps CycleCapableModuleRecord) which adds the appropriate [[Status]] member (which is the referenced instead of SourceTextModuleRecord in the prose in question) would solve the problem, at least from the spec perspective.

weswigham · 2018-03-13T10:24:09Z

Ignoring the obvious desire of for a wasm import to work like a similar js one (defs a good one), I feel it's worth mentioning that philosophically, the spec cares as little about wasm modules as it does json documents right now (or any other non-esm module, or how a non-esm graph could be reentrant with a esm one), yeah? If node is... Deferring to? Depending on? other projects to decide how to load them, it'd certainly be nice if all these not-really-js imports we're handled similarly or predictably (or at least that their loading behaviors could be specified predictably), rather than all of them being implemented and/or behaving slightly differently. Now, I'm not saying this must be a loader (it's a good thing to have on by default)... But this should probably be buildable as a userspace loader, too, right? ¯_(ツ)_/¯ it'd certainly be nice to have it as a loader that was just on by default, which'd allow it to be overridden/recomposed/reused/depended on like userspace loaders.

Oh, also, shouldn't, like, there be a simplified API like this to load wasm in cjs? Or is it expected that once this goes in dynamic import will essentially be that in cjs?

BridgeAR · 2018-04-28T13:51:37Z

What is the status here? Should this stay open?

devsnek · 2018-04-28T14:15:17Z

@BridgeAR blocked because we're waiting to see what happens with wasm integrating with es modules and its a new feature and modules team doesn't want to land new features

TimothyGu · 2018-04-28T20:09:48Z

@devsnek Do you think it would better for us to close this until the WebAssembly WG figure out the spec side of things?

devsnek · 2018-04-28T21:12:20Z

@TimothyGu probably an okay idea

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 24, 2018

devsnek added experimental Issues and PRs related to experimental features. esm Issues and PRs related to the ECMAScript Modules implementation. labels Feb 24, 2018

jdalton reviewed Feb 24, 2018

View reviewed changes

devsnek force-pushed the feature/esm-wasm-import branch 3 times, most recently from d01894c to d774051 Compare February 24, 2018 07:46

giltayar reviewed Feb 24, 2018

View reviewed changes

bmeck suggested changes Feb 24, 2018

View reviewed changes

devsnek force-pushed the feature/esm-wasm-import branch 2 times, most recently from 627be2a to c6a5756 Compare February 24, 2018 13:55

loader: allow importing wasm modules

55dab0b

devsnek force-pushed the feature/esm-wasm-import branch from c6a5756 to 55dab0b Compare February 24, 2018 17:45

benjamingr mentioned this pull request Feb 27, 2018

Node.js Foundation Modules Team Meeting 2018-02-28 nodejs/modules#36

Closed

MylesBorins added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Feb 27, 2018

BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Mar 6, 2018

MylesBorins mentioned this pull request Mar 12, 2018

Node.js Foundation Modules Team Meeting 2018-03-14 nodejs/modules#47

Closed

MylesBorins mentioned this pull request Mar 27, 2018

Node.js Foundation Modules Team Meeting 2018-03-28 nodejs/modules#52

Closed

MylesBorins removed the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Apr 10, 2018

devsnek closed this Apr 28, 2018

devsnek deleted the feature/esm-wasm-import branch June 25, 2018 07:20

devsnek restored the feature/esm-wasm-import branch June 25, 2018 07:20

devsnek deleted the feature/esm-wasm-import branch June 25, 2018 07:20

devsnek mentioned this pull request Aug 24, 2018

use builtin WebAssembly interface instead of AST parser wasm-tool/node-loader#4

Merged

devsnek mentioned this pull request Feb 28, 2019

esm: experimental wasm modules nodejs/ecmascript-modules#46

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader: allow importing wasm modules #18972

loader: allow importing wasm modules #18972

devsnek commented Feb 24, 2018 •

edited

Loading

devsnek commented Feb 24, 2018

jdalton Feb 24, 2018 •

edited

Loading

devsnek Feb 24, 2018

jdalton Feb 24, 2018 •

edited

Loading

devsnek Feb 24, 2018

jdalton Feb 24, 2018 •

edited

Loading

devsnek Feb 24, 2018

giltayar Feb 24, 2018 •

edited

Loading

devsnek Feb 24, 2018

giltayar Feb 24, 2018

giltayar Feb 24, 2018

devsnek Feb 24, 2018 •

edited

Loading

giltayar Feb 24, 2018

giltayar Feb 24, 2018

devsnek Feb 24, 2018

bmeck Feb 24, 2018

giltayar commented Feb 24, 2018

devsnek commented Feb 24, 2018

bmeck commented Feb 24, 2018

bmeck left a comment

bmeck commented Feb 24, 2018

weswigham commented Feb 24, 2018

devsnek commented Feb 24, 2018 •

edited

Loading

linclark commented Feb 26, 2018

weswigham commented Feb 26, 2018 •

edited

Loading

weswigham commented Mar 13, 2018

BridgeAR commented Apr 28, 2018

devsnek commented Apr 28, 2018

TimothyGu commented Apr 28, 2018

devsnek commented Apr 28, 2018


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

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

loader: allow importing wasm modules #18972

loader: allow importing wasm modules #18972

Conversation

devsnek commented Feb 24, 2018 • edited Loading

Checklist

Affected core subsystem(s)

devsnek commented Feb 24, 2018

jdalton Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giltayar Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giltayar commented Feb 24, 2018

devsnek commented Feb 24, 2018

bmeck commented Feb 24, 2018

bmeck left a comment

Choose a reason for hiding this comment

bmeck commented Feb 24, 2018

weswigham commented Feb 24, 2018

devsnek commented Feb 24, 2018 • edited Loading

linclark commented Feb 26, 2018

weswigham commented Feb 26, 2018 • edited Loading

weswigham commented Mar 13, 2018

BridgeAR commented Apr 28, 2018

devsnek commented Apr 28, 2018

TimothyGu commented Apr 28, 2018

devsnek commented Apr 28, 2018

devsnek commented Feb 24, 2018 •

edited

Loading

jdalton Feb 24, 2018 •

edited

Loading

jdalton Feb 24, 2018 •

edited

Loading

jdalton Feb 24, 2018 •

edited

Loading

giltayar Feb 24, 2018 •

edited

Loading

devsnek Feb 24, 2018 •

edited

Loading

devsnek commented Feb 24, 2018 •

edited

Loading

weswigham commented Feb 26, 2018 •

edited

Loading