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

esm: --experimental-wasm-modules integration support #27659

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

This PR provides support for node --experimental-modules --experimental-wasm-modules to support the ES module integration of WebAssembly modules through eg import './module.wasm'.

The semantics match the proposed ES module integration semantics for Web Assembly as specified in https://github.com/webassembly/esm-integration.

At the previous modules group meeting we got consensus approval for upstreaming this feature under the --experimental-wasm-modules flag.

//cc @nodejs/modules

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 12, 2019
Copy link

@littledan littledan left a comment

Choose a reason for hiding this comment

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

Note that this is slightly different than the standards track semantics, as it doesn't take a turn on the event loop during Instantiation. However, I don't expect this to affect most programs. I would also encourage you to write several more tests before landing.

const exports = WebAssembly.Module.exports(compiled).map(({ name }) => name);

return createDynamicModule(imports, exports, url, (reflect) => {
const { exports } = new WebAssembly.Instance(compiled, reflect.imports);

Choose a reason for hiding this comment

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

In a cycle-closing edge, will reflect.imports usually be an empty object? If so, this will be basically the right semantics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the unexecuted cycle edge, reflect.imports would contain all the imported module namespaces, with their named exports, but all of those named exports would be undefined. Function exports would be supported in cycles here though just like ES modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually cycles would throw for any accesses to uninitialized let bindings on the namespaces, since these would be in TDZ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To summarize:

  • Unexecuted cycle edge with function exports: fine - function is defined
  • Unexecuted cycle edge with let binding: throws - TDZ error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And specifically these errors are only thrown if the WASM module actually attempts to access the given binding.

Choose a reason for hiding this comment

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

Right, so in particular, Wasm exports should always act like const bindings, and all imports are accessed during Instantiate (in the ESM evaluate phase). Does this implementation do that?

Copy link
Member

Choose a reason for hiding this comment

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

yep! reflect.imports is namespace objects so the imports are accessed directly.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

please upload a wat source for simple.wasm with a comment in the top with the command used to produce simple.wasm

example: https://github.com/nodejs/node/blob/master/test/fixtures/shared-memory.wat

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

  • How might one have an entry point that's directly in wasm, without the need for an ESM wrapper?
  • how might one consume wasm modules in CJS?

@devsnek
Copy link
Member

devsnek commented May 12, 2019

@ljharb

How might one have an entry point that's directly in wasm, without the need for an ESM wrapper?

declare a wasm start function in your wasm module

this reminds me, @guybedford can you make a test which verifies that start functions run at the correct time?

how might one consume wasm modules in CJS?

at the moment, import()

@ljharb
Copy link
Member

ljharb commented May 12, 2019

@devsnek so node somethingWithStart.wasm will work?

I'll rephrase the question - how might one synchronously consume wasm modules in cjs?

@devsnek
Copy link
Member

devsnek commented May 12, 2019

@ljharb

so node somethingWithStart.wasm will work?

yes

how might one synchronously consume wasm modules in cjs

not possible atm. if require(esm) happens that would work i guess.

@devsnek devsnek added esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. wasm Issues and PRs related to WebAssembly. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels May 12, 2019
@guybedford guybedford requested a review from devsnek May 12, 2019 20:57
doc/api/esm.md Outdated Show resolved Hide resolved
src/node_options.cc Show resolved Hide resolved
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

actually... can you add a test that verifies that the wasm start function runs during the evaluate phase? in wat, you can specify it with (start $funcname)

@guybedford guybedford requested a review from devsnek May 12, 2019 22:25
@guybedford
Copy link
Contributor Author

Sure, I've added a start function test.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

CommonJS loader. Additional formats such as _"wasm"_ or _"addon"_ can be
extended in future updates.
CommonJS loader. Additional formats such as _"addon"_ can be extended in future
updates.
Copy link
Member

Choose a reason for hiding this comment

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

There's one additional mention of the non-existent --entry-type CLI flag 7 lines below this one. Is this the right PR to remove or update that material?

Copy link

@xtuc xtuc left a comment

Choose a reason for hiding this comment

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

Nice work @guybedford!

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@guybedford
Copy link
Contributor Author

Landed in bbc254d.

@guybedford guybedford closed this May 17, 2019
guybedford pushed a commit that referenced this pull request May 17, 2019
PR-URL: #27659
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
targos pushed a commit that referenced this pull request May 17, 2019
PR-URL: #27659
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@BridgeAR BridgeAR mentioned this pull request May 21, 2019
4 tasks
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label May 21, 2019
BridgeAR added a commit that referenced this pull request May 21, 2019
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799
BridgeAR added a commit that referenced this pull request May 21, 2019
Notable changes:

* esm:
  * Added the `--experimental-wasm-modules` flag to support
    WebAssembly modules (Myles Borins & Guy Bedford)
    #27659
* process:
  * Log errors using `util.inspect` in case of fatal exceptions
    (Ruben Bridgewater) #27243
* repl:
  * Add `process.on('uncaughtException')` support (Ruben Bridgewater)
    #27151
* stream:
  * Implemented `Readable.from` async iterator utility (Guy Bedford)
    #27660
* tls:
  * Expose built-in root certificates (Ben Noordhuis)
    #26415
  * Support `net.Server` options (Luigi Pinca)
    #27665
  * Expose `keylog` event on TLSSocket (Alba Mendez)
    #27654
* worker:
  * Added the ability to unshift messages from the `MessagePort`
    (Anna Henningsen) #27294

PR-URL: #27799
@vitaly-t
Copy link
Contributor

vitaly-t commented May 22, 2019

Ahem, very interesting! 😄

Any guidelines or examples on how to start playing with it in Node.js?

@florisdh
Copy link

I've created an example project using '--experimental-wasm-modules' for those who're interested.

https://github.com/florisdh/node-wasm

cjihrig referenced this pull request May 31, 2019
PR-URL: #27948
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. semver-minor PRs that contain new features and should be released in the next minor version. wasm Issues and PRs related to WebAssembly.
Projects
None yet
Development

Successfully merging this pull request may close these issues.