overwrite segments for removed code#147
overwrite segments for removed code#147aleclarson wants to merge 1 commit intoRich-Harris:masterfrom
Conversation
Not sure how useful this is yet.
|
@guybedford @lukastaegert what do you think? I'm not sure I fully understand the implications of this change for downstream users, so I'd refer to others for review. |
guybedford
left a comment
guybedford
left a comment
There was a problem hiding this comment.
Thanks for much @aleclarson for this PR.
This looks great in principle to me, but the test needs to be updated, and it would ideally come with another test that considers if there are any pathological edge cases of this behaviour - let me know if you can think of any interesting things that may come up here.
| let originalCharIndex = chunk.start; | ||
| let first = true; | ||
|
|
||
| let len = this.rawSegments.length; |
There was a problem hiding this comment.
Can we rather call this something more explanatory, perhaps:
let checkDuplicateSegmentIndex = this.rawSegments.length - 1;| while (originalCharIndex < chunk.end) { | ||
| if (this.hires || first || sourcemapLocations[originalCharIndex]) { | ||
| this.rawSegments.push([this.generatedCodeColumn, sourceIndex, loc.line, loc.column]); | ||
| if (len !== 0) { |
There was a problem hiding this comment.
This would then become if (checkDuplpicateSegmentIndex !== -1)
| if (len !== 0) { | ||
| const segment = this.rawSegments[len - 1]; | ||
| if (segment[0] === this.generatedCodeColumn) { | ||
| len -= 1; // overwrite segments for removed code |
There was a problem hiding this comment.
These changes to len can be unnecessary if you just modify the last segment directly:
this.rawSegments[checkDuplicateSegmentIndex] = [...];| len -= 1; // overwrite segments for removed code | ||
| } | ||
| } | ||
| this.rawSegments[len++] = [this.generatedCodeColumn, sourceIndex, loc.line, loc.column]; |
There was a problem hiding this comment.
The default path should be able to then revert to a .push as before.
| } | ||
|
|
||
| if (original[originalCharIndex] === '\n') { | ||
| len = 0; |
There was a problem hiding this comment.
And this becomes checkDuplicateSegmentIndex = -1 making it clearer this is a switch on the loop.
3 participants
Removed ranges create two source map segments that refer to the same generated column. I consider this bloat if there isn't a good reason behind it. Generally, there shouldn't be segments for removed code (not including the segment for the characters after, whose source column is the removed range's end column).
What's your take on this, @mourner and/or @Rich-Harris?
Figured I should get feedback before writing any tests.