fix: Avoid internally generating negative source maps columns #15719
Conversation
liuxingbaoyu
commented
Jun 23, 2023
•
liuxingbaoyu
commented
| Q | A |
|---|---|
| Fixed Issues? | Fixes #15712 |
| Patch: Bug Fix? | |
| Major: Breaking Change? | |
| Minor: New Feature? | |
| Tests Added + Pass? | √ |
| Documentation PR Link | |
| Any Dependency Changes? | |
| License | MIT |
liuxingbaoyu
added
the
PR: Bug Fix 🐛
| it("should not throw when loc.column === 0 with inputSourceMap", () => { | ||
| const ast = parseExpression("a(\n)"); | ||
|
|
||
| ast.loc.end.column = 0; |
There was a problem hiding this comment.
Can we construct a test without AST location mutations? If not it may point to a downstream issue and we can provide a better error message.
The current _normalizePosition implementation will also throw if any external tool sets ast.loc.end.line to -1. If we decide to mute such errors, we should also handle the .line info.
There was a problem hiding this comment.
This happened in a complex Angular project, and I thought it was a downstream bug, but since it's a regression, I took this approach.
The current _normalizePosition implementation will also throw if any external tool sets ast.loc.end.line to -1. If we decide to mute such errors, we should also handle the .line info.
You are right, but the reason for this regression is that we internally added columnOffset resulting in a possible -1, and lineOffset is not currently actually used, I'm not sure about doing the same.
Maybe we can remove lineOffset?
There was a problem hiding this comment.
Anything new here ? It would be awesome to have this fix :)
| target.line = pos.line + lineOffset; | ||
| target.column = pos.column + columnOffset; | ||
| target.line = pos.line; | ||
| target.column = Math.max(pos.column + columnOffset, 0); |
There was a problem hiding this comment.
Can you add a note here to #15712? So that we can investigate this issue in the future.
nicolo-ribaudo
changed the title
github-actions
bot
added
the
outdated
