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

Replace source-map with @jridgewell/trace-mapping #685

Merged
merged 3 commits into from
Feb 26, 2024
Merged

Replace source-map with @jridgewell/trace-mapping #685

merged 3 commits into from
Feb 26, 2024

Conversation

onigoetz
Copy link
Contributor

@onigoetz onigoetz commented Jun 1, 2022

The trace-mapping module is faster than source-map in every benchmark
https://github.com/jridgewell/trace-mapping#benchmarks

It's also being adopted by many packages across the ecosystem (Jest, Babel, Terser at least have included it already)
And it doesn't need to be async like source-map 0.7.*

@@ -90,7 +90,7 @@ describe('map store', () => {
},
end: {
line: 5,
column: Infinity
column: 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not the biggest expert in source-maps in general, my understanding is that in some edge cases @jridgewell/trace-mapping does have a different heuristic to resolve mappings.

Maybe @jridgewell can provide more details when this can happen

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this comes from

// export class SimpleClass {
// hy() {
// console.log("Hy");
// }
// }
?

If so, this appears to be the AST location for the export keyword, which ends at the ASI implied ; after the closing } of the class declaration. It seems to record that location as { line: 5, column: 1 }, since the } appears at column 0 of that line.

Loading this into a visualizer, the answer becomes a little complicated. That closing } maps to 3 locations in the generated code:

Screen Shot 2022-06-01 at 9 31 05 PM

And the implied ; maps to 4 locations:

Screen Shot 2022-06-01 at 9 30 29 PM

What's likely happened is that the old sourcemap code does not have a guaranteed lowest bound return value for its binary search. If you have multiple generated locations that originated from a source location, any of those generated positions can be returned by the lookup depending on which one is hit first by the binary search. So if you had 1 extra mapping on that line, you would have gotten a different result.

With TraceMap, we always return the the left-most or right-most result when using GREATEST_LOWER_BOUND or LEAST_UPPER_BOUND (respectively). In this case, it seems we're returning a mapping that is on the same source line as the original mapping (in the originalEndPositionFor code). So we're detecting the real end bound correctly.

If you wanted more robust code, we could add a allGeneratedPositionsFor API to return all the generated mapping locations. You could then iterate the array to find an appropriate one, and guarantee that you're always finding the real end location.

jridgewell added a commit to jridgewell/istanbuljs that referenced this pull request Feb 22, 2024
See reasoning in istanbuljs#743 (comment) and istanbuljs#685 (comment).

This will enable us to switch to `TraceMap` without any test failures.
SimenB pushed a commit that referenced this pull request Feb 26, 2024
…sforms (#768)

See reasoning in #743 (comment) and #685 (comment).

This will enable us to switch to `TraceMap` without any test failures.
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

@SimenB SimenB merged commit 293f8b9 into istanbuljs:main Feb 26, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Feb 26, 2024
@SimenB
Copy link
Member

SimenB commented Feb 26, 2024

jridgewell added a commit to jridgewell/istanbuljs that referenced this pull request Feb 26, 2024
…forms

Seems istanbuljs#685 accidentally reverted the change to use `allGeneratedPositionsFor`. The tests still pass, but but this is still a more correct implementation.
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