esm: WebAssembly.Global unwrapping for Wasm Namespaces #57525
Conversation
|
Review requested:
|
nodejs-github-bot
added
esm
guybedford
force-pushed
the
wasm-globals-ns-unwrapping
branch
3 times, most recently
from
0f49b0a to
91278fe
Compare
|
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 WebAssembly/esm-integration#104 has landed. The Wasm CG vote was 6 for, 15 neutral and 1 against. The 1 vote against has since been resolved via WebAssembly/esm-integration#108. I'm therefore planning to post a summary on WebAssembly/esm-integration#104 before landing that soon, unless you have outstanding concerns. |
To clarify, "land this" means land it unflagged?
Wasn't that a temperature check? I did not get the sense that we had browser agreement to eventually implement the unwrapping. |
The ESM Integration in Node.js remains flagged overall, there are no plans to have a separate flag for this feature, no.
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? |
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. |
|
WebAssembly/esm-integration#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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57525 +/- ##
==========================================
+ Coverage 89.64% 90.16% +0.52%
==========================================
Files 630 630
Lines 186470 186521 +51
Branches 36305 36622 +317
==========================================
+ Hits 167160 168185 +1025
+ Misses 12073 11127 -946
+ Partials 7237 7209 -28
🚀 New features to boost your workflow:
|
guybedford
force-pushed
the
wasm-globals-ns-unwrapping
branch
from
91278fe to
13a34c8
Compare
guybedford
force-pushed
the
wasm-globals-ns-unwrapping
branch
from
13a34c8 to
7e69a90
Compare
|
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? |
|
@JakobJingleheimer tests are passing here finally. |
| const { module } = createDynamicModule(imports, exports, url, (reflect) => { | ||
| const createDynamicModule = require('internal/modules/esm/create_dynamic_module'); | ||
|
|
||
| const { module } = createDynamicModule([...importsList], [...exportsList], url, (reflect) => { |
There was a problem hiding this comment.
Spread might not be safe even on Safe* objects. @aduh95 I think you know better.
This is a draft PR implementing a Wasm ESM Integration PR for global unwrapping in WebAssembly/esm-integration#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.Globalrequiring aglobal.valuegetter 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 justMemory,Table,FunctionandGlobal.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.