-
Notifications
You must be signed in to change notification settings - Fork 714
Fix JSX indentation in JavaScript output #1792
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.
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
Uncommented the code in printer.go that respects the EFStartOnNewLine emit flag. This flag is set by the JSX transformer when transforming JSX elements with multiple children, and causes the printer to emit each child on a new line with proper indentation. This fix brings tsgo's JSX output into alignment with TypeScript's reference implementation, reducing the number of .diff files from 4488 to 37, and properly formatting nested JSX as: React.createElement(Child, null, React.createElement(Child, null, React.createElement(Child, null))); instead of: React.createElement(Child, null, React.createElement(Child, null, React.createElement(Child, null))); Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
...ommentsOnJSXExpressionsArePreserved(jsx=react,module=commonjs,moduledetection=force).js.diff
Outdated
Show resolved
Hide resolved
The previous fix incorrectly cleared the inJsxChild flag when transforming nested JSX children. When transforming a nested child element, the defer statement would set inJsxChild back to false, causing the parent element to not receive the EFStartOnNewLine flag. Fixed by saving and restoring the previous value of inJsxChild instead of unconditionally setting it to false. This ensures that the flag state is properly maintained across nested transformations. Also fixed the logic for adding EFStartOnNewLine to children: the flag should only be added when there are multiple children AFTER filtering out null/whitespace children, not before. This matches TypeScript's behavior where a single child expression does not get a newline, but multiple children do. These fixes resolve the regression in commentsOnJSXExpressionsArePreserved tests, where a single JSX expression child was incorrectly getting a newline. Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes JSX indentation in JavaScript output to match TypeScript's behavior. The issue was that nested JSX elements were being collapsed to a single line instead of preserving proper indentation and newlines.
- Fixed JSX indentation by enabling the
EFStartOnNewLine
flag handling in the printer - Improved state management for JSX child transformations to prevent context corruption
- Updated newline logic to check children count after filtering out null children
Reviewed Changes
Copilot reviewed 91 out of 91 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
testdata/tests/cases/compiler/jsxNestedIndentation.tsx |
New test case to verify JSX nested indentation behavior |
testdata/tests/cases/compiler/jsxNestedIndentation.js |
Expected JavaScript output with proper indentation |
internal/transformers/jsxtransforms/jsx.go |
Fixed JSX child state management and newline logic for proper indentation |
internal/printer/printer.go |
Enabled EFStartOnNewLine flag handling to support multiline JSX output |
Various baseline files | Updated test baselines showing the fix applied across many JSX tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably better, since it makes sense to count the result nodes instead of the input nodes. However, in strada we did just always set the startOnNewLine flag on jsx elements which were children of other jsx elements, irrespective of sibling node count, so we're probably still different (we did, however, not set it on jsx text children - only tag-like children). In any case, we might need a followup change that swaps len(newChildren) > 1
to something like len(core.Filter(newChildren,isNotWhitespaceOnlyJsxText)) > 1
so whitespace only jsx text doesn't count as a "real" child element for the purpose of indentation - this is all different from strada, though, so it's kinda... opinion-y?
I dunno - maybe more diffs go away if we align with strada more instead and only set the flag on tag-like children irrespective of sibling count.
Yeah, this all seems like style. I tried doing: if tx.inJsxChild {
tx.EmitContext().AddEmitFlags(res, printer.EFStartOnNewLine)
} And it seems to create more differences, but maybe that's not what you mean. I did try out redoing the tag-like-children thing and only got more diffs, so I must be doing something wrong here. I'll just merge this as a slight improvement for now. But I agree, this is all sort of style opiniony. |
Er, well, need another approval to merge but oh well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking moreso replacing
// Add StartOnNewLine flag only if there are multiple actual children (after filtering)
if len(newChildren) > 1 {
for _, child := range newChildren {
tx.EmitContext().AddEmitFlags(child, printer.EFStartOnNewLine)
}
}
with
for _, child := range newChildren {
if !(ast.IsJsxElement(child) || ast.IsJsxSelfClosingElement(child) || ast.IsJsxFragment(child)) continue;
tx.EmitContext().AddEmitFlags(child, printer.EFStartOnNewLine)
}
which probably more closely matches the strada condition for indentation.
Just tried it, and that's seemingly worse than this PR too. Very weird.... |
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.