-
-
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
Conversation
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.
thats interesting, but it needs a better explanation what the assumptions around this are.
let mut sourcemaps = self | ||
.sources | ||
.values() | ||
.filter_map(|s| (s.ty == SourceFileType::SourceMap).then_some(s.url.clone())) |
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.
/// | ||
/// The intedend 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 comment
The reason will be displayed to describe this comment to others. Learn more.
I believe if we can change candidate_paths
to a &[&str]
, we also don’t need to clone for the output.
Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>
Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>
Fair point. Here is a more detailed description of how this works:
I think this covers the use case of code and sourcemaps residing in two separate, but "parallel" directory hierarchies with about as little complexity as possible. |
I wonder what's the chance for us to find an incorrect file. Either by a sheer chance that it's a mismatch or eg. sourcemap is out of date during deployment. But I think the same can happen now if they live in the place that pragma points to anyway ¯_(ツ)_/¯ |
This generalizes the discovery of sourcemaps when injecting, which should make the command easier to use. In particular, you can now have source and sourcemap files in different directories, but the directory hierarchies need to mirror each other. For example, consider:
With this change, each
index.js.map
reference will be matched to its counterpart in thesourcemaps
directory, but thefoo.js.map
reference would not be resolved because the file is not in the right place. If there is any ambiguity, as ina warning is printed and the sourcemap is treated as not found.
I believe this implementation hits a nice sweet spot between convenience and generality.