This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
feat(rome_js_formatter): Conditional JSX Chain layout #3150
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
MichaReiser
force-pushed
the
feat/jsx-conditional
branch
from
57b8c32 to
4d11dff
Compare
Deploying with
|
| Latest commit: |
33bab16
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://78239f50.tools-8rn.pages.dev |
| Branch Preview URL: | https://feat-jsx-conditional.tools-8rn.pages.dev |
MichaReiser
changed the title
MichaReiser
force-pushed
the
feat/jsx-conditional
branch
from
4d11dff to
c948a72
Compare
MichaReiser
commented
Sep 2, 2022
| /// Gets passed through from the root to the consequent and alternate of [JsConditionalExpression]s. | ||
| /// | ||
| /// Doesn't apply for [TsConditionalType]. | ||
| jsx_chain: ConditionalJsxChain, |
There was a problem hiding this comment.
I had to create a new FormatJsAnyConditionalRule so that it's possible to pass option values without cluttering the JsAnyConditional node. The reason for this is that it's necessary to know if a JsxTagExpression appears in any of the parent conditional expressions.
Checking this for every conditional is expensive, so we only check it at the root and then pass the information down to the consequent and alternate formatting.
| /// : c | ||
| /// ).member | ||
| /// ``` | ||
| fn should_extra_indent( |
There was a problem hiding this comment.
I only moved this method from the impl JsAnyConditional to implement it on the rule instead.
This PR implements Prettier's conditional JSX chain layout. ``` abcdefgh ? ( <Element> <Sub /> <Sub /> </Element> ) : ( <Element2> <Sub /> <Sub /> </Element2> ); ``` That parenthesizes the `consequent` and `alternate` except for `null`, `undefined`, and nested conditionals in the alternate. This PR further removes the `Default` implementation from `JsFormatOptions` because passing a `SourceType` that matches with the type of the AST is mandatory to get correct formatting results. ## Tests I verified the prettier snapshots and added our own tests to cover the exceptions of `null`, `undefined` and nested alternates that don't get parenthesized.
MichaReiser
force-pushed
the
feat/jsx-conditional
branch
from
c948a72 to
9b3ba60
Compare
✅ Deploy Preview for rometools canceled.
|
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
ematipico
approved these changes
Sep 2, 2022
Co-authored-by: Emanuele Stoppa <[email protected]>
ematipico
added a commit
that referenced
this pull request
Sep 5, 2022
Co-authored-by: Emanuele Stoppa <[email protected]>
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.
This PR implements Prettier's conditional JSX chain layout.
That parenthesizes the
consequentandalternateexcept fornull,undefined, and nested conditionals in the alternate.This PR further removes the
Defaultimplementation fromJsFormatOptionsbecause passing aSourceTypethat matches with the type of the AST is mandatory to get correct formatting results.Tests
I verified the prettier snapshots and added our own tests to cover the exceptions of
null,undefinedand nested alternates that don't get parenthesized.Average compatibility: 85.41 -> 85.53
Compatible lines: 83.70 -> 83.95
It's expected for the tests to fail until #3149 is merged