deno icon indicating copy to clipboard operation
deno copied to clipboard

fix(compile): wasm files might not be a part of module graph

Open CertainLach opened this issue 3 months ago • 2 comments

Fixes: https://github.com/denoland/deno/issues/31456

deno compile analyzes all --included files as module graph roots, however wasm files might not be used in module graph at all, and instead fetch()ed and WebAssembly.instantiated, in which case their imports should not be analyzed.

It should not break imported wasm files, as they will be added to module graph as a dependency anyway.

CertainLach avatar Nov 29 '25 23:11 CertainLach

Walkthrough

Internal helper predicates in the compile module are renamed for clarity: is_module_graph_module becomes is_module_graph_root_module and is_module_graph_media_type becomes is_module_graph_root_media_type. The root-media-type logic is extended to explicitly handle Wasm as non-root-eligible with accompanying comments. All call sites are updated to reference the renamed predicates. Existing behavior for non-file schemes remains unchanged. The match arms are expanded to differentiate which media types qualify as root-eligible, reflecting the new naming convention.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly addresses the main change: preventing wasm files from being treated as module graph roots, which matches the core functionality fix in the PR.
Description check ✅ Passed The description explains the issue (wasm imports being resolved when they shouldn't be), references the linked issue, and clarifies that the fix prevents analysis of wasm files not part of the module graph.
Linked Issues check ✅ Passed The PR changes directly address issue #31456 by modifying root-media-type logic to exclude wasm files from module graph analysis, preventing unwanted import resolution.
Out of Scope Changes check ✅ Passed The changes are focused on renaming predicates and adjusting wasm handling in the compile logic, all aligned with fixing wasm file module graph treatment.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 76068b214b3d01389b75418687a09226ff232dbc and cc9e180e818ed493b0c24c5cefb05635bc9d2060.

📒 Files selected for processing (1)
  • cli/tools/compile.rs (3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
cli/tools/**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

CLI tools should be implemented in cli/tools/<tool> or cli/tools/<tool>/mod.rs

Files:

  • cli/tools/compile.rs
**/*.rs

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.rs: For debugging Rust code, set breakpoints in IDE debuggers (VS Code with rust-analyzer, IntelliJ IDEA) or use lldb directly Use eprintln!() or dbg!() macros for debug prints in Rust code

Files:

  • cli/tools/compile.rs
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:19:37.808Z
Learnt from: CR
Repo: denoland/deno PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:19:37.808Z
Learning: Applies to cli/module_loader.rs : Module loading and resolution is handled in `cli/module_loader.rs`

Applied to files:

  • cli/tools/compile.rs
🧬 Code graph analysis (1)
cli/tools/compile.rs (1)
libs/resolver/emit.rs (3)
  • media_type (395-395)
  • media_type (413-415)
  • media_type (435-437)
🔇 Additional comments (3)
cli/tools/compile.rs (3)

349-354: Helper rename and root-only semantics look correct

Constraining is_module_graph_root_module to “root-ness” and delegating only file URLs to media-type inspection matches how get_module_roots_and_include_paths uses it. Non-file includes still become roots, and file includes are filtered by media type as intended.


356-381: Wasm exclusion from module graph roots aligns with compile behavior

The media-type filter now explicitly excludes MediaType::Wasm from being a graph root while still allowing all JS/TS variants as roots. That ensures .wasm files provided via --include or discovered under included directories are treated as side assets unless actually imported, which is exactly what’s needed to avoid resolving their internal imports.


427-432: Include handling now avoids unnecessary wasm graph roots

Using is_module_graph_root_module for each --include and is_module_graph_root_media_type inside analyze_path means:

  • Direct .wasm includes no longer become module roots.
  • .wasm files under included directories are skipped as roots but remain available via include_paths.

This should prevent the reported “import not a dependency” errors for runtime-instantiated wasm without affecting wasm modules that are actually imported.


Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 29 '25 23:11 coderabbitai[bot]

Was testing this change, and have encountered that fetching wasm doesn't work, as it seems like fetch() with file urls does not support reading files from DenoRtSys

This does not affect utility of this patch for cases when the wasm files are only used for frontend asset storage purposes, and if someone wants to use wasm file in deno itself - it is still possible by using WebAssembly.instantiate(new WebAssembly.Module(await Deno.readFile(...)))

CertainLach avatar Nov 30 '25 00:11 CertainLach