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 compressed source-maps have non-terminated segments #1106

Conversation

devversion
Copy link
Contributor

@devversion devversion commented Oct 31, 2021

Currently when terser compresses a given input file that comes
with a source-map, but the source-map does not have mappings for
all code parts in the input file. e.g. the input file has specific code-parts have been generated (e.g. when async/await ia downleveled). Such. generated code does not originate from any source-file.

In that case if mappings before the "generated code" are collected, and
the original file location for the "generated code" comes after previous
segments for the same line, Terser just ignores the fact that there is no
original location and the previous segment is not terminated. This causes
the "generated code" incorrectly to be mapped to the previous segment/
original source location.


Reverts #381, which is a revert of #342

#342 was reverted to work around a bug that was thought to be caused by source-map,
but surfaced on many projects due to Terser emitting generated-only mappings (which
is valid per source map spec and per source-map module).

It seems like Webpack or other tooling incorrectly added a source to mappings that
do not have any original source. It also looks like Webpack has changed its source
map logic over time already, so maybe this is no longer an issue? It's difficult to test
that out; but the source map emitted here is correct..

Visualized source map for the test:

image

@jridgewell
Copy link
Collaborator

@devversion Do you have a link for that source-map-visualization? You should be able to the "Link to this" link and it'll generate a share URL for you.

@devversion
Copy link
Contributor Author

devversion commented Nov 1, 2021

@jridgewell Sure, here is the link for the sourcemap from the test. Likely there are some mappings that could be simplified/improved in this test, but that is not of relevance.. Only the generated 1-length segment mappings are new with this change, allowing the say("hello") function to not accidentally map to line 1 (like it was without this change)

@fabiosantoscode
Copy link
Collaborator

I've already seen this but haven't said anything because it's going to be super hard to verify this doesn't have the same issues as before. I still do want to advance the version though.

@devversion
Copy link
Contributor Author

devversion commented Nov 13, 2021

yeah, that's very reasonable and I'm also not entirely sure on how to proceed here. It's definitely a little unfortunate that the corectness of source-maps previously couldn't be fixed due to a bug in some of the popular bundlers/source-map transformations, but at the same time, breaking the many users by triggering this bug (if it still there?!) transitively is also bad, yeah.

I think, if we go with that at some point, then it might make sense to really put that into a major where people would not happen to trigger sourcemap parsing bugs out of nowhere (as reported back in the days).

@fabiosantoscode
Copy link
Collaborator

Releasing major versions gives me the chills. People are really slow to update, and especially transient dependencies get really stuck in the past.

@fabiosantoscode
Copy link
Collaborator

Slow updates being ironically the reason why a major would be necessary/optimal in the first place.

@devversion
Copy link
Contributor Author

Agreed, worth noting that uglify-js fixed this some time ago as well: mishoo/UglifyJS@46d142c.

@devversion devversion changed the title Revert "Revert "Fix compressed source-maps have non-terminated segments"" Fix compressed source-maps have non-terminated segments Nov 14, 2021
@devversion
Copy link
Contributor Author

Hey, it seems like this has been a while. Happy to rebase, just let me know (also seeing the new source-map library used, nice! -- seems like this would also help with this change not generating a lot of unnecessary mappings)

@fabiosantoscode
Copy link
Collaborator

Hello there @devversion!

Please do rebase. I'm probably not going to release a new version soon due to pain in my fingers (I'm typing on my phone) but as soon as this is sorted I'll get back here.

@devversion devversion force-pushed the revert-381-revert-342-fix/compressed-source-maps-incorrect-segments branch from 00ad6ac to 5faa961 Compare May 20, 2022 13:37
@devversion
Copy link
Contributor Author

@fabiosantoscode @jridgewell Rebased. I also had to update one of the source-map test fixtures because they changed as part of this change. The test inputs and map seemed very broken though, so I re-created a test fixture using TS -> JS and async await downleveling. Now seems to work as expected when I visualize the input & minified maps.

lib/sourcemap.js Outdated Show resolved Hide resolved
lib/sourcemap.js Outdated Show resolved Hide resolved
@devversion devversion force-pushed the revert-381-revert-342-fix/compressed-source-maps-incorrect-segments branch from 5faa961 to e3be5c1 Compare May 20, 2022 16:47
@fabiosantoscode
Copy link
Collaborator

Awesome! Sorry it took so long, but I'm merging and releasing today

@fabiosantoscode fabiosantoscode merged commit 3483388 into terser:master May 30, 2022
@devversion
Copy link
Contributor Author

Thanks @fabiosantoscode. No problem at all! I realize this change unveiled some problems in the ecosystem (when we landed it initially), but I'm hoping this time it will work out.

@devversion devversion deleted the revert-381-revert-342-fix/compressed-source-maps-incorrect-segments branch May 30, 2022 12:52
@devversion
Copy link
Contributor Author

@fabiosantoscode Out of curiosity: Didn't you want to cut it in a major release (just confirming in case this is an oversight)

@fabiosantoscode
Copy link
Collaborator

I think it's been long enough that it's most likely not an issue. Webpack is more often updated than Terser, anyway.

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.

None yet

3 participants