Skip to content

fix: Correct sourceRoot logic#76

Merged
loewenheim merged 3 commits intomasterfrom
fix/idempotent
Nov 2, 2023
Merged

fix: Correct sourceRoot logic#76
loewenheim merged 3 commits intomasterfrom
fix/idempotent

Conversation

@loewenheim
Copy link
Copy Markdown
Contributor

This changes sourcemap parsing so that source names are not automatically prefixed with the sourceRoot field. The problem with doing that is that reading and then immediately writing a sourcemap would result in the source names changing.

Instead, SourceMap now maintains a copy of the vector of source names with the sourceRoot prefix prepended. Obviously this copy needs to be kept in sync with the original source names whenever a source name or the sourceRoot changes.

Note that this PR changes the behavior of SourceMapBuilder somewhat. Now, there is no difference between parsing a sourcemap like

{ "sourceRoot": "root", "sources": ["foo.js", "bar.js"], [...]}

and constructing it with a builder:

let mut builder = SourceMapBuilder::new(None);
builder.set_source_root(Some("root"));
builder.add_source("foo.js");
builder.add_source("bar.js");
builder.into_sourcemap();

Before, the parsed version would've contained the sources "root/foo.js" and "root/bar.js", while the constructed version would've had "foo.js" and "bar.js"

@loewenheim loewenheim self-assigned this Oct 31, 2023
@loewenheim loewenheim requested a review from Swatinem October 31, 2023 13:36
@loewenheim loewenheim linked an issue Oct 31, 2023 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

@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.

can you add a test that makes sure the get_source() accessor still works when parsing a sourcemap from JSON?

I had a hard time following the code to make sure this case is actually handled correctly.

@Swatinem
Copy link
Copy Markdown
Contributor

Swatinem commented Nov 2, 2023

Or maybe such a test already exists? Which might be even better :-)

@loewenheim
Copy link
Copy Markdown
Contributor Author

Fair point. I'll have a look to see if this case is already covered.

@loewenheim loewenheim merged commit 27402a7 into master Nov 2, 2023
@loewenheim loewenheim deleted the fix/idempotent branch November 2, 2023 13:15
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.

Sourcemap parsing incorrectly prefixes sources with sourceRoot

2 participants