Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

feature(formatter): JSX Element #2542

Merged
merged 46 commits into from
Jun 22, 2022
Merged

feature(formatter): JSX Element #2542

merged 46 commits into from
Jun 22, 2022

Conversation

NicholasLYang
Copy link
Contributor

@NicholasLYang NicholasLYang commented May 5, 2022

Summary

Closes #2275

This PR is a first pass on prettier's implementation of JSX element formatting. Prettier's algorithm is quite tricky in a few ways; it adds parentheses to JSX tag expressions in certain situations; it uses fill printing for printing jsx children when the children contain non-trivial text; it breaks on certain niche conditions.

Test Plan

Added some tests to element.jsx

@cloudflare-pages
Copy link

cloudflare-pages bot commented May 9, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 824e199
Status: ✅  Deploy successful!
Preview URL: https://88a5acbd.tools-8rn.pages.dev
Branch Preview URL: https://feature-jsx-element.tools-8rn.pages.dev

View logs

@NicholasLYang NicholasLYang temporarily deployed to aws May 10, 2022 17:23 Inactive
@github-actions
Copy link

github-actions bot commented May 10, 2022

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 45878 45878 0
Passed 44938 44938 0
Failed 940 940 0
Panics 0 0 0
Coverage 97.95% 97.95% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 39 39 0
Passed 36 36 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.31% 92.31% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 5946 5946 0
Passed 350 350 0
Failed 5591 5591 0
Panics 5 5 0
Coverage 5.89% 5.89% 0.00%

ts/babel

Test result main count This PR count Difference
Total 588 588 0
Passed 519 519 0
Failed 69 69 0
Panics 0 0 0
Coverage 88.27% 88.27% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16257 16257 0
Passed 12391 12391 0
Failed 3866 3866 0
Panics 0 0 0
Coverage 76.22% 76.22% 0.00%

@NicholasLYang NicholasLYang temporarily deployed to aws May 10, 2022 20:14 Inactive
@github-actions
Copy link

github-actions bot commented May 10, 2022

@NicholasLYang NicholasLYang temporarily deployed to aws May 11, 2022 22:26 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 11, 2022 22:29 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 18, 2022 16:32 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 18, 2022 19:29 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 18, 2022 21:49 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 19, 2022 15:22 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 19, 2022 15:24 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 19, 2022 18:23 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 19, 2022 20:45 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws May 20, 2022 14:49 Inactive
@ematipico
Copy link
Contributor

What's the status of this PR @NicholasLYang ?
Do you need help?

@NicholasLYang
Copy link
Contributor Author

Finally done haha. Took a bit to fix all the issues. Unfortunately because I now track the separator in fill, we have to create a Fill struct that's boxed to avoid making FormatElement too big. If there's an alternative, I'd be open to ideas. We can't just create a list with the separators inside, because of how fill printing is implemented.

@NicholasLYang NicholasLYang temporarily deployed to aws June 6, 2022 23:11 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws June 7, 2022 14:28 Inactive
@NicholasLYang NicholasLYang temporarily deployed to aws June 7, 2022 19:19 Inactive
Copy link
Contributor

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice work! I would like to better understand why the change inside of the Printer is necessary before merging and have a few proposal on how the code could be simplified.

crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/builders.rs Show resolved Hide resolved
crates/rome_formatter/src/builders.rs Outdated Show resolved Hide resolved
crates/rome_formatter/src/printer/mod.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/src/jsx/auxiliary/space.rs Outdated Show resolved Hide resolved
crates/rome_js_formatter/src/jsx/tag/opening_element.rs Outdated Show resolved Hide resolved
@ematipico ematipico self-assigned this Jun 21, 2022
@ematipico ematipico temporarily deployed to aws June 21, 2022 18:26 Inactive
@ematipico
Copy link
Contributor

@MichaReiser I addressed some of you comments. If you're able to provide more info on the others, I might be able to implement them and merge this PR.

@ematipico
Copy link
Contributor

!bench_formatter

@github-actions
Copy link

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    350.5±2.67ms     7.4 MB/sec    1.00    350.4±3.55ms     7.4 MB/sec
formatter/compiler.js                    1.01    218.3±1.52ms     4.8 MB/sec    1.00    216.5±1.24ms     4.8 MB/sec
formatter/d3.min.js                      1.00    179.9±1.27ms  1491.6 KB/sec    1.01    182.6±1.79ms  1470.2 KB/sec
formatter/dojo.js                        1.00     12.3±0.08ms     5.6 MB/sec    1.00     12.3±0.02ms     5.6 MB/sec
formatter/ios.d.ts                       1.01    236.7±1.11ms     7.9 MB/sec    1.00    234.6±2.64ms     8.0 MB/sec
formatter/jquery.min.js                  1.00     47.5±0.35ms  1782.4 KB/sec    1.00     47.5±0.29ms  1783.2 KB/sec
formatter/math.js                        1.00    348.0±1.52ms  1905.3 KB/sec    1.01    350.8±5.07ms  1890.3 KB/sec
formatter/parser.ts                      1.00      7.9±0.03ms     6.1 MB/sec    1.00      7.9±0.04ms     6.1 MB/sec
formatter/pixi.min.js                    1.00    193.4±1.34ms     2.3 MB/sec    1.02    197.3±1.26ms     2.2 MB/sec
formatter/react-dom.production.min.js    1.00     58.3±0.51ms  2021.5 KB/sec    1.01     58.8±0.48ms  2003.2 KB/sec
formatter/react.production.min.js        1.01      3.0±0.02ms     2.1 MB/sec    1.00      2.9±0.04ms     2.1 MB/sec
formatter/router.ts                      1.00      6.0±0.01ms     9.9 MB/sec    1.00      5.9±0.03ms     9.9 MB/sec
formatter/tex-chtml-full.js              1.00    448.9±1.89ms     2.0 MB/sec    1.01    452.0±2.06ms     2.0 MB/sec
formatter/three.min.js                   1.00    227.4±1.35ms     2.6 MB/sec    1.01    230.8±1.51ms     2.5 MB/sec
formatter/typescript.js                  1.00   1412.0±7.43ms     6.7 MB/sec    1.00  1414.9±11.12ms     6.7 MB/sec
formatter/vue.global.prod.js             1.00     75.8±0.58ms  1626.7 KB/sec    1.01     76.2±0.87ms  1618.4 KB/sec

@ematipico ematipico temporarily deployed to aws June 22, 2022 14:41 Inactive
@ematipico ematipico temporarily deployed to aws June 22, 2022 14:44 Inactive
@ematipico ematipico temporarily deployed to aws June 22, 2022 14:53 Inactive
@ematipico ematipico merged commit 7bdd3aa into main Jun 22, 2022
@ematipico ematipico deleted the feature/jsx-element branch June 22, 2022 14:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format of JsxElement
4 participants