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

feat(rome_js_formatter): Member chain formatting #3283

Merged
merged 5 commits into from
Sep 29, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Sep 27, 2022

Summary

This PR refactors the definition of when a best_fitting element fits. I plan to merge #3273 into this branch before merging to main because chaining the fits definition requires changes to the member chain which are already part of #3273 (reason why some tests are currently failing on this branch).

The existing definition of when a best fitting element fits is the same as for groups:

  • It does not contain any content that forces a line break: expand_parent, any non-soft line break, ...
  • Printing all the content does not exceed the configured print width

This PR changes the definition for best fitting to:

  • Printing all its content up to the first hard line break does not exceed the configured print width

This is important to support use cases like:

expect().some.long.member.chain({
  object: withProperties,
  a: 4,
})

Where the whole member chain is a best-fitting element and it should take the first variant regardless of that the {...} object expression is printed over multiple lines.

The main challenge of this change comes from that groups inside of a best fitting variant that contains an element that forces them to break must be measured in expanded mode. This complicates things because the fits function would need to "unwind" to the outermost group (best fitting variants form a boundary) if it finds any content that causes it to expand and then continue to measure in expanded mode. The problem with "unwinding" is that it would require restoring the fits queue and fits stack to the point right before measuring the group.

I haven't found a way to do so with reasonable runtime which is why I opted for the same approach as prettier by pre-processing the IR before printing (or, in our case, after formatting). The pre-processing traverses through the whole document and sets expanded to propagated on Group if it contains any content that forces it to expand but only if there's no best-fitting element in between. The downside of this approach is that it does not respect "if_group_breaks" nor "if_group_fits" (could potentially be added) but it seems to be working reasonably well. It also comes with the advantage that less back-tracking is required in the printer when measuring if a group fits because it only has to measure groups that contain no hard line break.

Test Plan

See #3273 that fixes many prettier compatibility issues. I also added a new printer test.

@MichaReiser MichaReiser temporarily deployed to netlify-playground September 27, 2022 12:06 Inactive
@netlify
Copy link

netlify bot commented Sep 27, 2022

Deploy Preview for rometools ready!

Name Link
🔨 Latest commit f604c39
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/6334649ac1193c000813b203
😎 Deploy Preview https://deploy-preview-3283--rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Sep 27, 2022

@MichaReiser MichaReiser added this to the 0.10.0 milestone Sep 27, 2022
@MichaReiser MichaReiser added the A-Formatter Area: formatter label Sep 27, 2022
@MichaReiser MichaReiser temporarily deployed to netlify-playground September 27, 2022 12:33 Inactive
@MichaReiser MichaReiser marked this pull request as ready for review September 27, 2022 12:48
@MichaReiser MichaReiser temporarily deployed to netlify-playground September 27, 2022 12:52 Inactive
@MichaReiser
Copy link
Contributor Author

!bench_formatter

@calibre-analytics
Copy link

calibre-analytics bot commented Sep 27, 2022

Comparing refactor(rome_formatter): Best fitting fits definition Snapshot #4 to median since last deploy of rome.tools.

LCP? CLS? TBT?
Overall
Median across all pages and test profiles
570ms
from 554ms
0.049
no change
31ms
from 30ms
Chrome Desktop
Chrome Desktop • Cable
657ms
from 592ms
0.049
from 0.006
212ms
from 238ms
iPhone, 4G LTE
iPhone 12 • 4G LTE
303ms
from 222ms
0.077
no change
15ms
from 8ms
Motorola Moto G Power, 3G connection
Motorola Moto G Power • Regular 3G
570ms
from 554ms
0.026
no change
31ms
from 30ms

1 page tested

 Home

Browser previews

Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection
Chrome Desktop iPhone, 4G LTE Motorola Moto G Power, 3G connection

Most significant changes

Value Budget
Total JavaScript Size in Bytes
Chrome Desktop
1.24 MB
from 86.8 KB
Total JavaScript Size in Bytes
iPhone, 4G LTE
1.24 MB
from 86.8 KB
Total JavaScript Size in Bytes
Motorola Moto G Power, 3G connection
1.24 MB
from 86.8 KB
Cumulative Layout Shift
Chrome Desktop
0.049
from 0.006
Number of Requests
Chrome Desktop
39
from 5

5 other significant changes: Number of Requests on iPhone, 4G LTE, Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection

Calibre: Site dashboard | View this PR | Edit settings | View documentation

@github-actions
Copy link

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    409.5±5.54ms     6.3 MB/sec    1.00    410.9±7.72ms     6.3 MB/sec
formatter/compiler.js                    1.00    220.8±3.09ms     4.7 MB/sec    1.02    224.5±2.33ms     4.7 MB/sec
formatter/d3.min.js                      1.00    175.9±1.48ms  1526.2 KB/sec    1.00    176.1±1.66ms  1524.1 KB/sec
formatter/dojo.js                        1.01     11.1±0.32ms     6.2 MB/sec    1.00     11.0±0.16ms     6.2 MB/sec
formatter/ios.d.ts                       1.00    253.7±1.67ms     7.4 MB/sec    1.02    258.6±3.74ms     7.2 MB/sec
formatter/jquery.min.js                  1.00     46.5±0.42ms  1820.5 KB/sec    1.00     46.7±0.61ms  1812.7 KB/sec
formatter/math.js                        1.00    341.5±2.89ms  1941.9 KB/sec    1.00    341.0±3.65ms  1944.3 KB/sec
formatter/parser.ts                      1.00      7.3±0.05ms     6.6 MB/sec    1.02      7.5±0.04ms     6.5 MB/sec
formatter/pixi.min.js                    1.00    189.6±2.84ms     2.3 MB/sec    1.01    190.9±1.94ms     2.3 MB/sec
formatter/react-dom.production.min.js    1.00     57.3±1.11ms     2.0 MB/sec    1.01     57.8±0.67ms  2039.7 KB/sec
formatter/react.production.min.js        1.00      2.6±0.02ms     2.4 MB/sec    1.01      2.6±0.01ms     2.3 MB/sec
formatter/router.ts                      1.00      6.0±0.05ms    10.2 MB/sec    1.02      6.1±0.07ms    10.0 MB/sec
formatter/tex-chtml-full.js              1.00    445.3±3.05ms     2.0 MB/sec    1.00    444.1±4.12ms     2.1 MB/sec
formatter/three.min.js                   1.00    229.6±1.45ms     2.6 MB/sec    1.00    230.2±3.05ms     2.6 MB/sec
formatter/typescript.js                  1.00  1491.2±13.66ms     6.4 MB/sec    1.00  1487.3±14.16ms     6.4 MB/sec
formatter/vue.global.prod.js             1.00     73.0±0.53ms  1689.2 KB/sec    1.02     74.5±1.17ms  1656.6 KB/sec

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
@MichaReiser MichaReiser temporarily deployed to netlify-playground September 28, 2022 15:12 Inactive
Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
@MichaReiser MichaReiser requested a review from a team September 28, 2022 15:13
@MichaReiser MichaReiser temporarily deployed to netlify-playground September 28, 2022 15:13 Inactive
@MichaReiser MichaReiser changed the title refactor(rome_formatter): Best fitting fits definition feat(rome_js_formatter): Member chain formatting Sep 29, 2022
@MichaReiser MichaReiser merged commit a38f2ef into main Sep 29, 2022
@MichaReiser MichaReiser deleted the refactor/best-fitting-fits branch September 29, 2022 08:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants