feat: add native source maps support by fraidev · Pull Request #1241 · denoland/deno_core

fraidev · 2025-11-12T05:14:25Z

Related:
denoland/deno#4499
denoland/deno#21988

Summary by CodeRabbit

New Features
- Added support for external source maps and inline data URL source maps in error reporting and stack traces.
Improvements
- Enhanced path resolution logic for more readable and accurate file references in error messages.

codecov-commenter · 2025-11-13T22:10:07Z

Codecov Report

❌ Patch coverage is 94.52555% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.39%. Comparing base (0c7f83e) to head (22bebf7).
⚠️ Report is 377 commits behind head on main.

Files with missing lines	Patch %	Lines
core/source_map.rs	82.50%	7 Missing ⚠️
core/modules/loaders.rs	0.00%	6 Missing ⚠️
core/modules/map.rs	96.77%	1 Missing ⚠️
core/modules/tests.rs	99.21%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1241      +/-   ##
==========================================
- Coverage   81.43%   80.39%   -1.05%     
==========================================
  Files          97      105       +8     
  Lines       23877    27277    +3400     
==========================================
+ Hits        19445    21929    +2484     
- Misses       4432     5348     +916

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bartlomieju · 2025-11-17T09:01:15Z

+    r#"export function greet(name) {{
+  console.log("Hello, " + name);
+}}
+
+greet("World");


I think this code needs to throw an error to test it fully - otherwise we're not checking if the source map is actually getting applied, right?

good catch! I update the test to throw an error.

bartlomieju · 2025-11-17T09:02:37Z

+  // External source maps with relative paths (./) are loaded lazily when needed
+  // We've verified that the module loads successfully


If an error is not thrown in a test, is the source map actually loaded? Maybe we need some more assertions here, like checking if the load_external_source_map was called?

I added more assets here

bartlomieju · 2025-11-17T09:03:26Z

+  // Verify the error shows the synthetic path from the bundle
+  // Should show as absolute: /a.ts (not with excessive ../)
+  assert!(
+    err_str.contains("/a.ts") || err_str.contains("file:///a.ts"),
+    "Error should contain synthetic bundle path: {}",
+    err_str
+  );


Why is it either /a.ts or file:///a.ts?

good catch! Should be only file:///a.ts. Fixed!

bartlomieju · 2025-11-17T09:04:22Z

  // TODO(bartlomieju): I feel like these two should be cleared when Isolate
  // reaches "near heap limit" to free up some space. This needs to be confirmed though.
-  maps: HashMap<String, Option<SourceMap>>,
+  maps: HashMap<String, Option<Arc<SourceMap>>>,


Ouch, now I'm a bit confused - what's the difference between maps and native_source_maps? Since they look almost identical is there a reason to keep them separate?

Oh! Actually, we don't need native_source_maps. I remove it to use maps directly.

bartlomieju

Looks, great, a few questions left before we merge it

bartlomieju · 2025-11-17T23:56:32Z

@coderabbitai review

coderabbitai · 2025-11-17T23:56:39Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2025-11-17T23:56:43Z

Walkthrough

The pull request enhances source map support by introducing external source map loading capabilities. Changes include adding a new trait method for loading external source maps, extracting and processing source map URLs from V8 modules, refining path abbreviation logic for error messages, and managing Arc-wrapped source maps with URL tracking.

Changes

Cohort / File(s)	Summary
Path Abbreviation Improvements `core/error.rs`	Introduces `relative_specifier_within()` helper for conditional relative path abbreviation. Refines data URL and file:// URL handling to prefer within-CWD relative paths, with fallback to absolute paths. Extends tests with comprehensive `test_relative_specifier_within` covering edge cases and various path scenarios.
Module Loader Interface Extension `core/modules/loaders.rs`	Adds `load_external_source_map(source_map_url)` method to `ModuleLoader` trait with default None implementation. Updates `TestingModuleLoader` to propagate the new hook to its inner loader.
Source Map URL Extraction `core/modules/map.rs`	Extracts native source map URLs from V8 module scripts during loading. Handles both data: URLs (decoded via sourcemap) and external URLs (resolved relative to module URL), registering both with `SourceMapper`. Adds `DATA_PREFIX` constant and `sourcemap::DecodedMap` import.
Source Map Test Coverage `core/modules/tests.rs`	Introduces comprehensive test suite covering native source maps with trailing content, external source maps with relative paths, absolute paths outside CWD, and synthetic bundle paths. Adds `ExternalSourceMapLoader` struct implementing `ModuleLoader` for test scenarios.
Source Map Storage & Resolution `core/source_map.rs`	Changes `maps` field to use `Arc<SourceMap>` for shared ownership. Adds `source_map_urls` field tracking external URLs. Introduces `add_source_map()` and `add_source_map_url()` methods. Extends resolution logic to fetch external source maps via loader and attempt relative URL resolution for mapped source files. Applies 150-character filter to mapped lines.

