Keep translated cursor inside formatted node #1983
Conversation
Since the `cursorOffset` option (introduced in prettier#1637) works by tracking the cursor position relative to an AST node (rather than a CST token), it can produce incorrect results. See prettier#1981
This partially addresses prettier#1981 by ensuring that the cursor cannot jump outside of its node upon formatting. It could be improved upon by keeping track of the offset relative to the end of the node as well.
| } | ||
|
|
||
| if (out.length) { | ||
| if (out.length && out[out.length - 1].replace) { |
There was a problem hiding this comment.
This is a quick/dirty way of getting the tests to pass, and should probably be replaced with more robust logic that skips over cursor placeholder Symbols.
| // Trim whitespace at the end of line | ||
| while ( | ||
| out.length > 0 && | ||
| out[out.length - 1].match && |
There was a problem hiding this comment.
This is a quick/dirty way of getting the tests to pass, and should probably be replaced with more robust logic that skips over cursor placeholder Symbols.
Partially addresses prettier#1981
| cursorOffset: cursorOffsetResult + cursorOffset | ||
| }; | ||
| } else { | ||
| return { |
There was a problem hiding this comment.
You just need this return right? Math.max should remove the need for the condition right?
There was a problem hiding this comment.
Well, maybe. Before this PR, the cursor translation simply applied the offset from the beginning of the node the cursor was on. Now, it tries to do the same thing, unless that would move the cursor outside the node, in which case it applies the offset from the end of the node the cursor was on.
However, if the cursor doesn't escape the node after the beginning-offset is applied, it could be at a different position than if the end-offset is applied, so this logic gets a little complicated. Does that make sense?
Side note: at first, I thought you were talking about simply returning, rather than putting it inside an else block, because the if already has a return. I'll open a separate PR for enforcing that lint rule.
| cursorOffsetResult | ||
| ) | ||
| }; | ||
| } |
There was a problem hiding this comment.
I don't understand this logic, shouldn't it be
Math.min(cursorNodeEnd, cursorOffsetResult + cursorOffset)
| expect(prettier.formatWithCursor(code, { cursorOffset: 15 })).toEqual({ | ||
| formatted: "return 15;\n", | ||
| cursorOffset: 8 | ||
| }); |
There was a problem hiding this comment.
Would be nice to write a small wrapper where you can add {{cursor}} inside of the string. It would greatly help review those changes.
|
This is an interesting edge case. Just thinking out loud, it looks like the issue here isn't that the offset is > nodeEnd but that it's > identifierStart. There are two nodes here that are of importance:
and we mess up with the invariants of the second one. I'm not sure what the solution is, but it seems like taking into account the identifier is need to get a correct result. What do you think? |
|
What's the status of this PR? |
|
I've been busy with other things, and haven't done any further work on this. |
|
I'm going to close this in order for doing a cleaning, feel free to reopen if you make progress. |
|
No problem, I've dropped this for now, since some editors (whose plugins could use this feature) already do a pretty good job of preserving cursor location (I've only tried VScode) : prettier/prettier-vscode#12 (comment) |
lock
bot
added
the
locked-due-to-inactivity
2 participants
This partially addresses
#1981 by ensuring that the
cursor cannot jump outside of its node upon formatting. It could be
improved upon by keeping track of the offset relative to the end of the
node as well.