Skip to content
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

Merged
merged 10 commits into from Jul 4, 2023

Conversation

loewenheim
Copy link
Contributor

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:

code/
  index.js -> index.js.map
  child/
    foo.js -> foo.js.map
    index.js -> index.js.map
sourcemaps/
  index.js.map
  foo.js.map
  child/
    index.js.map

With this change, each index.js.map reference will be matched to its counterpart in the sourcemaps directory, but the foo.js.map reference would not be resolved because the file is not in the right place. If there is any ambiguity, as in

code/
  index.js -> index.js.map
sourcemaps/
  index.js.map
sourcemaps2/
  index.js.map

a warning is printed and the sourcemap is treated as not found.

I believe this implementation hits a nice sweet spot between convenience and generality.

@loewenheim loewenheim self-assigned this Jun 29, 2023
@loewenheim loewenheim linked an issue Jun 29, 2023 that may be closed by this pull request
Copy link
Member

@Swatinem Swatinem left a 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()))
Copy link
Member

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?

Copy link
Contributor Author

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.

src/utils/sourcemaps.rs Outdated Show resolved Hide resolved
src/utils/sourcemaps/inject.rs Outdated Show resolved Hide resolved
src/utils/sourcemaps/inject.rs Outdated Show resolved Hide resolved
src/utils/sourcemaps/inject.rs Outdated Show resolved Hide resolved
///
/// 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> {
Copy link
Member

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.

src/utils/sourcemaps/inject.rs Outdated Show resolved Hide resolved
loewenheim and others added 4 commits July 3, 2023 12:27
Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>
Co-authored-by: Arpad Borsos <arpad.borsos@sentry.io>
@loewenheim
Copy link
Contributor Author

thats interesting, but it needs a better explanation what the assumptions around this are.

Fair point. Here is a more detailed description of how this works:

  1. A minified source file's sourcemap URL is interpreted relative to the file's directory. For example, if code/page/foo.js refers to foo.js.map, we interpret that as code/page/foo.js.map, and if it refers to ../foo.js.map, we interpret it as code/foo.js.map. This is the sourcemap's expected path.
  2. If we have a sourcemap file under the expected path, we're done.
  3. Otherwise, we iterate over all sourcemaps we know of. We consider a sourcemap's path a match for the intended path if the two paths have the same number of segments and differ in exactly one segment (before the file name—this is an edge case that I actually forgot to handle). For example, maps/page/foo.js.map would be a match for code/page/foo.js.map, as would code/page2/foo.js.map, but code/foo.js.map or code/page/bar.js.map would not.
  4. If there is exactly one match, we take it. If there are none, we clearly don't have the sourcemap. If there are multiple, we make no further attempt to choose the "right" one and just bail.

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.

@kamilogorek
Copy link
Contributor

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 ¯_(ツ)_/¯

@loewenheim loewenheim merged commit e8f9c16 into master Jul 4, 2023
16 checks passed
@loewenheim loewenheim deleted the feat/sourcemap-discovery branch July 4, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

inject: Make sourcemap discovery more flexible
3 participants