Fix jsx formatting (#42671)

* Refactored scanJsxToken when is formatting * Added bug regression test * Simplify JsxText formatting * Renamed isFormatting to allowMultilineJsxText * Updated baselines
microsoft · Mar 13, 2021 · 36f7623 · 36f7623
1 parent 1e9b21f
commit 36f7623
Show file tree

Hide file tree

Showing 8 changed files with 119 additions and 56 deletions.
diff --git a/src/compiler/scanner.ts b/src/compiler/scanner.ts
@@ -40,7 40,7 @@ namespace ts {
  scanJsxIdentifier(): SyntaxKind;
  scanJsxAttributeValue(): SyntaxKind;
  reScanJsxAttributeValue(): SyntaxKind;
- reScanJsxToken(): JsxTokenSyntaxKind;
  reScanJsxToken(allowMultilineJsxText?: boolean): JsxTokenSyntaxKind;
  reScanLessThanToken(): SyntaxKind;
  reScanQuestionToken(): SyntaxKind;
  reScanInvalidIdentifier(): SyntaxKind;
@@ -2223,9 2223,9 @@ namespace ts {
  return token = scanTemplateAndSetTokenValue(/* isTaggedTemplate */ true);
  }
 
- function reScanJsxToken(): JsxTokenSyntaxKind {
  function reScanJsxToken(allowMultilineJsxText = true): JsxTokenSyntaxKind {
  pos = tokenPos = startPos;
- return token = scanJsxToken();
  return token = scanJsxToken(allowMultilineJsxText);
  }
 
  function reScanLessThanToken(): SyntaxKind {
@@ -2242,7 2242,7 @@ namespace ts {
  return token = SyntaxKind.QuestionToken;
  }
 
- function scanJsxToken(): JsxTokenSyntaxKind {
  function scanJsxToken(allowMultilineJsxText = true): JsxTokenSyntaxKind {
  startPos = tokenPos = pos;
 
  if (pos >= end) {
@@ -2266,19 2266,11 @@ namespace ts {
 
  // First non-whitespace character on this line.
  let firstNonWhitespace = 0;
- let lastNonWhitespace = -1;
 
  // These initial values are special because the first line is:
  // firstNonWhitespace = 0 to indicate that we want leading whitespace,
 
  while (pos < end) {
-
- // We want to keep track of the last non-whitespace (but including
- // newlines character for hitting the end of the JSX Text region)
- if (!isWhiteSpaceSingleLine(char)) {
- lastNonWhitespace = pos;
- }
-
  char = text.charCodeAt(pos);
  if (char === CharacterCodes.openBrace) {
  break;
@@ -2297,8 2289,6 @@ namespace ts {
  error(Diagnostics.Unexpected_token_Did_you_mean_or_rbrace, pos, 1);
  }
 
- if (lastNonWhitespace > 0) lastNonWhitespace ;
-
  // FirstNonWhitespace is 0, then we only see whitespaces so far. If we see a linebreak, we want to ignore that whitespaces.
  // i.e (- : whitespace)
  // <div>----
@@ -2308,15 2298,19 @@ namespace ts {
  if (isLineBreak(char) && firstNonWhitespace === 0) {
  firstNonWhitespace = -1;
  }
  else if (!allowMultilineJsxText && isLineBreak(char) && firstNonWhitespace > 0) {
  // Stop JsxText on each line during formatting. This allows the formatter to
  // indent each line correctly.
  break;
  }
  else if (!isWhiteSpaceLike(char)) {
  firstNonWhitespace = pos;
  }
 
  pos ;
  }
 
- const endPosition = lastNonWhitespace === -1 ? pos : lastNonWhitespace;
- tokenValue = text.substring(startPos, endPosition);
  tokenValue = text.substring(startPos, pos);
 
  return firstNonWhitespace === -1 ? SyntaxKind.JsxTextAllWhiteSpaces : SyntaxKind.JsxText;
  }

diff --git a/src/services/formatting/formatting.ts b/src/services/formatting/formatting.ts
@@ -659,11 659,6 @@ 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
- formattingScanner.advance();
- continue;
- }
  consumeTokenAndAdvanceScanner(tokenInfo, node, nodeDynamicIndentation, node);
  }
 
@@ -756,22 751,6 @@ namespace ts.formatting {
  const childIndentation = computeIndentation(child, childStartLine, childIndentationAmount, node, parentDynamicIndentation, effectiveParentStartLine);
 
  processNode(child, childContextNode, childStartLine, undecoratedChildStartLine, childIndentation.indentation, childIndentation.delta);
- if (child.kind === SyntaxKind.JsxText) {
- const range: TextRange = { pos: child.getStart(), end: child.getEnd() };
- if (range.pos !== range.end) { // don't indent zero-width jsx text
- const siblings = parent.getChildren(sourceFile);
- const currentIndex = findIndex(siblings, arg => arg.pos === child.pos);
- const previousNode = siblings[currentIndex - 1];
- if (previousNode) {
- // The jsx text needs no indentation whatsoever if it ends on the same line the previous sibling ends on
- if (sourceFile.getLineAndCharacterOfPosition(range.end).line !== sourceFile.getLineAndCharacterOfPosition(previousNode.end).line) {
- // The first line is (already) "indented" if the text starts on the same line as the previous sibling element ends on
- const firstLineIsIndented = sourceFile.getLineAndCharacterOfPosition(range.pos).line === sourceFile.getLineAndCharacterOfPosition(previousNode.end).line;
- indentMultilineCommentOrJsxText(range, childIndentation.indentation, firstLineIsIndented, /*indentFinalLine*/ false, /*jsxStyle*/ true);
- }
- }
- }
- }
 
  childContextNode = node;
 
@@ -930,7 909,7 @@ namespace ts.formatting {
  switch (triviaItem.kind) {
  case SyntaxKind.MultiLineCommentTrivia:
  if (triviaInRange) {
- indentMultilineCommentOrJsxText(triviaItem, commentIndentation, /*firstLineIsIndented*/ !indentNextTokenOrTrivia);
  indentMultilineComment(triviaItem, commentIndentation, /*firstLineIsIndented*/ !indentNextTokenOrTrivia);
  }
  indentNextTokenOrTrivia = false;
  break;
@@ -1073,7 1052,7 @@ namespace ts.formatting {
  return indentationString !== sourceFile.text.substr(startLinePosition, indentationString.length);
  }
 
- function indentMultilineCommentOrJsxText(commentRange: TextRange, indentation: number, firstLineIsIndented: boolean, indentFinalLine = true, jsxTextStyleIndent?: boolean) {
  function indentMultilineComment(commentRange: TextRange, indentation: number, firstLineIsIndented: boolean, indentFinalLine = true) {
  // split comment in lines
  let startLine = sourceFile.getLineAndCharacterOfPosition(commentRange.pos).line;
  const endLine = sourceFile.getLineAndCharacterOfPosition(commentRange.end).line;
@@ -1111,19 1090,13 @@ namespace ts.formatting {
  }
 
  // shift all parts on the delta size
- let delta = indentation - nonWhitespaceColumnInFirstPart.column;
  const delta = indentation - nonWhitespaceColumnInFirstPart.column;
  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
- if (isLineBreak(sourceFile.text.charCodeAt(getStartPositionOfLine(startLine, sourceFile)))) continue;
- // reset delta on every line
- delta = indentation - nonWhitespaceCharacterAndColumn.column;
- }
  const newIndentation = nonWhitespaceCharacterAndColumn.column   delta;
  if (newIndentation > 0) {
  const indentationString = getIndentationString(newIndentation, options);

diff --git a/src/services/formatting/formattingScanner.ts b/src/services/formatting/formattingScanner.ts
@@ -124,13 124,7 @@ namespace ts.formatting {
  }
 
  function shouldRescanJsxText(node: Node): boolean {
- const isJSXText = isJsxText(node);
- if (isJSXText) {
- const containingElement = findAncestor(node.parent, p => isJsxElement(p));
- if (!containingElement) return false; // should never happen
- return !isParenthesizedExpression(containingElement.parent);
- }
- return false;
  return isJsxText(node);
  }
 
  function shouldRescanSlashToken(container: Node): boolean {
@@ -252,7 246,7 @@ namespace ts.formatting {
  return scanner.scanJsxIdentifier();
  case ScanAction.RescanJsxText:
  lastScanAction = ScanAction.RescanJsxText;
- return scanner.reScanJsxToken();
  return scanner.reScanJsxToken(/* allowMultilineJsxText */ false);
  case ScanAction.RescanJsxAttributeValue:
  lastScanAction = ScanAction.RescanJsxAttributeValue;
  return scanner.reScanJsxAttributeValue();

diff --git a/tests/baselines/reference/api/tsserverlibrary.d.ts b/tests/baselines/reference/api/tsserverlibrary.d.ts
@@ -3989,7 3989,7 @@ declare namespace ts {
  scanJsxIdentifier(): SyntaxKind;
  scanJsxAttributeValue(): SyntaxKind;
  reScanJsxAttributeValue(): SyntaxKind;
- reScanJsxToken(): JsxTokenSyntaxKind;
  reScanJsxToken(allowMultilineJsxText?: boolean): JsxTokenSyntaxKind;
  reScanLessThanToken(): SyntaxKind;
  reScanQuestionToken(): SyntaxKind;
  reScanInvalidIdentifier(): SyntaxKind;

diff --git a/tests/baselines/reference/api/typescript.d.ts b/tests/baselines/reference/api/typescript.d.ts
@@ -3989,7 3989,7 @@ declare namespace ts {
  scanJsxIdentifier(): SyntaxKind;
  scanJsxAttributeValue(): SyntaxKind;
  reScanJsxAttributeValue(): SyntaxKind;
- reScanJsxToken(): JsxTokenSyntaxKind;
  reScanJsxToken(allowMultilineJsxText?: boolean): JsxTokenSyntaxKind;
  reScanLessThanToken(): SyntaxKind;
  reScanQuestionToken(): SyntaxKind;
  reScanInvalidIdentifier(): SyntaxKind;

diff --git a/tests/cases/fourslash/formattingJsxTexts.ts → tests/cases/fourslash/formattingJsxTexts1.ts b/tests/cases/fourslash/formattingJsxTexts.ts → tests/cases/fourslash/formattingJsxTexts1.ts
diff --git a/tests/cases/fourslash/formattingJsxTexts2.ts b/tests/cases/fourslash/formattingJsxTexts2.ts
@@ -0,0 1,79 @@
 /// <reference path='fourslash.ts' />
 
 //@Filename: file.tsx
 //// const a = (
 //// <div>
 //// foo
 //// </div>
 //// );
 //// 
 //// const b = (
 //// <div>
 //// { foo }
 //// </div>
 //// );
 //// 
 //// const c = (
 //// <div>
 //// foo
 //// { foobar }
 //// bar
 //// </div>
 //// );
 //// 
 //// const d = 
 //// <div>
 //// foo
 //// </div>;
 //// 
 //// const e = 
 //// <div>
 //// { foo }
 //// </div>
 //// 
 //// const f = 
 //// <div>
 //// foo
 //// { foobar }
 //// bar
 //// </div>
 
 format.document();
 
 verify.currentFileContentIs(
 `const a = (
  <div>
  foo
  </div>
 );
 
 const b = (
  <div>
  {foo}
  </div>
 );
 
 const c = (
  <div>
  foo
  {foobar}
  bar
  </div>
 );
 
 const d =
  <div>
  foo
  </div>;
 
 const e =
  <div>
  {foo}
  </div>
 
 const f =
  <div>
  foo
  {foobar}
  bar
  </div>`);
diff --git a/tests/cases/fourslash/formattingJsxTexts3.ts b/tests/cases/fourslash/formattingJsxTexts3.ts
@@ -0,0 1,23 @@
 /// <reference path='fourslash.ts' />
 
 // Github issue #41925
 
 //@Filename: file.tsx
 //// function foo() {
 //// const bar = "Oh no";
 //// 
 //// return (
 //// <div>"{bar}"</div>
 //// )
 //// }
 
 format.document();
 
 verify.currentFileContentIs(
 `function foo() {
  const bar = "Oh no";
 
  return (
  <div>"{bar}"</div>
  )
 }`);