Fixing fixup of source locations, mostly. #2356
Conversation
NTillmann
added
Instant Render
WIP
NTillmann
removed
the
WIP
NTillmann
changed the title
src/environment.js
Outdated
| ); | ||
| return info; | ||
| } | ||
| function fixupPosition(pos: BabelNodePosition, posInfo, otherInfo) { |
There was a problem hiding this comment.
Please provide parameter types and a return type.
There was a problem hiding this comment.
Since posInfo already includes all of the information in pos, why pass it in?
There was a problem hiding this comment.
posInfo does not include a pointer to pos which is what we want to mutate.
src/environment.js
Outdated
| new_pos.column = old_pos.column; | ||
| new_loc.source = old_pos.source; | ||
| }); | ||
| function getPositionInfo(position: BabelNodePosition) { |
There was a problem hiding this comment.
Please provide a return type annotation.
src/environment.js
Outdated
| if (posInfo.rewritten) return; | ||
| if (posInfo.originalPosition.source == null) { | ||
| invariant(otherInfo.originalPosition.source != null); | ||
| pos.line = Math.max(0, pos.line + otherInfo.originalPosition.line - otherInfo.newLine); |
There was a problem hiding this comment.
I would prefer to see something like:
let delta = posInfo.newLine - otherInfo.newLine;
pos.line = Max.max(0, otherInfo.originalPosition.line + delta);
src/environment.js
Outdated
| fixupPosition(locEnd, endInfo, startInfo); | ||
|
|
||
| // NOTE: The end position seems to be sometimes wrong, probably due to a limitation of | ||
| // what's persisted in the sourcemaps. In fact, the end position maybe so wrong that |
NTillmann
force-pushed
the
FixingSourceMaps
branch
2 times, most recently
from
f8a408d
to
78a7204
Compare
There was a problem hiding this comment.
NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
NTillmann
force-pushed
the
FixingSourceMaps
branch
from
78a7204
to
554e59e
Compare
There was a problem hiding this comment.
NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release notes: Fixing source map support. I kept getting seemingly garbage source locations in error messages, and looked into that. I found various issues: - We in-place update positions, but they are actually shared, and thus we may revisit positions. When we do, we map them again, and so on... - The actual mapping doesn't pay attention to the filename. I am not fixing this, but added a TODO, and opened #2353. - Another remaining issue is that something goes wrong with end-positions: They are sometimes mapped to some seemingly random position in the right line. This also requires further investigation. If anyone knows anything about this, please let me know...
… serialization. Also, never show end positions to user.
Typing source maps. Fixing casing Fixing filename logic
NTillmann
force-pushed
the
FixingSourceMaps
branch
from
554e59e
to
5fef5b0
Compare
There was a problem hiding this comment.
NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release notes: Fixing source map support.
I kept getting seemingly garbage source locations in error messages, and looked into that.
I found various issues: