Skip to content
This repository was archived by the owner on Apr 16, 2020. It is now read-only.

esm: experimental wasm modules - #46

Closed
MylesBorins wants to merge 8 commits into
modules-lkgrfrom
wasm-modules
Closed

esm: experimental wasm modules#46
MylesBorins wants to merge 8 commits into
modules-lkgrfrom
wasm-modules

Conversation

@MylesBorins

Copy link
Copy Markdown
Contributor

This is a first pass at a loader for wasm modules.

It offers support for both named imports as well as deafult.
It may be possible to move some of this logic down to C++, but
I think the current implementation might be good enough. It doesn't
currently use the ESM cache, but AFAICT it isn't currently exposed.

Will dig in more with tests / examples / cache after initial
implementation gets feedback.

/cc @linclark@lukewagner@tschneidereit@sunfishcode

@devsnek

Copy link
Copy Markdown
Member

related: nodejs/node#18972

i think we should wait for wasm modules to be specified, at which point v8 would provide them for us.

Comment threadlib/internal/modules/esm/translators.js Outdated
Comment threadlib/internal/modules/esm/translators.js Outdated
@MylesBorins

Copy link
Copy Markdown
ContributorAuthor

@devsnek I had a meeting with some of the folks working on the WASM team today. I don't think it is clear how much of this will be in ecma262 / V8 vs. blink

Since this is related to the loader there way not actually be as much as expected inside of V8 proper. I also think it might be worth exploring this as an experimental interface to get an implementation of the expected behavior into the hands of developers, this in turn could affect the standard being developed.

i'll let the folks I cc'd chime in about timing and implementation thoughts

@guybedford

Copy link
Copy Markdown
Contributor

