Fix sourcemaps failing on refresh #1755
Conversation
src/SourceMap.js
Outdated
| @@ -38,6 +39,8 @@ class SourceMap { | |||
| } | |||
|
|
|||
| async addMap(map, lineOffset = 0, columnOffset = 0) { | |||
| map = clone(map); | |||
There was a problem hiding this comment.
what kind of perf impact does this have? is there a specific situation where we need to clone, or just all the time?
There was a problem hiding this comment.
We could probably move it to before line 62, as the actual reuse of mappings happens on the lines after that as far as I know. I haven't tested moving it, but it should work in theory
There was a problem hiding this comment.
I'm not able to compare performance as I don't really have a project which is big enough to notice a difference
|
where do we mutate the mappings? |
|
Tested perf on a larger app, and it was noticeable - about 5 seconds slower build time. Changed it to manually clone each mapping, and this reduced the time to about the same again. |
|
Awesome, thanks for testing it out :) |
| }; | ||
|
|
||
| this.mappings.push(mapping); | ||
| this.mappings.push({ |
There was a problem hiding this comment.
Not familiar with this API, but does this mapping have a .name? I see it used in addConsumerMapping, but not this one.
There was a problem hiding this comment.
Ow it should, thanks for noticing
| line: mapping.generated.line + lineOffset, | ||
| column: mapping.generated.column + columnOffset | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| addConsumerMapping(mapping, lineOffset = 0, columnOffset = 0) { |
There was a problem hiding this comment.
FYI, the handling below here seems questionable. Mappings with no original location are still important because they terminate the end of a conceptual range over the generated file by saying that a generated location points at nothing. By discarding mappings with no original location, you're essentially force-extending every generated mapping to instead extend to the end of the line/next original-location mapping.
There was a problem hiding this comment.
Not sure why this would be an issue, if code is generated it shouldn't have a mapping right?
There was a problem hiding this comment.
Not quite. Sourcemaps are conceptually ranges over the generated code, but the actual mappings passed from the consumer are single points of data, from one generated line/col to an original line/col (optionally).
It's the fact that the original part is option that is critical here, and that is getting lost with the check in
if (
!mapping.source ||
!mapping.originalLine ||
(!mapping.originalColumn && mapping.originalColumn !== 0)
) {
return;
}
Imagine a generated piece of code like this:
console.log
If I want to map just the console to something in the original JS file, setting no original location on the .log part, I'd generate two mappings. One would say
{
generated: {
line: 0,
column: 0,
},
original: {
line: 0,
column: 0,
},
source: "input.js",
name: "console",
}
to say that the start of the word "console" (line 0, column 0) maps to the original file, also at line 0 column 0.
So far so good, but nothing sets an end position, so that mapping on its own would also apply to the .log part.
That's where non-source mappings come in. You'd also include a mapping as
{
generated: {
line: 0,
column: 7,
},
original: null
source: null,
name: null,
}
to say that, starting at line 0, column 7, the code maps to nothing in the original file.
By discarding that information, you've essentially made the original line 0 column 0 mapping apply to the whole of console.log.
There was a problem hiding this comment.
Ow I never saw that as an issue but I'll definitely look into it.
Now that you've explained it, I can see it could and will cause issues.
Thank you
There was a problem hiding this comment.
No problem, I'm happy to answer any other sourcemap questions if you have them. Babel has had it's fair share of questionable mapping logic over the ears too.
|
when is this supposed to be fixed? i need this and it still breaks in "parcel-bundler": "1.10.0-beta.1 |
|
Still having this issue with new typescript project and fresh install of parcel |
Sourcemaps inside assets were being overwritten due to not cloning the actual mappings array before merging and manipulating them inside the packager. Which caused the sourcemaps to get their position + bundle offset, resulting in incorrect mappings
Closes #997