nodejs
esm: WebAssembly.Global unwrapping for Wasm Namespaces
This is a draft PR implementing a Wasm ESM Integration PR for global unwrapping in https://github.com/WebAssembly/esm-integration/pull/104.
Opening here as a draft only for feedback and discussion, as it helps to have a concrete implementation for understanding and experimentation.
With this change, WebAssembly namespaces exporting globals have the global value provided directly on the namespace exports, instead of exporting WebAssembly.Global requiring a global.value getter access in turn to retrieve the underlying binding. With this, Wasm namespaces can effectively export any JS binding type when using Wasm GC, instead of just Memory, Table, Function and Global.
We do this by unwrapping the global on the Wasm namespace exports interface, while still linking Wasm - Wasm imports to the underlying global. Ideally this would support live exported bindings as well, but that would require V8 work in turn.
Is the plan to gate this behind an experimental flag, independently of unflagging Wasm ESM integration generally? I didn't get the sense there's consensus for this from the Wasm CG.
@syg I'll only land this once https://github.com/WebAssembly/esm-integration/pull/104 has landed. The Wasm CG vote was 6 for, 15 neutral and 1 against. The 1 vote against has since been resolved via https://github.com/WebAssembly/esm-integration/pull/108. I'm therefore planning to post a summary on https://github.com/WebAssembly/esm-integration/pull/104 before landing that soon, unless you have outstanding concerns.
I'll only land this once https://github.com/WebAssembly/esm-integration/pull/104 has landed
To clarify, "land this" means land it unflagged?
The Wasm CG vote was 6 for, 15 neutral and 1 against. The 1 vote against has since been resolved via https://github.com/WebAssembly/esm-integration/pull/108.
Wasn't that a temperature check? I did not get the sense that we had browser agreement to eventually implement the unwrapping.
To clarify, "land this" means land it unflagged?
The ESM Integration in Node.js remains flagged overall, there are no plans to have a separate flag for this feature, no.
Wasn't that a temperature check? I did not get the sense that we had browser agreement to eventually implement the unwrapping.
There is no requirement for Phase 3 specification changes to go through CG vote. The temperature check was to ensure we are making all members aware and to engage on any concerns. I wasn't aware that there was any outstanding disagreement - do you have outstanding concerns?
I wasn't aware that there was any outstanding disagreement - do you have outstanding concerns?
My concern is that this change has had the least baking time, and if Node will be the first one to ship for a while, by the time the browsers get to implementing and giving implementation feedback, there may be interop issues with Node's behavior.
By landing this flagged we can continue to build implementation feedback per Phase 3 process. We will also continue to bring implementation updates to the CG to ensure adequate feedback throughout the process.
Point taken on ensuring baking time, and it's good for others to be aware of this as well. We very much would like to see this alignment come together for a fully supported WebAssembly Module Record in V8.
Unflagging will not be rushed as discussed, but if the concern of implementation divergence leads to delays of many months or next year then that does start to affect adoption quite considerably, and put Node.js in a position where we may have to unflag first and without Chrome's implementation experience.
To avoid any implementation divergence scenario, it would help to have any concerns from Chromium clearly raised early in the process, and also to have a clear statement if there remains after shipping the source phase no intent to ship the instance phase. It would be a real shame to continue to hold Node.js and other environments back in this regard unnecessarily by not providing clear intent when these environments could be benefitting from this feature already. If Chromium does not want to ship the instance phase, it cannot simultaneously block the feature for other environments like Node.js and Deno that do want to ship it, by claiming it doesn't have implementation experience when that is a choice that was made by Chrome not to engage in implementation and not a choice made by Node.js and Deno.
https://github.com/WebAssembly/esm-integration/pull/104 has now landed, so I'm marking that as non-draft for further review.
@syg please do open upstream issues if you have any concerns here at all or items to discuss, and feel free to reach out to me directly.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 90.19%. Comparing base (
cd68e35) to head (c4570b6). Report is 142 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #57525 +/- ##
==========================================
- Coverage 90.19% 90.19% -0.01%
==========================================
Files 635 635
Lines 187224 187274 +50
Branches 36765 36775 +10
==========================================
+ Hits 168876 168904 +28
- Misses 11131 11138 +7
- Partials 7217 7232 +15
| Files with missing lines | Coverage Ξ | |
|---|---|---|
| lib/internal/modules/esm/create_dynamic_module.js | 95.78% <100.00%> (ΓΈ) |
|
| lib/internal/modules/esm/translators.js | 92.26% <100.00%> (+0.69%) |
:arrow_up: |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks @JakobJingleheimer for the review here, I believe I've covered all the feedback.
Thanks! I think it does look/flow better now :) I see there are test failures now; I'm not sure if they were there before? If they're new, sorry π Do you want help debugging?
Landed in https://github.com/nodejs/node/commit/a7e4a402986003f3a07107c388df06dcea1fa3c1.
This does not land cleanly on v22.x-staging and would require a manual backport PR if we want it there
