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

Reimplement input sourcemap merging using range matching instead of closest-position matching #7761

Merged
merged 4 commits into from Apr 25, 2018

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues? Fixes #7632
Patch: Bug Fix? Y
Major: Breaking Change? Not unless things were relying on our previous awful maps
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

This PR ended up bigger than I'd originally expected, so sorry about that. The core of it comes down to the fact that just calling generatedPositionFor is an extremely bad/inaccurate way to merge two sourcemaps.

The core of this PR is making full use of the fact that source mappings are generally processed as ranges rather than individual locations. The new code walks from the original input mappings down to the actual mappings generated by Babel, but does so taking ranges into account. Rather than blindly calling generatedPositionFor, we explicitly search for the set of ranges that overlap.

This fix is driven by my work on FF in firefox-devtools/debugger#5561

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 19, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7736/

@hzoo hzoo added area: sourcemaps PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 21, 2018
source: source.path,
original: {
line: original.line,
column: original.column,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use original.column + item.original.column - generated.column here? We'll have higher precision sourcemap if so.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow that logic. If the input sourcemap had a range over a set of locations, isn't it reasonable to expect exactly the same range over the original files in the output map?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that, in vue-loader's case column info are missing with original.column.

original.column + item.original.column - generated.column is based on an assumption that the mapped range will be exactly same as the original one. There may be exceptions, so I'm not sure if that will lead to any problems.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is based on an assumption that the mapped range will be exactly same as the original one.

That is my primary concern. I don't think that's a safe assumption to make for merge logic that should ideally be fairly generic. Any kind of math trying to calculate column numbers would be making assumptions about the structure of the code being mapped. I don't think I'm personally comfortable with doing that.

@loganfsmyth
Copy link
Member Author

Fixed a few small issues with this. I'm probably going to merge it after the tests pass, since it seems like realistically no-one has the background to review it...

@loganfsmyth loganfsmyth merged commit 138d609 into babel:master Apr 25, 2018
@loganfsmyth loganfsmyth deleted the bad-map-merge branch April 25, 2018 19:29
loganfsmyth added a commit to loganfsmyth/babel that referenced this pull request Apr 25, 2018
@hzoo hzoo mentioned this pull request May 8, 2018
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: sourcemaps outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merging inputSourceMaps may result in off-by-one lines
4 participants