-
-
Notifications
You must be signed in to change notification settings - Fork 222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(inject): Make sourcemap discovery smarter #1663
Changes from all commits
99313b2
c0c4cdf
24ad886
02d0003
724473a
b838203
8f2fc76
336a37e
66a9756
625cd64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -378,6 +378,60 @@ pub fn normalize_sourcemap_url(source_url: &str, sourcemap_url: &str) -> String | |
format!("{}{}", &joined[..cutoff], clean_path(&joined[cutoff..])) | ||
} | ||
|
||
/// Returns a list of those paths among `candidate_paths` that differ from `expected_path` in | ||
/// at most one segment (modulo `.` segments). | ||
/// | ||
/// The differing segment cannot be the last one (i.e., the filename). | ||
/// | ||
/// If `expected_path` occurs among the `candidate_paths`, no other paths will be returned since | ||
/// that is considered a unique best match. | ||
/// | ||
/// The intended usecase is finding sourcemaps even if they reside in a different directory; see | ||
/// the `test_find_matching_paths_sourcemaps` test for a minimal example. | ||
pub fn find_matching_paths(candidate_paths: &[String], expected_path: &str) -> Vec<String> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe if we can change |
||
let mut matches = Vec::new(); | ||
for candidate in candidate_paths { | ||
let mut expected_segments = expected_path | ||
.split('/') | ||
.filter(|&segment| segment != ".") | ||
.peekable(); | ||
let mut candidate_segments = candidate | ||
.split('/') | ||
.filter(|&segment| segment != ".") | ||
.peekable(); | ||
|
||
// If there is a candidate that is exactly equal to the goal path, | ||
// return only that one. | ||
if Iterator::eq(candidate_segments.clone(), expected_segments.clone()) { | ||
return vec![candidate.clone()]; | ||
} | ||
|
||
// Iterate through both paths and discard segments so long as they are equal. | ||
while candidate_segments | ||
.peek() | ||
.zip(expected_segments.peek()) | ||
.map_or(false, |(x, y)| x == y) | ||
{ | ||
candidate_segments.next(); | ||
expected_segments.next(); | ||
} | ||
|
||
// The next segments (if there are any left) must be where the paths disagree. | ||
candidate_segments.next(); | ||
expected_segments.next(); | ||
|
||
// The rest of both paths must agree and be nonempty, so at least the filenames definitely | ||
// must agree. | ||
if candidate_segments.peek().is_some() | ||
&& Iterator::eq(candidate_segments, expected_segments) | ||
{ | ||
matches.push(candidate.clone()); | ||
} | ||
} | ||
|
||
matches | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use std::io::Write; | ||
|
@@ -386,7 +440,7 @@ mod tests { | |
|
||
use crate::utils::fs::TempFile; | ||
|
||
use super::{fixup_js_file, fixup_sourcemap, normalize_sourcemap_url, replace_sourcemap_url}; | ||
use super::*; | ||
|
||
#[test] | ||
fn test_fixup_sourcemap() { | ||
|
@@ -601,4 +655,69 @@ more text | |
"#; | ||
assert_eq!(std::str::from_utf8(&js_contents).unwrap(), expected); | ||
} | ||
|
||
#[test] | ||
fn test_find_matching_paths_unique() { | ||
let expected = "./foo/bar/baz/quux"; | ||
let candidates = &[ | ||
"./foo/baz/quux".to_string(), | ||
"foo/baar/baz/quux".to_string(), | ||
]; | ||
|
||
assert_eq!( | ||
find_matching_paths(candidates, expected), | ||
vec!["foo/baar/baz/quux"] | ||
); | ||
|
||
let candidates = &[ | ||
"./foo/baz/quux".to_string(), | ||
"foo/baar/baz/quux".to_string(), | ||
"./foo/bar/baz/quux".to_string(), | ||
]; | ||
|
||
assert_eq!(find_matching_paths(candidates, expected), vec![expected]); | ||
} | ||
|
||
#[test] | ||
fn test_find_matching_paths_ambiguous() { | ||
let expected = "./foo/bar/baz/quux"; | ||
let candidates = &[ | ||
"./foo/bar/baaz/quux".to_string(), | ||
"foo/baar/baz/quux".to_string(), | ||
]; | ||
|
||
assert_eq!(find_matching_paths(candidates, expected), candidates,); | ||
} | ||
|
||
#[test] | ||
fn test_find_matching_paths_filename() { | ||
let expected = "./foo/bar/baz/quux"; | ||
let candidates = &[ | ||
"./foo/bar/baz/nop".to_string(), | ||
"foo/baar/baz/quux".to_string(), | ||
]; | ||
|
||
assert_eq!( | ||
find_matching_paths(candidates, expected), | ||
["foo/baar/baz/quux".to_string()] | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_find_matching_paths_sourcemaps() { | ||
let candidates = &[ | ||
"./project/maps/index.js.map".to_string(), | ||
"./project/maps/page/index.js.map".to_string(), | ||
]; | ||
|
||
assert_eq!( | ||
find_matching_paths(candidates, "project/code/index.js.map"), | ||
&["./project/maps/index.js.map"] | ||
); | ||
|
||
assert_eq!( | ||
find_matching_paths(candidates, "project/code/page/index.js.map"), | ||
&["./project/maps/page/index.js.map"] | ||
); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
``` | ||
$ sentry-cli sourcemaps inject ./code ./maps ./maps2 --log-level warn | ||
? success | ||
> Searching ./code | ||
> Found 2 files | ||
> Searching ./maps | ||
> Found 2 files | ||
> Searching ./maps2 | ||
> Found 1 file | ||
> Analyzing 5 sources | ||
> Injecting debug ids | ||
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] Ambiguous matches for sourcemap path ./code/foo/index.js.map: | ||
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] ./maps/foo/index.js.map | ||
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] ./maps2/foo/index.js.map | ||
|
||
Source Map Debug ID Injection Report | ||
Modified: The following source files have been modified to have debug ids | ||
[..]-[..]-[..]-[..]-[..] - ./code/foo/index.js | ||
[..]-[..]-[..]-[..]-[..] - ./code/index.js | ||
Ignored: The following sourcemap files already have debug ids | ||
[..]-[..]-[..]-[..]-[..] - ./maps/index.js.map | ||
|
||
|
||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
``` | ||
$ sentry-cli sourcemaps inject ./code ./maps | ||
? success | ||
> Searching ./code | ||
> Found 2 files | ||
> Searching ./maps | ||
> Found 2 files | ||
> Analyzing 4 sources | ||
> Injecting debug ids | ||
|
||
Source Map Debug ID Injection Report | ||
Modified: The following source files have been modified to have debug ids | ||
[..]-[..]-[..]-[..]-[..] - ./code/foo/index.js | ||
[..]-[..]-[..]-[..]-[..] - ./code/index.js | ||
Modified: The following sourcemap files have been modified to have debug ids | ||
[..]-[..]-[..]-[..]-[..] - ./maps/foo/index.js.map | ||
Ignored: The following sourcemap files already have debug ids | ||
[..]-[..]-[..]-[..]-[..] - ./maps/index.js.map | ||
|
||
|
||
``` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you really need to clone the url? does it not outlive your usage here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that if we don't clone here, we hold a borrow of
self.sources
, which we later try to borrow mutably as well.