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 4 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
17 changes: 15 additions & 2 deletions 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 All @@ -387,7 +389,10 @@ namespace ts.formatting {

let indentation = inheritedIndentation;
if (indentation === Constants.Unknown) {
if (isSomeBlock(node.kind)) {
if (isIndentPreventedChildNode(parent.kind, node.kind)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would inline the body of isIndentPreventedChildNode,

if (parent.kind === SyntaxKind.JsxElement && child.kind === SyntaxKind.JsxClosingElement)

it is more readable this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is similar to closing braces and parens, maybe we should add it to get indentationForToken.

Copy link
Contributor

Choose a reason for hiding this comment

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

possibly move getIndentationForToken to a new helper, call it in getIndentationForToken, and from here and add support for jsx elements in it as well.. possibly give the new helper a better name as getIndentationForNode.

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 currently have no idea so I just inlined it. I agree that the closing node is similar to closing tokens but I think nodes and tokens are being processed with some differences by different functions so a merged helper may not really simplify the code.

indentation = parentDynamicIndentation.getIndentation();
}
else if (isSomeBlock(node.kind)) {
// blocks should be indented in
// - other blocks
// - source file
Expand Down Expand Up @@ -1016,6 +1021,14 @@ namespace ts.formatting {
return false;
}

function isIndentPreventedChildNode(parent: SyntaxKind, child: SyntaxKind) {
switch (parent) {
case SyntaxKind.JsxElement: {
return child === SyntaxKind.JsxClosingElement;
}
}
}

function getOpenTokenForList(node: Node, list: Node[]) {
switch (node.kind) {
case SyntaxKind.Constructor:
Expand Down
2 changes: 2 additions & 0 deletions src/services/formatting/smartIndenter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,8 @@ namespace ts.formatting {
case SyntaxKind.ArrayBindingPattern:
case SyntaxKind.ObjectBindingPattern:
case SyntaxKind.JsxElement:
case SyntaxKind.JsxOpeningElement:
case SyntaxKind.JsxExpression:
return true;
}
return false;
Expand Down
49 changes: 46 additions & 3 deletions tests/cases/fourslash/formattingJsxElements.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,57 @@
//// <div className="commentBox" >
////Hello, World!/*autoformat*/
/////*indent*/
//// </div>
//// </div>/*closingTagAutoformat*/
//// )
////}
////

////function () {
//// return <div
////className=""/*attrAutoformat*/
/////*attrIndent*/
////id={
////"abc" + "cde"/*expressionAutoformat*/
/////*expressionIndent*/
////}
//// >/*danglingBracketAutoformat*/
//// </div>
////}
////
////let h5 = <h5>
////<span>/*childJsxElementAutoformat*/
/////*childJsxElementIndent*/
////<span></span>/*grandchildJsxElementAutoformat*/
////</span>/*containedClosingTagAutoformat*/
////</h5>

format.document();
goTo.marker("autoformat");
verify.currentLineContentIs(' Hello, World!');
goTo.marker("indent");
verify.indentationIs(12);
verify.indentationIs(12);
goTo.marker("closingTagAutoformat");
verify.currentLineContentIs(" </div>");

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("childJsxElementAutoformat");
verify.currentLineContentIs(" <span>");
goTo.marker("childJsxElementIndent");
verify.indentationIs(8);
goTo.marker("grandchildJsxElementAutoformat");
verify.currentLineContentIs(" <span></span>");
goTo.marker("containedClosingTagAutoformat");
verify.currentLineContentIs(" </span>");