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

[aleclarson]

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:

• might fix sourcemaps passed from non-js assets seem to be ignored  #3938

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!

[codecov]

Codecov Report

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

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

[lukastaegert]

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]

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]

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]

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]

I believe your changes will work for me, thanks!

[TrySound]

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

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

Ok, I'm just pointing fingers 😄

[lukastaegert]

Maybe they are also twice as good now?

[aleclarson]

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]

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]

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 😅