-
-
Notifications
You must be signed in to change notification settings - Fork 231
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: replace source-map
with @jridgewell/trace-mapping
#743
Conversation
filePath | ||
); | ||
this.sourceStore.set(sourceFilePath, content); | ||
if (s) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types says s
is string | null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. If sources content is enabled, some tools will use null
if the file's source content isn't available. So some files in the array will have source content (a string), and some won't (null).
@@ -90,7 +90,7 @@ describe('map store', () => { | |||
}, | |||
end: { | |||
line: 5, | |||
column: Infinity | |||
column: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if good or bad?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the source file, output file, and source map? I don't return an Infinity
value in trace-mapping
, and I don't know what computes end
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really know this code base, so the best answer I have for your q is "this test" 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gotten admin access to this repo, so I'm able to merge this now. I'd like to verify this one, tho. So if you're able to take a look, I'd be grateful!
What is the source file, output file, and source map?
I believe there are present in a comment right below:
istanbuljs/packages/istanbul-lib-source-maps/test/map-store.test.js
Lines 136 to 302 in 5584b50
// Original source | |
// export class SimpleClass { | |
// hy() { | |
// console.log("Hy"); | |
// } | |
// } | |
// Transpiled Source | |
// "use strict"; | |
// var SimpleClass = (function () { | |
// function SimpleClass() { | |
// } | |
// SimpleClass.prototype.hy = function () { | |
// console.log("Hy"); | |
// }; | |
// return SimpleClass; | |
// }()); | |
// exports.SimpleClass = SimpleClass; | |
const coverageData = { | |
'test.js': { | |
path: 'test.js', | |
statementMap: { | |
'1': { | |
start: { | |
line: 2, | |
column: 19 | |
}, | |
end: { | |
line: 9, | |
column: 3 | |
} | |
}, | |
'2': { | |
start: { | |
line: 5, | |
column: 4 | |
}, | |
end: { | |
line: 7, | |
column: 6 | |
} | |
}, | |
'3': { | |
start: { | |
line: 6, | |
column: 8 | |
}, | |
end: { | |
line: 6, | |
column: 26 | |
} | |
}, | |
'4': { | |
start: { | |
line: 8, | |
column: 4 | |
}, | |
end: { | |
line: 8, | |
column: 23 | |
} | |
}, | |
'5': { | |
start: { | |
line: 10, | |
column: 0 | |
}, | |
end: { | |
line: 10, | |
column: 34 | |
} | |
} | |
}, | |
fnMap: { | |
'1': { | |
name: '(anonymous_1)', | |
decl: { | |
start: { | |
line: 2, | |
column: 19 | |
}, | |
end: { | |
line: 2, | |
column: 20 | |
} | |
}, | |
loc: { | |
start: { | |
line: 2, | |
column: 31 | |
}, | |
end: { | |
line: 9, | |
column: 1 | |
} | |
} | |
}, | |
'2': { | |
name: 'SimpleClass', | |
decl: { | |
start: { | |
line: 3, | |
column: 13 | |
}, | |
end: { | |
line: 3, | |
column: 24 | |
} | |
}, | |
loc: { | |
start: { | |
line: 3, | |
column: 27 | |
}, | |
end: { | |
line: 4, | |
column: 5 | |
} | |
} | |
}, | |
'3': { | |
name: '(anonymous_3)', | |
decl: { | |
start: { | |
line: 5, | |
column: 31 | |
}, | |
end: { | |
line: 5, | |
column: 32 | |
} | |
}, | |
loc: { | |
start: { | |
line: 5, | |
column: 43 | |
}, | |
end: { | |
line: 7, | |
column: 5 | |
} | |
} | |
} | |
}, | |
branchMap: {}, | |
s: { | |
'1': 1, | |
'2': 1, | |
'3': 1, | |
'4': 1, | |
'5': 1 | |
}, | |
f: { | |
'1': 1, | |
'2': 1, | |
'3': 1 | |
}, | |
b: {}, | |
inputSourceMap: { | |
version: 3, | |
file: 'test.js', | |
sourceRoot: '', | |
sources: ['test.ts'], | |
names: [], | |
mappings: | |
';AAAA;IAAA;IAIA,CAAC;IAHG,wBAAE,GAAF;QACI,OAAO,CAAC,GAAG,CAAC,IAAI,CAAC,CAAC;IACtB,CAAC;IACL,kBAAC;AAAD,CAAC,AAJD,IAIC;AAJY,mBAAW,cAIvB,CAAA' | |
} | |
} |
Input lines 136-141, output 142-152 and inputSourceMap
is lines 293-301
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The origin of Infinity
I'm guessing is from:
istanbuljs/packages/istanbul-lib-source-maps/lib/get-mapping.js
Lines 57 to 72 in 5584b50
if ( | |
// If this is null, it means that we've hit the end of the file, | |
// so we can use Infinity as the end column. | |
afterEndMapping.line === null || | |
// If these don't match, it means that the call to | |
// 'generatedPositionFor' didn't find any other original mappings on | |
// the line we gave, so consider the binding to extend to infinity. | |
sourceMap.originalPositionFor(afterEndMapping).line !== | |
beforeEndMapping.line | |
) { | |
return { | |
source: beforeEndMapping.source, | |
line: beforeEndMapping.line, | |
column: Infinity | |
}; | |
} |
(with line 64 being originalPositionFor(sourceMap, afterEndMapping).line
on this branch)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jridgewell sorry to ping you again, but does the added context above help in evaluating if the change is correct (or at least not regressive) or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, I missed this during Thanksgiving week.
Based on this, the visualization we're tracing is this.
The column that's changed is the end location for the anonymous IIFE. The mappings on line 9 are (using 1-based line and 0-based column as source-map
does):
{ genColumn: 0, sourcesIndex: 0, sourceLine: 5, sourceColumn: 0 }
{ genColumn: 1, sourcesIndex: 0, sourceLine: 5, sourceColumn: 1 }
{ genColumn: 1, sourcesIndex: 0, sourceLine: 1, sourceColumn: 0 }
{ genColumn: 5, sourcesIndex: 0, sourceLine: 5, sourceColumn: 1 }
Notice that genColumn
1 has 2 mappings. In trace-mapping
when doing the originalPositionFor search with { line: 9, column: 1 }
, the first specified is returned (due to GREATEST_LOWER_BOUND
). So beforeEndMapping points to { line: 5, column: 1 }
in the original code. We then run a generatedPositionFor search, returning { line: 10, column: 34 }
. Note that the last seen generated position that points to that source location is used (because of the LEAST_UPPER_BOUND
bias), and there are many in this example.
However source-map
uses a first-match style binary search. So our originalPostitionFor
returns any match for the generated position { line: 9, column: 1 }
. In this case, it hits { genColumn: 1, sourcesIndex: 0, sourceLine: 1, sourceColumn: 0 }
first. Doing the generatedPositionFor
with that position gives { line: 2, column: 0 }
.
I dislike the results that source-map
gives when there are multiple matches. If there were a different number of mappings on that line, then a different original position could be hit by the binary search.
I know we've discussed originalEndPositionFor
in other PRs, but I can't find it now. The way that you're doing this kinda buggy. You're returning a single generated position out of the mapping, and that generated position could be sourced any line (hence the line check). In a case like this, you could incorrectly return an Infinity
end when there could be more code on that source line that would actually end that range. What you really want is to get the list of every generated position corresponding to that source location, and see if any of them are on the correct line. Ie, call allGeneratedPositionsFor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh hey, it was #685. Looks like my opinion hasn't changed since I wrote that, we should be using allGeneratedPositionsFor
. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#768 shows that this same result happens when using allGeneratedPositionsFor
to test all positions matching our end location.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I completely missed #685! I'll land that then (after your new PR). Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested these changes on Vitest's internal coverage tests (V8 and Istanbul) by patching the package. All tests pass like usual and coverage reports are identical.
Anything missing from this PR? Happy to see source-map
replaced with lighter package.
Missing a review pretty much - I cannot self review 😅 All tests pass in Jest as well with this patch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB on vacation and am starting to look through a few old PRs.
This update looks innocent enough to me, but I'm worried about the change from Infinity to 1 too ... and admittedly don't know this part of the codebase too well 😅
I think this might be able to left open for a bit until someone has the time to dig in deeply.
See reasoning in istanbuljs#743 (comment) and istanbuljs#685 (comment). This will enable us to switch to `TraceMap` without any test failures.
…sforms (#768) See reasoning in #743 (comment) and #685 (comment). This will enable us to switch to `TraceMap` without any test failures.
Duplicate of #685 |
Following up istanbuljs/v8-to-istanbul#186
I don't think this is visible to consumers?
/cc @jridgewell FYI 🙂