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

fix(sourcemap): fall back to low-resolution line mapping #4334

Merged
merged 6 commits into from Mar 4, 2022

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Dec 30, 2021

This PR contains:

  • bugfix
  • feature
  • refactor
  • documentation
  • other

Are tests included?

  • yes (bugfixes and features will not be merged without tests)
  • no

Breaking Changes?

  • yes (breaking changes will not be merged unless absolutely necessary)
  • no

List any relevant issue numbers:

Description

Basically, segment tracing was failing when a high-resolution sourcemap (which is column sensitive) comes after a low-resolution sourcemap (whose segments only have line information). This led to the related source file being excluded from the collapsed sourcemap, which is bad.

I'm not sure if unit testing this change is necessary. I've tested it manually in my project, and it works great!

…inside `Link.traceSegment` instead of returning null, so that a low-resolution sourcemap preceding a high-resolution sourcemap in the sourcemap chain doesn't fail.
@codecov
Copy link

@codecov codecov bot commented Dec 30, 2021

Codecov Report

Merging #4334 (4041d4c) into master (10dc326) will increase coverage by 0.01%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4334      +/-   ##
==========================================
+ Coverage   98.75%   98.76%   +0.01%     
==========================================
  Files         204      204              
  Lines        7308     7309       +1     
  Branches     2080     2077       -3     
==========================================
+ Hits         7217     7219       +2     
  Misses         33       33              
+ Partials       58       57       -1     
Impacted Files Coverage Δ
src/utils/collapseSourcemaps.ts 90.42% <90.90%> (+0.20%) ⬆️
src/utils/getOriginalLocation.ts 100.00% <100.00%> (+17.64%) ⬆️
cli/cli.ts 50.00% <0.00%> (-21.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10dc326...4041d4c. Read the comment docs.

Copy link
Member

@lukastaegert lukastaegert left a comment

Thank you very much.

I'm not sure if unit testing this change is necessary.

A test would really go a long way here, especially to document the case you are fixing. Currently I can only guess from your comments what the scenario is you are fixing, with a test we would actually know.
Also, if we ever start introducing WebAssembly parts into Rollup, source map handling would be the first candidate to be rewritten, in which case we really need this test to avoid regressions.

@aleclarson
Copy link
Contributor Author

@aleclarson aleclarson commented Feb 13, 2022

Just added a similar fix to getOriginalLocation too.

I've no time to write tests for a fix that I know works, unfortunately.
Hope you can merge this soon so I can stop using a fork :D

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Feb 13, 2022

I've no time to write tests for a fix that I know works, unfortunately.

That is very unfortunate. You could tell us at least what plugins cause the issue so that someone else can add a test, or how these source maps are generated. That is, if you want to get rid of your fork.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 3, 2022

Now that I think I understand the issue I added some tests and changed the logic: Instead of adding a special case for mappings with a single mapping per line, the logic will now handle general cases of lower resolution sourcemaps by using the closest matching column if no exact match can be found, following the principle that an imprecise source map is still more helpful than none at all.
Please have another look if that still fixes your issue.

@aleclarson
Copy link
Contributor Author

@aleclarson aleclarson commented Mar 3, 2022

I believe your changes will work for me, thanks!

@lukastaegert lukastaegert merged commit b255b52 into rollup:master Mar 4, 2022
11 of 12 checks passed
@TrySound
Copy link
Member

@TrySound TrySound commented Mar 9, 2022

@aleclarson Looks like you just made rollup sourcemaps twice bigger
https://unpkg.com/browse/rollup@2.69.0/dist/
https://unpkg.com/browse/rollup@2.69.1/dist/

@aleclarson
Copy link
Contributor Author

@aleclarson aleclarson commented Mar 9, 2022

@TrySound It was Lukas' commits, as evidenced by this fork that included eeca9a7 but not those made by Lukas.

https://unpkg.com/browse/@aleclarson/rollup@2.62.1/dist/

@TrySound
Copy link
Member

@TrySound TrySound commented Mar 9, 2022

Ok, I'm just pointing fingers 😄

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 9, 2022

Maybe they are also twice as good now?

@aleclarson
Copy link
Contributor Author

@aleclarson aleclarson commented Mar 9, 2022

Maybe they are also twice as good now?

Probably 😄

Vite doesn't publish sourcemaps to NPM anymore, perhaps Rollup should do the same?

The rationale is that people who need sourcemaps can clone the repository easily enough. And of course, it means less disk usage and (most importantly?) faster downloads.

As far as reducing the size of sourcemaps generated by Rollup, you could add a low-resolution option, which only maps lines instead of columns. I don't personally share the concern about fat sourcemaps, but it would certainly be appreciated by those who do.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 10, 2022

We are only publishing the source map for the browser build, and the reason is that this allows people to use the REPL for debugging Rollup itself. This was done on request by the people from replay who are using Rollup as some kind of example: https://app.replay.io/recording/rollupjs--fce3ed6c-091b-4c0d-95a5-33f35965986c?point=1622592768338673616292244694761494&time=1797&hasFrames=false

Nevertheless, this was already critizised by others and the plan is to move the browser build to a separate artifact with the next major version.

So the problem with the current sourcemaps is likely that most generated code fragments are now mapped to the closest source position in the same line instead of having no mappings. This will likely improve debugging in some cases, but otherwise just takes up a lot of space.

@lukastaegert
Copy link
Member

@lukastaegert lukastaegert commented Mar 10, 2022

Maybe we should go with Alec's solution after all, it will only blow up the source map if one plugin uses a low-resolution source map 😅

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.

3 participants