-
- Notifications
You must be signed in to change notification settings - Fork 19
esm: experimental wasm modules #46
Conversation
| related: nodejs/node#18972 i think we should wait for wasm modules to be specified, at which point v8 would provide them for us. |
| @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 |
7c92011 to c809f78 Compare | @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. |
da0667d to f0a4ff5 Compare | 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. |
be696f2 to 325886d Compare 6c03eed to 15daf0a Compare 15daf0a to 03fb3b1 Compare f0a4ff5 to 9ae7a1d Compare bd5c877 to 0237465 Compare bec588f to ea59221 Compare 9ae7a1d to aa1e86e Compare 9301a06 to e721cd2 Compare 484d1fb to 7efc53d Compare
littledan left a comment
There was a problem hiding this comment.
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.
| 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. |
| Yes Web Assembly modules dont have live bindings, instead features like mutable global are captured through a static binding interface. …On Tue, 07 May 2019 at 09:12, Wesley Wigham ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/internal/modules/esm/create_wasm_module.js <#46 (comment)> : > +${[...importModules].map((module) => { + const specifierStr = JSON.stringify(module); + const importNames = new Set( + imports + .filter(({ module: m }) => m === module) + .map(({ name }) => name) + ); + const impts = + [...importNames].map((name) => `${name} as $${name}`).join(', '); + const defs = [...importNames].map((name) => `${name}: $${name}`).join(', '); + return `import { ${impts} } from ${specifierStr}; + imports[${specifierStr}] = { ${defs} };`; + }).join('\n')} +const { exports } = new WebAssembly.Instance(import.meta.module, imports); +${WebAssembly.Module.exports(module) + .map(({ name }) => `export const ${name} = exports.${name};`).join('\n')} A facade like this means that there won't be live bindings for the wasm module, no? Are wasm exports supposed to be live bound? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAESFSR7HIRBNIA3HR4MATDPUETUZANCNFSM4G2XCCQQ> . |
| That can’t provide live bindings, or they can’t consume them? |
| @ljharb Web Assembly modules neither import or export live bindings. |
| What @guybedford says about WebAssembly modules matches the proposal which you can find in more detail at https://github.com/webassembly/esm-integration . |
devsnek left a comment
There was a problem hiding this comment.
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!
littledan left a comment
There was a problem hiding this comment.
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
| Upstream PR created at nodejs/node#27659. |
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>
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