Skip to content

Fixing JSX/TSX closing tag/attribute/expression formatting #4398

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Dec 30, 2015
Merged

Fixing JSX/TSX closing tag/attribute/expression formatting #4398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
67878c9
adding tests for JSX attributes/closing bracket
saschanaz Aug 19, 2015
b8cbfcb
Merge https://github.com/Microsoft/TypeScript into formatTsxAttr
saschanaz Aug 19, 2015
2677b3f
fixing autoformat not working with empty JsxText
saschanaz Aug 21, 2015
eae30fd
adding JSX closing tag/attribute/expression test
saschanaz Aug 21, 2015
060828c
partially move getIndentationToken into new function
saschanaz Aug 22, 2015
56fc7ec
revert 060828c
saschanaz Aug 22, 2015
92e3b3b
remove isIndentPreventedChildNode
saschanaz Aug 25, 2015
c49c68d
Manual merge
saschanaz Sep 4, 2015
74ef1b2
Merge https://github.com/Microsoft/TypeScript into formatTsxAttr
saschanaz Sep 4, 2015
8bf79a2
Resolving conflicts with #4646
saschanaz Sep 9, 2015
e387f69
merge conflicts
saschanaz Dec 9, 2015
593503f
move blocker to nodeWillIndentChild
saschanaz Dec 9, 2015
9e3ee5d
remove whitespace
saschanaz Dec 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/services/formatting/formatting.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,9 @@ namespace ts.formatting {
range: TextRange,
inheritedIndentation: number): number {

if (rangeOverlapsWithStartEnd(range, startPos, endPos)) {
if (rangeOverlapsWithStartEnd(range, startPos, endPos) ||
rangeContainsStartEnd(range, startPos, endPos) /* Not to miss zero-range nodes e.g. JsxText */) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* Not to miss zero-range nodes e.g. JsxText */

text can not be zero-length, is there an example that would trigger this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhegazy I've found that this code blocks autoformatting:

let x = <div>
<span>
</span>
</div>

<span> won't be indented here because by some reason a zero-length JsxText node right before JsxElement node incorrectly sets indentation value to zero. An additional rangeContainsStartEnd check prevents this bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a parser bug then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe related to #4332?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#4596 didn't fix this zero-length problem. CC: @vladima


if (inheritedIndentation !== Constants.Unknown) {
return inheritedIndentation;
}
Expand Down
6 changes: 5 additions & 1 deletion src/services/formatting/smartIndenter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,8 +450,9 @@ namespace ts.formatting {
case SyntaxKind.ConditionalExpression:
case SyntaxKind.ArrayBindingPattern:
case SyntaxKind.ObjectBindingPattern:
case SyntaxKind.JsxElement:
case SyntaxKind.JsxOpeningElement:
case SyntaxKind.JsxSelfClosingElement:
case SyntaxKind.JsxExpression:
case SyntaxKind.MethodSignature:
case SyntaxKind.CallSignature:
case SyntaxKind.ConstructSignature:
Expand All @@ -467,6 +468,7 @@ namespace ts.formatting {
return false;
}

/* @internal */
export function nodeWillIndentChild(parent: TextRangeWithKind, child: TextRangeWithKind, indentByDefault: boolean) {
let childKind = child ? child.kind : SyntaxKind.Unknown;
switch (parent.kind) {
Expand All @@ -484,6 +486,8 @@ namespace ts.formatting {
case SyntaxKind.GetAccessor:
case SyntaxKind.SetAccessor:
return childKind !== SyntaxKind.Block;
case SyntaxKind.JsxElement:
return childKind !== SyntaxKind.JsxClosingElement;
}
// No explicit rule for given nodes so the result will follow the default value argument
return indentByDefault;
Expand Down
47 changes: 45 additions & 2 deletions tests/cases/fourslash/formattingJsxElements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
//// </div>
//// )
////}
////
////
////function foo1() {
//// return (
//// <div className="commentBox" data-id="test">
Expand Down Expand Up @@ -45,8 +45,26 @@
//// class3= {/*5*/
//// }/>/*6*/
//// )
////}
////
////(function () {
//// return <div
////className=""/*attrAutoformat*/
/////*attrIndent*/
////id={
////"abc" + "cde"/*expressionAutoformat*/
/////*expressionIndent*/
////}

//// >/*danglingBracketAutoformat*/
//// </div>/*closingTagAutoformat*/
////})
////
////let h5 = <h5>
////<span>/*childJsxElementAutoformat*/
/////*childJsxElementIndent*/
////<span></span>/*grandchildJsxElementAutoformat*/
////</span>/*containedClosingTagAutoformat*/
////</h5>

format.document();
goTo.marker("autoformat");
Expand Down Expand Up @@ -83,3 +101,28 @@ goTo.marker("5");
verify.currentLineContentIs(' class3= {');
goTo.marker("6");
verify.currentLineContentIs(' }/>');


goTo.marker("attrAutoformat");
verify.currentLineContentIs(' className=""');
goTo.marker("attrIndent");
verify.indentationIs(8);
goTo.marker("expressionAutoformat");
verify.currentLineContentIs(' "abc" + "cde"');
goTo.marker("expressionIndent");
verify.indentationIs(12);

goTo.marker("danglingBracketAutoformat")
// TODO: verify.currentLineContentIs(" >");
verify.currentLineContentIs(" >");
goTo.marker("closingTagAutoformat");
verify.currentLineContentIs(" </div>");

goTo.marker("childJsxElementAutoformat");
verify.currentLineContentIs(" <span>");
goTo.marker("childJsxElementIndent");
verify.indentationIs(8);
goTo.marker("grandchildJsxElementAutoformat");
verify.currentLineContentIs(" <span></span>");
goTo.marker("containedClosingTagAutoformat");
verify.currentLineContentIs(" </span>");