Sequence Diagram(s)

sequenceDiagram
    participant Module as V8 Module Script
    participant MapLoader as map.rs
    participant ModuleLoader as ModuleLoader
    participant SourceMapper as SourceMapper
    participant DataDecoder as sourcemap

    Module->>MapLoader: Load module with sourceMappingURL
    alt Data URL
        MapLoader->>DataDecoder: Decode data: URL
        DataDecoder->>SourceMapper: Register decoded SourceMap (Arc)
    else External URL
        MapLoader->>MapLoader: Resolve URL relative to module
        MapLoader->>ModuleLoader: load_external_source_map(url)
        ModuleLoader->>SourceMapper: Register external URL
    end
    SourceMapper->>SourceMapper: Store with Arc<SourceMap>

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas requiring extra attention:

core/source_map.rs: Arc wrapping of SourceMap and new source map resolution logic with external fetching; verify thread-safety assumptions and cache invalidation semantics.
core/modules/map.rs: Correct relative URL resolution against module URLs and proper handling of both data: and external URL branches.
core/error.rs: Logic for conditional relative path abbreviation in relative_specifier_within() and edge cases around CWD boundaries.
Test coverage validation: Ensure the new test suite in core/modules/tests.rs adequately exercises the integration between external source map loading and error message formatting.

Poem

🐰 Mapping errors through the code,
External paths now overflowed,
With Arc-wrapped maps so neat,
The source trails find their treat,
No more mystery in the stack! 🗺️

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: add native source maps support' accurately captures the main objective of the changeset, which introduces comprehensive native source maps support across multiple files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

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

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa15f5a and d5e9176.

📒 Files selected for processing (5)

core/error.rs (3 hunks)
core/modules/loaders.rs (2 hunks)
core/modules/map.rs (3 hunks)
core/modules/tests.rs (1 hunks)
core/source_map.rs (7 hunks)

coderabbitai · 2025-11-18T00:05:35Z

+      if source_mapping_url.starts_with(DATA_PREFIX) {
+        if let Ok(DecodedMap::Regular(sm)) =
+          sourcemap::decode_data_url(&source_mapping_url)
+        {
+          self
+            .source_mapper
+            .borrow_mut()
+            .add_source_map(module_name, sm);
+        }
+      } else {
+        // Resolve external source map URL relative to the module URL
+        let resolved_url =
+          if let Ok(module_url) = ModuleSpecifier::parse(name.as_str()) {
+            module_url
+              .join(&source_mapping_url)
+              .unwrap_or(module_url)
+              .to_string()
+          } else {
+            source_mapping_url
+          };
+
+        self
+          .source_mapper
+          .borrow_mut()
+          .add_source_map_url(module_name, resolved_url);


⚠️ Potential issue | 🟠 Major

Support all decoded data URL map variants

decode_data_url() can return DecodedMap::Index / DecodedMap::IndexOwned (multi-section maps) in addition to Regular*. By matching only DecodedMap::Regular we silently drop any inline source map that happens to be an index, which means no native mapping for many real-world bundler outputs (Rollup/esbuild frequently emit sectioned maps). Please accept every variant (e.g. call decoded.into_sourcemap() or handle the index cases explicitly) before stashing the map; otherwise we regress the new feature for a large class of inputs.

bartlomieju

LGTM, let's try it out in the CLI

fraidev force-pushed the native_sourcemaps branch 8 times, most recently from ce3be99 to c67c483 Compare November 13, 2025 05:35

fraidev changed the title ~~[draft] feat: add native SourceMap support~~ feat: add native SourceMap support [draft] Nov 13, 2025