Align deno/module WASM import namespace with other targets#4871
Conversation
83b89b7 to
ab9d461
Compare
There was a problem hiding this comment.
Pull request overview
This PR migrates the Deno target to use a dual-file ES module structure, aligning it with the bundler and experimental-nodejs-module targets for consistency.
Key Changes:
- Introduced dual-file structure: main entry file (
.js) and bindings file (_bg.js) - Changed WASM import namespace from
__wbindgen_placeholder__to./{name}_bg.jsin generated WAT files - Unified code generation path for Deno with other ESM integration targets
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/cli-support/src/lib.rs |
Added Deno to ESM integration mode check and dual-file output logic |
crates/cli-support/src/js/mod.rs |
Migrated Deno target to unified ESM integration code path with dedicated handling for Deno wasm loading |
crates/cli/tests/wasm-bindgen/reference.rs |
Updated test to check _bg.js output for Deno target |
crates/cli/tests/reference/targets-target-deno.wat |
Updated WASM imports from __wbindgen_placeholder__ to ./reference_test_bg.js namespace |
crates/cli/tests/reference/targets-target-deno.js |
Main entry file now imports from _bg.js and re-exports bindings |
crates/cli/tests/reference/targets-target-deno.bg.js |
New bindings file containing __wbg_set_wasm, exported functions, and glue code |
crates/cli/tests/reference/targets-target-deno-mvp.* |
MVP variant updated with same structural changes |
crates/cli/tests/reference/targets-target-deno-atomics.* |
Atomics variant updated with same structural changes |
crates/cli/tests/reference/import-target-deno.* |
Import test files updated to reflect new dual-file structure and namespace changes |
CHANGELOG.md |
Documented breaking change with description of structural and namespace changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
guybedford
left a comment
There was a problem hiding this comment.
Thinking about this some more, how does this contrast with the source phase imports form? Is there alignment with that or not here?
|
The source-phase alignment will be implemented in #4850. As we bring all targets into alignment, the differences between their glue code will diminish. I will open a dedicated discussion to explore how to unify the glue code across targets — this will reduce duplication and improve maintainability. |
Thanks, my point here is that we have two kinds of designs in play - the old "wasm sandwich" model with the bg file, which is good for instance outputs for bundlers that support tree-shaking of Wasm, and then the more modern instantiation models using source phase that are single file but don't require separate JS files and can have one JS file, and then dynamic instnatiation models which can align with this source phase model. That is, it is only when using So my preference would be unifying on the source phase output model, but then retaining the "bg" model in the direct Wasm instance phase cases only. Let me know if that makes sense? |
|
Thanks for the clarification — I should have explained my original intention more clearly. I was actually planning to change the source phase output to use a separate That said, I completely understand and agree with your point—having multiple files adds extra network requests or file reads, so prioritizing performance over maintainability makes sense. Could you confirm whether this is the design direction the wasm-bindgen team has chosen? If so, I’ll be happy to adjust this PR back to the single-file model. |
|
Yes, exactly, thats the way we need to do it rather to only have bg for the direct Wasm instance phase output cases, and not otherwise, as we must prioritise output simplicity first and foremost. |
726dd8e to
68d3425
Compare
deno WASM import namespace with other targetsdeno/module WASM import namespace with other targets
d8caa93 to
2f37aaa
Compare
|
This looks good to land to me, we just need to update the PR description and add a changelog entry. |
Update WASM import namespace from __wbindgen_placeholder__ to ./{name}_bg.js
2f37aaa to
09fa99f
Compare
|
Done. |
Description
This PR updates the WASM import namespace for the Deno target to align with other targets, replacing the placeholder namespace with a relative module path.
Changes
WASM Import Namespace Change
This change also applies to the source-phase target, which reuses the Deno implementation for consistency.
Checklist