runtime: implement WebAssembly/ESM integration for .wasm imports - #35587
runtime: implement WebAssembly/ESM integration for .wasm imports#35587robobun wants to merge 9 commits into
Code review found 7 potential issues
Found 5 candidates, confirmed 7. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 6 |
| 🟣 Pre-existing | 1 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/jsc/bindings/ModuleLoader.cpp:1179-1201 |
Wasm tag handled in fetchESMSourceCode but not sibling consumer handleVirtualModuleResult |
| 🟡 Nit | test/regression/issue/12434/12434.test.ts:17-19 |
Test suite covers only import-free wasm — the ESM-integration linking path is untested |
| 🟡 Nit | src/runtime/jsc_hooks.rs:3251-3258 |
?query check silently overrides explicit with { type: "wasm" } attribute |
| 🟡 Nit | test/regression/issue/12434/12434.test.ts:1-12 |
Test file misplaced in test/regression/ for a feature request, not a regression |
| 🟣 Pre-existing | src/jsc/bindings/ModuleLoader.cpp:1197-1201 |
Wasm branch leaks +1 refs on ResolvedSource.specifier / .source_url (pre-existing pattern) |
Annotations
Check warning on line 1201 in src/jsc/bindings/ModuleLoader.cpp
claude / Claude Code Review
Wasm tag handled in fetchESMSourceCode but not sibling consumer handleVirtualModuleResult
`Bun__transpileVirtualModule` is a second entry into `transpile_source_code_inner` that can now return `tag == Wasm`, but its consumer — `handleVirtualModuleResult`'s `OnLoadResultTypeCode` branch (ModuleLoader.cpp:389-396) — still hands the result straight to `Zig::SourceProvider::create` without checking the tag. A `Bun.plugin` `b.module()` whose specifier ends in `.wasm` and omits `loader` now hits `SyntaxError: Invalid character '\\0'` on the `\0asm` header (previously it silently produced a
Check warning on line 19 in test/regression/issue/12434/12434.test.ts
claude / Claude Code Review
Test suite covers only import-free wasm — the ESM-integration linking path is untested
All 7 tests use `add.wasm`, which per this comment has *no import section* — so the defining behaviour of WebAssembly/ESM integration (a wasm module's `(import "mod" "name" ...)` entries becoming ES module dependencies routed through `moduleLoaderResolve`/`moduleLoaderFetch` with a `.wasm`-keyed referrer) is never exercised. The `moduleKey`/`SourceOrigin` wiring at ModuleLoader.cpp:1197-1200 exists precisely for that resolve→fetch→link chain; per REVIEW.md ("Cover the variant matrix" / "deleting
Check warning on line 3258 in src/runtime/jsc_hooks.rs
claude / Claude Code Review
?query check silently overrides explicit with { type: "wasm" } attribute
The `contains_char(specifier, b'?')` demotion runs after the loader is already determined, so it fires even when `Loader::Wasm` was set by an explicit `with { type: 'wasm' }` attribute rather than inferred from the `.wasm` extension. `await import('./module?v=1', { with: { type: 'wasm' } })` — where the type attribute is the *only* thing selecting the wasm loader — is silently demoted to the file loader and returns `{ default: '<path>' }`. The #16476 compat shim should apply only to extension-in
Check warning on line 12 in test/regression/issue/12434/12434.test.ts
claude / Claude Code Review
Test file misplaced in test/regression/ for a feature request, not a regression
This test file belongs in `test/js/bun/wasm/` (which already exists and holds `wasi.test.js`), not `test/regression/issue/`. Per CLAUDE.md's Test Organization section, `test/regression/issue/` is reserved for true regressions — behavior that worked in a previous release and then broke — whereas #12434 is a feature request (WebAssembly/ESM integration was never implemented; the path-string return was a placeholder). Consider moving to `test/js/bun/wasm/esm-integration.test.ts`.
Check notice on line 1201 in src/jsc/bindings/ModuleLoader.cpp
claude / Claude Code Review
Wasm branch leaks +1 refs on ResolvedSource.specifier / .source_url (pre-existing pattern)
Pre-existing (not introduced by this PR): this branch never derefs the +1 refs on `res->result.value.specifier` / `.source_url` that Rust hands over via `dupe_ref()`/`create_if_different()` — the contract-holder is `Zig::SourceProvider::~SourceProvider` (ZigSourceProvider.cpp:188-190), but `JSC::WebAssemblySourceProvider` never receives the `ResolvedSource`, and `ResolvedSourceCodeHolder` only derefs `source_code`. The sibling `JSONForObjectLoader`/`ExportsObject`/`ExportDefaultObject` branches