@MylesBorins I think named exports are important to include here, as that is how the WASM spec integration with ES modules will work (//cc @littledan).

If you are looking for the code to do identifier validation, I included this in 9377b27#diff-e866c1b8fc8510c620d56a7c4b841885R133 for the CommonJS named exports PR. I think that would be a worthy addition and important inclusion for merging this.

Comment threadlib/internal/modules/esm/translators.js Outdated
Comment threadlib/internal/modules/esm/translators.js Outdated
@littledan

Copy link
Copy Markdown

Hey, I'm working on standards-track Wasm/ESM integration at https://github.com/WebAssembly/esm-integration/tree/master/proposals/esm-integration . I hope we can work out semantics here that match the proposed standard. I don't have any planned changes to semantics, but over the next few days I'll be trying to clean up the explainer and spec text to be more clear and well-layered with other changes.

@nodejs-ci
nodejs-ciforce-pushed the modules-lkgr branch 3 times, most recently from be696f2 to 325886dCompareMarch 3, 2019 09:07
@kevinkassimokevinkassimo mentioned this pull request Mar 4, 2019
@nodejs-ci
nodejs-ciforce-pushed the modules-lkgr branch 2 times, most recently from 6c03eed to 15daf0aCompareMarch 5, 2019 09:07
@MylesBorinsMylesBorins added the enhancement New feature or request label Mar 6, 2019
@MylesBorinsMylesBorins changed the title esm: experimental wasm modules[wip] esm: experimental wasm modulesMar 6, 2019
@nodejs-ci
nodejs-ciforce-pushed the modules-lkgr branch 2 times, most recently from bd5c877 to 0237465CompareMarch 7, 2019 09:06
@nodejs-ci
nodejs-ciforce-pushed the modules-lkgr branch 3 times, most recently from 9301a06 to e721cd2CompareMarch 14, 2019 08:10
@MylesBorins
MylesBorinsforce-pushed the modules-lkgr branch 2 times, most recently from 484d1fb to 7efc53dCompareMarch 18, 2019 22:07
Comment threadlib/internal/modules/esm/translators.js Outdated

@littledanlittledan 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.

This change from @devsnek LGTM, but there is more work to do.

@guybedford

Copy link
Copy Markdown
Contributor

I've pushed up a commit to support the exact import semantics here now. Because the v8 module system can only link Module objects that are SourceTextModule objects, we need to create one to handle this, just like we do for dynamic modules.

Should be good to go now, and thanks all for review.

Comment threadlib/internal/modules/esm/create_wasm_module.js Outdated
@guybedford

guybedford commented May 7, 2019 via email

Copy link
Copy Markdown
Contributor

Comment threadlib/internal/modules/esm/create_wasm_module.js Outdated
@ljharb

Copy link
Copy Markdown
Member

That can’t provide live bindings, or they can’t consume them?

@guybedford

Copy link
Copy Markdown
Contributor

@ljharb Web Assembly modules neither import or export live bindings.

@littledan

Copy link
Copy Markdown

What @guybedford says about WebAssembly modules matches the proposal which you can find in more detail at https://github.com/webassembly/esm-integration .

Comment threadlib/internal/modules/esm/default_resolve.js Outdated

@devsnekdevsnek 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.

modulo my comment and assuming this works, it looks pretty good!

@littledanlittledan 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.

The logic in translators.js makes sense to me, but I don't understand enough of the context to do a proper review.

It'd be good to have more tests here. We've gone through various iterations on semantics in this code review, so it'd be good to have tests for some of those scenarios, for example.

cc @Ms2ger

@guybedford

Copy link
Copy Markdown
Contributor

Upstream PR created at nodejs/node#27659.

@ljharb
ljharb deleted the wasm-modules branch May 20, 2019 15:57
nodejs-ci pushed a commit that referenced this pull request Aug 21, 2019
BUGFIXES
* [`27cccfbda`](npm/cli@27cccfb)
[#223](npm/cli#223) vulns → vulnerabilities in
npm audit output ([@sapegin](https://github.com/sapegin))
* [`d5e865eb7`](npm/cli@d5e865e)
[#222](npm/cli#222)
[#226](npm/cli#226) install, doctor: don't crash
if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin),
[@isaacs](https://github.com/isaacs))
* [`5b3890226`](npm/cli@5b38902)
[#227](npm/cli#227)
[npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5)
Handle unhandledRejections, tell user what to do when encountering an
`EACCES` error in the cache. ([@isaacs](https://github.com/isaacs))
DEPENDENCIES
* [`77516df6e`](npm/cli@77516df)
`licensee@7.0.3` ([@isaacs](https://github.com/isaacs))
* [`ceb993590`](npm/cli@ceb9935)
`query-string@6.8.2` ([@isaacs](https://github.com/isaacs))
* [`4050b9189`](npm/cli@4050b91)
`hosted-git-info@2.8.2`
* [#46](npm/hosted-git-info#46)
[#43](npm/hosted-git-info#43)
[#47](npm/hosted-git-info#47)
[#44](npm/hosted-git-info#44) Add support for
GitLab subgroups ([@mterrel](https://github.com/mterrel),
[@isaacs](https://github.com/isaacs),
[@ybiquitous](https://github.com/ybiquitous))
* [`3b1d629`](npm/hosted-git-info@3b1d629)
[#48](npm/hosted-git-info#48) fix http
protocol using sshurl by default
([@fengmk2](https://github.com/fengmk2))
* [`5d4a8d7`](npm/hosted-git-info@5d4a8d7)
ignore noCommittish on tarball url generation
([@isaacs](https://github.com/isaacs))
* [`1692435`](npm/hosted-git-info@1692435)
use gist tarball url that works for anonymous gists
([@isaacs](https://github.com/isaacs))
* [`d5cf830`](npm/hosted-git-info@d5cf830)
Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs))
* [`e518222`](npm/hosted-git-info@e518222)
Use LRU cache to prevent unbounded memory consumption
([@iarna](https://github.com/iarna))
PR-URL: nodejs/node#29023
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@MylesBorins@devsnek@guybedford@littledan@ljharb@weswigham@GeoffreyBooth@gengjiawen