feat: add load_external_source_map method to ModuleLoader #31268
Conversation
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
4 times, most recently
from
c53725d to
5a94d7c
Compare
fraidev
changed the title
fraidev
changed the title
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
5a94d7c to
2ddd584
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading external source maps by implementing the load_external_source_map method in the ModuleLoader trait. The changes enable proper source mapping for JavaScript files with external .map files and files with inline base64-encoded source maps, including those with trailing content after the sourceMappingURL comment.
Key changes:
- Implements
load_external_source_mapin bothCliModuleLoaderInnerandEmbeddedModuleLoader - Enhances
get_source_mapped_source_lineto read original source files from the file system when not in the module graph - Adds comprehensive test coverage for various sourcemap scenarios
Reviewed Changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/module_loader.rs | Implements load_external_source_map method to fetch external source maps via file_fetcher, refactors get_source_mapped_source_line to support files outside the module graph, extracts helper function extract_source_line |
| cli/rt/run.rs | Implements load_external_source_map for EmbeddedModuleLoader to load source maps from the embedded module store |
| Cargo.toml | Updates deno_core dependency to use a git branch with native sourcemap support |
| Cargo.lock | Updates dependency lockfile with git-based deno_core, deno_ops, and serde_v8 versions |
| tests/specs/run/sourcemap_external/* | Adds test for external source map files (.js.map) |
| tests/specs/run/sourcemap_inline_base64/* | Adds test for inline base64-encoded source maps |
| tests/specs/run/sourcemap_none/* | Adds test for files without source maps |
| tests/specs/run/sourcemap_relative_paths/* | Adds tests for source maps with relative paths both within and outside CWD |
| tests/specs/run/sourcemap_trailing_content/* | Adds test for source maps with trailing content after sourceMappingURL |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fraidev
force-pushed
the
native_sourcemaps_core
branch
4 times, most recently
from
c7e15bf to
e28defa
Compare
|
|
||
| fn load_external_source_map( | ||
| &self, | ||
| source_map_url: &str, |
There was a problem hiding this comment.
It feels like we should respect permission checks here - though I'm not sure how to determine if we should use "static" or "dynamic" permissions here. @dsherret any ideas?
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
This looks really good to me, I'm just worried about the missing permission check. Let's discuss during the meeting
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
e28defa to
7355fdd
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the workspace Changes
Sequence Diagram(s)sequenceDiagram
participant User as Runtime/Error flow
participant Loader as CliModuleLoader
participant Graph as ModuleGraph / PreparedModule
participant Fetcher as FileFetcher / Cache
participant Embedded as EmbeddedModuleLoader
User->>Loader: request source-mapped line for specifier
Loader->>Graph: resolve specifier & prepare module (sync)
Graph-->>Loader: prepared module bytes
alt inline source map in prepared module
Loader->>Loader: extract inline map (extract_source_line)
Loader-->>User: return inline map / mapped line
else external sourceMappingURL present
Loader->>Loader: parse source_map_url
Loader->>Embedded: load_external_source_map(url)
Embedded->>Fetcher: fetch module bytes from store/cache
Fetcher-->>Embedded: module bytes (if present)
Embedded-->>Loader: source map bytes as Cow<[u8]>
Loader-->>User: return external map / mapped line
else module not in graph but on filesystem
Loader->>Fetcher: read file from filesystem
Fetcher-->>Loader: file bytes
Loader->>Loader: extract line / map
Loader-->>User: return mapped line or None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
7355fdd to
b3128a5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/rt/run.rs (1)
586-593: Standaloneload_external_source_mapwiring looks correct; confirm data layout and consider a brief doc commentFor the embedded/standalone case this implementation is straightforward:
- Parses
source_map_urlinto aUrl.- Reads from
self.shared.modules, which is already the canonical store for snapshot contents.- Returns
Cow::Owned(data.data.to_vec()), assuming the corresponding entry’sdatafield contains the raw sourcemap bytes for the external map.That matches the model where external sourcemaps are embedded as separate modules/assets in the snapshot and avoids any extra I/O at runtime.
Two small follow‑ups:
- Please double‑check that the snapshot writer indeed stores external sourcemaps as dedicated entries whose
datafield is the sourcemap payload (and not, for example, a JS wrapper), so consumers interpret this correctly.- A one‑line doc comment on this method (mirroring the trait’s intent) would make the contract very clear for future maintainers.
🧹 Nitpick comments (4)
tests/specs/run/sourcemap_external/external.js (1)
1-8: Fixture correctly exercises external sourcemap stack tracesThe fixture reliably throws and points at
external.js.map, which is exactly what the external sourcemap tests need. The"Original TypeScript file"comment is slightly misleading for a.jsfile, but harmless—consider clarifying it or pointing atoriginal.tsif you touch this again.Cargo.toml (1)
64-65: Ensure git override ofdeno_coreis temporary and reverted before mergePointing
deno_coreat a personal fork/branch is reasonable for a draft while iterating, but this should not ship onmain. Please plan to:
- Switch back to the official
deno_coresource (versioned crate or official git repo/branch) once the native sourcemaps work lands upstream.- Drop or update the commented version line accordingly.
cli/module_loader.rs (2)
1213-1249: Inline and external sourcemap loading logic looks structurally sound
get_source_map:
- Resolves
file_nameto aModuleSpecifierand filters to the same external schemes we previously supported.- Uses
load_prepared_module_for_source_map_syncagainst the current graph, thensource_map_from_codeover the prepared source bytes. That keeps sourcemap extraction tied to the same pipeline that produced the code, which is good for correctness and avoids direct disk layout assumptions.
load_external_source_map:
- Resolves
source_map_urland usesfile_fetcher.get_cached_source_or_localto retrieve the bytes, returning an ownedCow<[u8]>.- This deliberately avoids hitting the network on sourcemap lookup and relies on caches/local file system, which seems like a reasonable trade‑off for stack traces.
One thing to clarify: both code paths bypass explicit
CheckSpecifierKind/permission routing and rely on the fact that these files have already been fetched or are local sources the runtime is allowed to see. If there are expectations around treating sourcemap reads as “static” vs “dynamic” file accesses, it may be worth documenting that here or threading the appropriate permission context intoget_cached_source_or_localin the future.
1251-1304: Source-mapped line lookup andextract_source_linehelper are correct; consider clarifying permission expectations
get_source_mapped_source_linenow:
- Resolves
file_nameto a specifier.- For
Js/Jsonmodules present in the graph, uses the in‑graphmodule.source.text, which is the right source of truth.- Early returns
NoneforWasm,Npm,Node, andExternalmodules, avoiding misleading line lookups where there is no meaningful source.- When the specifier is not in the graph, falls back to
file_fetcher.get_cached_source_or_localand delegates toextract_source_lineon the decoded contents. This nicely handles original TS/JS files that only exist on disk/cache but are referenced by sourcemaps.
extract_source_line:
- Uses
split('\n')instead of.lines()to preserve a terminating empty line, matching the comment.- Avoids allocating an intermediate
Vec, which is an improvement over the previous pattern.- Provides a consistent warning message when
line_numberis out of bounds.As with
load_external_source_map, this fallback introduces a direct read viaget_cached_source_or_localwithout an explicit permission token. If sourcemap‑driven reads should obey any particularPermissionsContainerpolicy (for example, mirroring the static vs dynamic distinction you already use inload_asset), it would be good to either:
- Document that sourcemap lookups are intentionally “best‑effort debugging helpers” that may read from cache/local disk, or
- Route this through a checked fetch helper similar to
load_asset.Functionally the logic looks right; the main open question is just how strict these reads should be.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (15)
Cargo.toml(1 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cli/rt/run.rs (1)
cli/module_loader.rs (1)
load_external_source_map(1233-1249)
cli/module_loader.rs (3)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)runtime/worker.rs (1)
get_source_mapped_source_line(1304-1315)cli/graph_container.rs (2)
graph(26-26)graph(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-x86_64
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug windows-x86_64
- GitHub Check: test debug macos-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug macos-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug linux-x86_64
🔇 Additional comments (12)
tests/specs/run/sourcemap_external/original.ts (1)
1-6: External sourcemap TS fixture looks goodStraightforward error-throwing fixture with a clear, scenario-specific message; suitable for exercising external sourcemap handling.
tests/specs/run/sourcemap_inline_base64/__test__.jsonc (1)
1-5: Inline base64 sourcemap test spec is consistentThe
args,output, andexitCodealign with theinline_base64.jsfixture and the fact it throws; structure matches other run specs.tests/specs/run/sourcemap_none/no_sourcemap.js (1)
1-6: No-sourcemap JS fixture is appropriateSimple, self-explanatory error-throwing script with no
sourceMappingURL, which is exactly what this scenario needs.tests/specs/run/sourcemap_relative_paths/src/original.ts (1)
1-6: Relative-path original TS fixture matches scenarioThe location under
src/and the error message clearly target the “within cwd / relative path” sourcemap case; implementation is minimal and correct.tests/specs/run/sourcemap_none/__test__.jsonc (1)
1-5: No-sourcemap test spec is consistentArguments, output file name, and exit code all line up with
no_sourcemap.jsthrowing an error and match the structure of the other sourcemap run specs.tests/specs/run/sourcemap_inline_base64/inline_base64.js (1)
1-8: Inline base64 JS + embedded sourcemap look correctThe script throws with a message that matches the embedded TS in the base64 data URL, and the inline
sourceMappingURLformat is appropriate for exercising inline sourcemap handling.tests/specs/run/sourcemap_trailing_content/trailing_content.js (1)
1-13: Trailing-content sourcemap fixture is well-structuredThis neatly captures the “inline sourcemap plus trailing comments/blank lines” scenario, with a clear issue reference and matching error message for debugging expectations.
tests/specs/run/sourcemap_trailing_content/__test__.jsonc (1)
1-5: Trailing-content test spec matches the fixtureThe command, expected output file, and non-zero exit code are all aligned with
trailing_content.jsthrowing as intended and are consistent with the other sourcemap specs.tests/specs/run/sourcemap_external/__test__.jsonc (1)
1-5: Test spec matches fixture behaviorCommand, output file, and exit code all line up with
external.jsthrowing an error once. Looks consistent with otherrunspecs.tests/specs/run/sourcemap_relative_paths/__test__.jsonc (1)
1-9: LGTM! Test configuration is well-structured.The test specification correctly configures the test to run
outside_cwd.js, expects exit code 1 (appropriate since the script throws an error), and references an output file for verification.tests/specs/run/sourcemap_relative_paths/outside_cwd.js (2)
1-6: LGTM! Test case is well-structured.The test correctly sets up a scenario to verify source map handling for files outside the current working directory. The function throws an error that will test whether the stack trace correctly references the mapped source location.
8-8: Test correctly validates absolute path handling in source maps.The expected output file uses
[WILDCARD]pattern matching (part of Deno's test framework) to validate stack traces. The test is working as designed:
- The inline source map contains
file:///absolute/path/outside/original.ts(absolute path intentionally outside CWD)- The expected output pattern
file:///[WILDCARD]original.tscorrectly matches resolved stack traces- Stack trace line numbers (3:1, 6:1) properly resolve through the source map to the original source content
- The implementation correctly handles absolute paths in source maps without security issues
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
Cargo.lockis excluded by!**/*.locktests/specs/npm/cjs_this_in_exports/cjs_this_in_exports/main.outis excluded by!**/*.outtests/specs/run/sourcemap_external/external.js.mapis excluded by!**/*.maptests/specs/run/sourcemap_external/external.js.outis excluded by!**/*.outtests/specs/run/sourcemap_inline_base64/inline_base64.js.outis excluded by!**/*.outtests/specs/run/sourcemap_none/no_sourcemap.js.outis excluded by!**/*.outtests/specs/run/sourcemap_relative_paths/outside_cwd.js.outis excluded by!**/*.outtests/specs/run/sourcemap_trailing_content/trailing_content.js.outis excluded by!**/*.out
📒 Files selected for processing (15)
Cargo.toml(1 hunks)cli/module_loader.rs(2 hunks)cli/rt/run.rs(1 hunks)tests/specs/run/sourcemap_external/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_external/external.js(1 hunks)tests/specs/run/sourcemap_external/original.ts(1 hunks)tests/specs/run/sourcemap_inline_base64/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_inline_base64/inline_base64.js(1 hunks)tests/specs/run/sourcemap_none/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_none/no_sourcemap.js(1 hunks)tests/specs/run/sourcemap_relative_paths/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_relative_paths/outside_cwd.js(1 hunks)tests/specs/run/sourcemap_relative_paths/src/original.ts(1 hunks)tests/specs/run/sourcemap_trailing_content/__test__.jsonc(1 hunks)tests/specs/run/sourcemap_trailing_content/trailing_content.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/specs/run/sourcemap_inline_base64/test.jsonc
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/specs/run/sourcemap_external/original.ts
- tests/specs/run/sourcemap_trailing_content/test.jsonc
- tests/specs/run/sourcemap_inline_base64/inline_base64.js
- tests/specs/run/sourcemap_none/test.jsonc
- tests/specs/run/sourcemap_relative_paths/outside_cwd.js
- Cargo.toml
- cli/rt/run.rs
🧰 Additional context used
🧬 Code graph analysis (1)
cli/module_loader.rs (3)
cli/rt/run.rs (2)
load_external_source_map(586-593)get_source_mapped_source_line(595-616)runtime/worker.rs (1)
get_source_mapped_source_line(1292-1303)cli/graph_container.rs (2)
graph(26-26)graph(162-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test debug linux-aarch64
- GitHub Check: test debug macos-x86_64
- GitHub Check: test debug linux-x86_64
- GitHub Check: test release linux-x86_64
- GitHub Check: test debug macos-aarch64
- GitHub Check: test debug windows-x86_64
- GitHub Check: build libs
- GitHub Check: lint debug linux-x86_64
- GitHub Check: lint debug windows-x86_64
- GitHub Check: lint debug macos-x86_64
🔇 Additional comments (9)
tests/specs/run/sourcemap_relative_paths/__test__.jsonc (1)
1-9: LGTM!The test specification is well-formed and correctly configured to test source map handling with relative paths outside the current working directory.
tests/specs/run/sourcemap_none/no_sourcemap.js (1)
1-6: LGTM!This baseline test correctly verifies that JavaScript execution works normally without a source map.
tests/specs/run/sourcemap_relative_paths/src/original.ts (1)
1-6: LGTM!This original TypeScript source file is correctly structured for source map testing.
tests/specs/run/sourcemap_trailing_content/trailing_content.js (1)
1-13: LGTM!This test correctly validates that inline source maps remain valid when followed by trailing content, addressing issue #21988.
tests/specs/run/sourcemap_external/__test__.jsonc (1)
1-5: LGTM!The test specification is correctly configured to test external source map loading functionality.
tests/specs/run/sourcemap_external/external.js (1)
1-8: LGTM!This test correctly exercises external source map loading by referencing an external
.mapfile.cli/module_loader.rs (3)
1213-1231: LGTM!The refactored
get_source_mapmethod correctly loads prepared modules and extracts inline source maps. The scheme filtering appropriately restricts lookups to external module types.
1292-1304: LGTM!The
extract_source_linehelper function is well-implemented with an efficient approach usingsplit('\n').nth()that avoids allocating all lines. The comment explaining why.lines()is not used is helpful, and the out-of-bounds handling provides a clear warning message.
1251-1289: The review comment incorrectly conflates error formatting with code loading.The original concern about permission handling in
get_source_mapped_source_line(lines 1272-1277) is based on a misunderstanding of the method's purpose and design. The method is designed to retrieve source files where cached files are returned, and for local files, they are read from disk.This method is used exclusively for error formatting and stack trace generation in diagnostic contexts (as seen in worker.rs), not for code execution or loading. The design of
get_cached_source_or_localexplicitly restricts access to cached or local files, which is appropriate for this use case. The use of&Default::default()for permissions is intentional and correct for a diagnostics context—it differs from code loading paths for good reason.The method does not require explicit permission checks at the call site because it operates in a different security model than code loading: error formatting and stack trace generation have fundamentally different security requirements than code execution.
Likely an incorrect or invalid review comment.
| fn load_external_source_map( | ||
| &self, | ||
| source_map_url: &str, | ||
| ) -> Option<Cow<'_, [u8]>> { | ||
| let specifier = resolve_url(source_map_url).ok()?; | ||
|
|
||
| if let Ok(Some(file)) = self | ||
| .0 | ||
| .shared | ||
| .file_fetcher | ||
| .get_cached_source_or_local(&specifier) | ||
| { | ||
| return Some(Cow::Owned(file.source.to_vec())); | ||
| } | ||
|
|
||
| None | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Add permission checks and documentation.
This method has two concerns:
-
Missing permission checks: As previously noted by @bartlomieju, the method accesses source map files via
get_cached_source_or_localwithout explicit permission validation. This could allow unauthorized access to source map files on the filesystem. -
Missing documentation: The method lacks doc comments explaining its purpose, parameters, and return value.
Please verify whether get_cached_source_or_local performs its own permission checks internally. If not, explicit permission validation should be added here. Run the following to check the implementation:
Additionally, consider adding doc comments:
+ /// Loads an external source map from the given URL.
+ /// Returns the source map bytes if available in cache or on the local filesystem.
+ ///
+ /// # Arguments
+ /// * `source_map_url` - The URL of the external source map to load
+ ///
+ /// # Returns
+ /// An optional `Cow<[u8]>` containing the source map bytes
fn load_external_source_map(
&self,
source_map_url: &str,
) -> Option<Cow<'_, [u8]>> {Based on past review comments.
🏁 Script executed:
#!/bin/bash
# Search for get_cached_source_or_local implementation to verify permission handling
ast-grep --pattern $'fn get_cached_source_or_local($$$) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for get_cached_source_or_local function definition using ripgrep
rg -n "get_cached_source_or_local" --type rust -A 10 -B 2Length of output: 2948
🏁 Script executed:
#!/bin/bash
# Search for permission checks or deno_web related permission handling
rg -n "permission" --type rust cli/module_loader.rs -C 3Length of output: 8306
🏁 Script executed:
#!/bin/bash
# Find where file_fetcher is defined to understand the type
rg -n "file_fetcher\s*:" --type rust -B 5 -A 5Length of output: 71976
Add permission checks and documentation to load_external_source_map.
The method bypasses permission checks when accessing source map files. Remote fetches perform permission checks via permissions.check_specifier(), but local file access through fetch_local does not. Since load_external_source_map loads user-facing debug information that should respect file access permissions, explicit permission validation is required. The method should follow the pattern used by load_asset (lines 732-753), passing FetchPermissionsOptionRef::Restricted with appropriate permissions to enforce consistent security boundaries.
Additionally, add doc comments explaining the method's purpose, parameters, and return value.
🤖 Prompt for AI Agents
In cli/module_loader.rs around lines 1233 to 1249, load_external_source_map
currently skips permission checks for local file access; update it to perform
explicit permission validation like load_asset (lines 732-753) by calling the
file fetcher with FetchPermissionsOptionRef::Restricted and the same permissions
object (e.g., self.0.shared.permissions) so both remote and local source map
loads honor permissions.check_specifier(), and return None on denied access;
also add a short doc comment above the function describing its purpose,
parameters (source_map_url) and return value (Option<Cow<'_, [u8]>>) indicating
it returns the source map bytes if allowed and found.
b3128a5 to
d95b86d
Compare
fraidev
force-pushed
the
native_sourcemaps_core
branch
from
d95b86d to
3e987b1
Compare
fraidev
changed the title
2 participants
Needs:
Related:
#4499
#21988