FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: --experimental-wasm-modules integration support by guybedford · Pull Request #27659 · nodejs/node · GitHub

/ node Public

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

Closed
guybedford wants to merge 1 commit into
nodejs:masterfrom
guybedford:wasm-modules
ConversationCommits (1)ChecksFiles changed
Closed

esm: --experimental-wasm-modules integration support#27659
guybedford wants to merge 1 commit into
nodejs:masterfrom
guybedford:wasm-modules

Conversation

guybedford commented May 12, 2019

Copy link
Copy Markdown
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 commented May 12, 2019

Copy link
Copy Markdown
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3491

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 12, 2019
guybedford mentioned this pull request May 12, 2019
Closed
littledan approved these changes May 12, 2019
View reviewed changes

littledan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread lib/internal/modules/esm/translators.js Outdated

littledan May 12, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

guybedford May 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

guybedford May 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

guybedford May 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

To summarize:

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

guybedford May 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

littledan May 12, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

devsnek May 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

devsnek suggested changes May 12, 2019
View reviewed changes

devsnek left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

devsnek reviewed May 12, 2019
View reviewed changes
Comment thread test/es-module/test-esm-wasm.mjs Outdated
ljharb reviewed May 12, 2019
View reviewed changes

ljharb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
  • 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 commented May 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

@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 commented May 12, 2019

Copy link
Copy Markdown
Member

@devsnek so node somethingWithStart.wasm will work?

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

devsnek commented May 12, 2019

Copy link
Copy Markdown
Member

@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 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 requested a review from devsnek May 12, 2019 20:57
targos approved these changes May 12, 2019
View reviewed changes
Comment thread doc/api/esm.md Outdated
Comment thread src/node_options.cc Outdated
devsnek reviewed May 12, 2019
View reviewed changes
Comment thread test/fixtures/es-modules/simple.wat Outdated
devsnek suggested changes May 12, 2019
View reviewed changes

devsnek left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 requested a review from devsnek May 12, 2019 22:25

guybedford commented May 12, 2019

Copy link
Copy Markdown
Contributor Author

Sure, I've added a start function test.

MylesBorins approved these changes May 13, 2019
View reviewed changes

MylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott reviewed May 13, 2019
View reviewed changes
Comment thread doc/api/esm.md Outdated

Trott May 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

xtuc approved these changes May 14, 2019
View reviewed changes

xtuc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice work @guybedford!

xtuc reviewed May 14, 2019
View reviewed changes
Comment thread doc/api/esm.md Outdated
RReverser reviewed May 14, 2019
View reviewed changes
Comment thread doc/api/esm.md Outdated

nodejs-github-bot commented May 16, 2019

Copy link
Copy Markdown
Collaborator

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

guybedford commented May 17, 2019

Copy link
Copy Markdown
Contributor Author

Landed in bbc254d.

guybedford closed this May 17, 2019
guybedford pushed a commit that referenced this pull request May 17, 2019
esm: --experimental-wasm-modules integration support
bbc254d 
PR-URL: #27659
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request May 17, 2019
esm: --experimental-wasm-modules integration support
47f913b 
PR-URL: #27659
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BridgeAR mentioned this pull request May 21, 2019
Merged
4 tasks
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
2019-05-21, Version 12.3.0 (Current)
20c11be 
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
2019-05-21, Version 12.3.0 (Current)
9fec70a 
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 commented May 22, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Ahem, very interesting! 😄

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

florisdh commented May 31, 2019

Copy link
Copy Markdown

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
doc: add missing --experimental-wasm-modules docs
d26473b 
PR-URL: #27948
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Shingo Inoue <leko.noor@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins mentioned this pull request Jan 6, 2021
Closed
hi-ogawa mentioned this pull request Feb 7, 2024
Merged
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ljharb ljharb left review comments
Trott Trott left review comments
benjamingr benjamingr approved these changes
+6 more reviewers
littledan littledan approved these changes
RReverser RReverser left review comments
xtuc xtuc approved these changes
MylesBorins MylesBorins approved these changes
targos targos approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL