This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_js_formatter): Format JSX #3144
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for rometools canceled.
|
MichaReiser
force-pushed
the
feat/jsx-formatting
branch
from
c918261
to
1b7e336
Compare
Deploying with
|
| Latest commit: |
534dff8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2f609d86.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-jsx-formatting.tools-8rn.pages.dev |
1 task
MichaReiser
commented
Sep 1, 2022
| @@ -70,8 +70,19 @@ const value = (bifornCringerMoshedPerplexSawder | |||
| - </Element2> | |||
| -); | |||
| +) | |||
| + ? <Element><Sub /><Sub /><Sub /><Sub /><Sub /><Sub /></Element> | |||
| + : <Element2><Sub /><Sub /><Sub /></Element2>; | |||
| + ? <Element> | |||
There was a problem hiding this comment.
Matching Prettier's formatting for conditional's containing JSX requires changes to the conditional formatting. I'll follow up with another PR to not further increase the size of this PR.
MichaReiser
force-pushed
the
feat/jsx-formatting
branch
from
1b7e336
to
4c1d6f7
Compare
MichaReiser
changed the base branch from
main
to
fix/skipped-token-trivia-spacing
MichaReiser
force-pushed
the
feat/jsx-formatting
branch
2 times, most recently
from
7b5a7f6
to
16f3aa2
Compare
MichaReiser
commented
Sep 1, 2022
|
|
||
| match interned.0.as_ref() { | ||
| element @ FormatElement::Text(_) | element @ FormatElement::Space => { | ||
| write!(f, [text("\""), element, text("\"")]) |
There was a problem hiding this comment.
This fixes an issue where an interned string token wasn't put in quotes which made it very confusing when reading Rome's format IR
MichaReiser
force-pushed
the
feat/jsx-formatting
branch
from
16f3aa2
to
9cee4fa
Compare
ematipico
approved these changes
Sep 2, 2022
crates/rome_js_formatter/src/jsx/attribute/expression_attribute_value.rs
Outdated
Show resolved
Hide resolved
MichaReiser
force-pushed
the
feat/jsx-formatting
branch
from
c2c35c2
to
3977bb1
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves Rome's formatting of JSX to bring it closer to Prettier's formatting. It improves the formatting of attributes, element heads, and most importantly of the children.
The formatting of the children is complicated for two reasons:
{' '}{' '}should be merged into a single whitespace. Isolating children in isolation is, therefore, not possible.The way this PR solves this challenge is by transforming a
JsxChildListinto an intermediate representation that strips any children that have no semantic meaning and have no effect on the formatting (empty line has no semantic meaning, but is something we want to preserve). It then uses this transformed representation to perform the formatting, applying slightly different formatting between the "flat" and expanded versions.This PR does not implement the special handling of JSX inside of conditional expression. This is left for another pull request.
Test Plan
Average compatibility: 84.94 -> 85.41
Compatible lines: 83.08 -> 83.70
I formatted the ant design project with Prettier and then Rome and manually went through the changes. This helped me identify a few bugs that I fixed. The main remaining differences are now caused by call arguments being formatted differently.
Verified that the example from the bug report #2830 now gets formatted the same as prettier.