Offload all jsx text indentation handling to indentMultilineCommentOrJsxText #1
Conversation
weswigham
force-pushed
the
small-patch-to-fix
branch
from
3d2cd43 to
b5725bb
Compare
| @@ -652,6 +649,11 @@ namespace ts.formatting { | |||
| if (tokenInfo.token.end > node.end) { | |||
| break; | |||
| } | |||
| if (node.kind === SyntaxKind.JsxText) { | |||
| // Intentation rules for jsx text are handled by `indentMultilineCommentOrJsxText` inside `processChildNode`; just fastforward past it here | |||
There was a problem hiding this comment.
Notably, the formatting scanner actually doesn't handle scanning JSX correctly, like, at all; so the only thing that even makes sense is to fastforward over it - otherwise we do silly things like interpret words within the jsxtext as identifiers.
| for (let i = startIndex; i < parts.length; i++ , startLine++) { | ||
| const startLinePos = getStartPositionOfLine(startLine, sourceFile); | ||
| const nonWhitespaceCharacterAndColumn = | ||
| i === 0 | ||
| ? nonWhitespaceColumnInFirstPart | ||
| : SmartIndenter.findFirstNonWhitespaceCharacterAndColumn(parts[i].pos, parts[i].end, sourceFile, options); | ||
|
|
||
| if (jsxTextStyleIndent) { | ||
| // skip adding indentation to blank lines |
There was a problem hiding this comment.
So for comments we actually add trailing whitespace, apparently. This seems silly, and causes some jsx text formatting baselines to fail, so we choose not to emit any whitespace on empty lines here to avoid emitting that trailing whitespace.
| @@ -343,24 +343,6 @@ namespace ts.formatting { | |||
| return false; | |||
| } | |||
|
|
|||
| export function isJSXClosingWithMultiLineText(parent: Node, child: TextRangeWithKind, _childStartLine: number, sourceFile: SourceFileLike): boolean { | |||
There was a problem hiding this comment.
Most of this logic is just now directly in processNode instead.
| @@ -501,9 +501,6 @@ namespace ts.formatting { | |||
| else if (SmartIndenter.argumentStartsOnSameLineAsPreviousArgument(parent, node, startLine, sourceFile)) { | |||
| return { indentation: parentDynamicIndentation.getIndentation(), delta }; | |||
| } | |||
| else if (SmartIndenter.isJSXClosingWithMultiLineText(parent, node, startLine, sourceFile)) { | |||
There was a problem hiding this comment.
So this is where we were trying to compensate for the old bad scanner behavior; we'd essentially dedent (or at least choose not to increase the indent) in this case to try to compensate for the extra whitespace the scanner used to provide.
| @@ -1072,7 +1087,7 @@ namespace ts.formatting { | |||
| const nonWhitespaceColumnInFirstPart = | |||
| SmartIndenter.findFirstNonWhitespaceCharacterAndColumn(startLinePos, parts[0].pos, sourceFile, options); | |||
|
|
|||
| if (indentation === nonWhitespaceColumnInFirstPart.column) { | |||
| if (indentation === nonWhitespaceColumnInFirstPart.column && !jsxTextStyleIndent) { | |||
There was a problem hiding this comment.
Do note - if we choose to remove this line because we prefer the whitespace-preserving style, then we can revert this line, too.
| const currentNode = find(siblings, arg => arg.pos === child.pos); | ||
| const currentIndex = siblings.indexOf(currentNode!); | ||
| const previousNode = siblings[currentIndex - 1]; | ||
| if (previousNode) { |
There was a problem hiding this comment.
I'm... unclear on why this can happen, but formatTsx produces a case where you have a JsxText node, and it has no previous sibling. Which should be impossible, as it should always have an associated opening tag. The input does have a syntax error, so that's probably why.
|
Thanks so much ❤️ |
| if (range.pos !== range.end) { // don't indent zero-width jsx text | ||
| const siblings = parent.getChildren(sourceFile); | ||
| const currentNode = find(siblings, arg => arg.pos === child.pos); | ||
| const currentIndex = siblings.indexOf(currentNode!); |
…etion providing auto import suggestions (microsoft#42141) * Add AutoImportSuggestions for UMD module export declarations instead of global keywords * Add test for scripts * Add more comments * Provide auto import suggestion only for modules and not scripts * PR review #1 * PR review #1
This should fix the remaining test issues your PR had - mostly, now that the scanners' un-borked, it just de-duplicates jsx text indentation handling, so it's only performed by
indentMultilineCommentOrJsxText(and adjusts it so it reliably re-indents jsx text like we'd like).Our tests bring up a good question though - as is, this will reformat:
as
which one of the new tests seemed to imply was correct, rather than the probably preferable:
I can see why it sort-of is, I guess, but if it's not, and we'd like to reconsider preserving the whitespace on the lines after the first, like we do for comments, we'd just need to remove this new line.