refactor(rome_formatter): Comments #3227

MichaReiser · 2022-09-15T09:29:16Z

Summary

This PR is empty for now but I want to use it to merge all comment-related PRs into it as the individual PRs may not compile on their own.

I split the whole refactory mainly on file boundaries to avoid having to deal with many merge conflicts between the different PRs. However, this means, that individual PRs won't compile nor will tests be passing.

This is what this PR is for. It ensures that all changes compile together and that there are no regressing tests. The main motivation of splitting this PR into multiple smaller PRs is to ease reviewing.

Functionality

JSX Suppressions

It's now possible to suppress formatting of a JSX element or fragment by using

<div>
	{/* rome-format ignore: reason */}
	<span>This element
  does not get formatted</span>
</div>

Fix instability issues

Fixes #2768

Formatting order

The comment formatting relied on the fact that tokens (and thus nodes) are formatted in the order they get written to the final document to be able to decide whether to insert a space before/after a comment. However, there is no enforcement of this constraint and ignoring it leads to very subtle spacing bugs that are hard to notice and track down. There are even situations where it's surprisingly hard, if not even impossible, to format the document in order if it is necessary to test if some other element will break that only gets written later.

This PR removes the constraint that tokens must be formatted in order (they still must be written in the correct order to the output buffer).

Performance

The overall performance is improving. The reasons for this are

No longer necessary to track the formatted comments inside of the FormatState. The FormatState snapshot has a size of zero now, significantly reducing the cost of formatting statements.
The GroupElementsBuffer added a level of indirection to every write, state, context etc. calls. It also required to use a Boxed buffer snapshot to store its empty flag
The old implementation iterated over all trivia pieces to extract the suppression comments AND when formatting the token. The new implementation only iterates over the pieces to extract the comments but not when formatting the tokens.
FxHashMaps are fast :)

group                                    comments                               main
-----                                    --------                               ----
formatter/checker.ts                     1.00   246.5±18.26ms    10.5 MB/sec    1.07   263.8±20.88ms     9.9 MB/sec
formatter/compiler.js                    1.00   134.0±10.79ms     7.8 MB/sec    1.23   164.9±18.11ms     6.4 MB/sec
formatter/d3.min.js                      1.00    111.2±8.91ms     2.4 MB/sec    1.12   124.1±10.32ms     2.1 MB/sec
formatter/dojo.js                        1.00      7.1±0.28ms     9.6 MB/sec    1.25      8.9±0.53ms     7.7 MB/sec
formatter/ios.d.ts                       1.00    156.3±3.85ms    11.9 MB/sec    1.00    156.8±3.63ms    11.9 MB/sec
formatter/jquery.min.js                  1.00     28.4±1.96ms     2.9 MB/sec    1.26     35.7±3.02ms     2.3 MB/sec
formatter/math.js                        1.00   223.0±15.95ms     2.9 MB/sec    1.22   271.1±18.93ms     2.4 MB/sec
formatter/parser.ts                      1.04      5.4±0.45ms     9.1 MB/sec    1.00      5.2±0.24ms     9.4 MB/sec
formatter/pixi.min.js                    1.00    136.2±8.19ms     3.2 MB/sec    1.02   138.9±10.80ms     3.2 MB/sec
formatter/react-dom.production.min.js    1.00     35.9±2.33ms     3.2 MB/sec    1.13     40.4±2.95ms     2.8 MB/sec
formatter/react.production.min.js        1.00  1760.7±35.35µs     3.5 MB/sec    1.31      2.3±0.00ms     2.7 MB/sec
formatter/router.ts                      1.06      4.3±0.37ms    14.2 MB/sec    1.00      4.1±0.07ms    15.1 MB/sec
formatter/tex-chtml-full.js              1.00   276.5±14.46ms     3.3 MB/sec    1.16   321.2±24.47ms     2.8 MB/sec
formatter/three.min.js                   1.00    137.5±8.73ms     4.3 MB/sec    1.09    149.6±5.57ms     3.9 MB/sec
formatter/typescript.js                  1.00   923.6±52.21ms    10.3 MB/sec    1.14  1054.0±75.65ms     9.0 MB/sec
formatter/vue.global.prod.js             1.00     47.1±3.80ms     2.6 MB/sec    1.20     56.7±4.34ms     2.1 MB/sec

Test Plan

cargo test

netlify · 2022-09-15T09:29:40Z

✅ Deploy Preview for rometools ready!

Name	Link
🔨 Latest commit	`6a430dc`
🔍 Latest deploy log	https://app.netlify.com/sites/rometools/deploys/632c09332ca6c100081a9f00
😎 Deploy Preview	https://deploy-preview-3227--rometools.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

calibre-analytics · 2022-09-16T12:35:01Z

Comparing refactor(rome_formatter): Comments Snapshot #14 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	595ms ^from 566ms	0.049 ^no change	51ms ^from 46ms
Chrome Desktop ^{Chrome Desktop • Cable}	616ms ^no change	0.049 ^from 0.006	203ms ^from 239ms
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	244ms ^from 218ms	0.077 ^no change	18ms ^from 6ms
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	595ms ^from 566ms	0.049 ^no change	51ms ^from 46ms

1 page tested

Home

Browser previews

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 · 2022-09-16T12:40:10Z

Playground for commit dde6065

github-actions · 2022-09-22T06:21:59Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44939	44939	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	1621	1621	0
Failed	4325	4325	0
Panics	0	0	0
Coverage	27.26%	27.26%	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	12395	12395	0
Failed	3862	3862	0
Panics	0	0	0
Coverage	76.24%	76.24%	0.00%

MichaReiser · 2022-09-22T06:31:32Z

!bench_formatter

github-actions · 2022-09-22T07:01:57Z

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.00    422.6±8.11ms     6.2 MB/sec    1.14   479.7±16.16ms     5.4 MB/sec
formatter/compiler.js                    1.00    231.1±1.88ms     4.5 MB/sec    1.15   266.3±10.37ms     3.9 MB/sec
formatter/d3.min.js                      1.00    192.0±1.55ms  1397.9 KB/sec    1.12    214.5±8.75ms  1251.5 KB/sec
formatter/dojo.js                        1.00     12.9±0.07ms     5.3 MB/sec    1.06     13.7±0.13ms     5.0 MB/sec
formatter/ios.d.ts                       1.00    260.1±3.00ms     7.2 MB/sec    1.11    288.4±2.27ms     6.5 MB/sec
formatter/jquery.min.js                  1.00     55.3±0.30ms  1530.8 KB/sec    1.02     56.3±1.35ms  1503.8 KB/sec
formatter/math.js                        1.00    377.7±2.68ms  1755.7 KB/sec    1.10    415.3±4.45ms  1596.6 KB/sec
formatter/parser.ts                      1.00      8.6±0.03ms     5.6 MB/sec    1.06      9.1±0.04ms     5.3 MB/sec
formatter/pixi.min.js                    1.00    219.5±3.94ms  2047.1 KB/sec    1.06    233.2±3.34ms  1927.4 KB/sec
formatter/react-dom.production.min.js    1.00     66.6±1.05ms  1770.0 KB/sec    1.05     69.8±1.26ms  1688.8 KB/sec
formatter/react.production.min.js        1.00      3.2±0.02ms  1980.5 KB/sec    1.04      3.3±0.01ms  1904.0 KB/sec
formatter/router.ts                      1.00      6.8±0.03ms     9.1 MB/sec    1.10      7.5±0.10ms     8.2 MB/sec
formatter/tex-chtml-full.js              1.00    489.3±5.22ms  1907.0 KB/sec    1.09   531.6±21.27ms  1755.2 KB/sec
formatter/three.min.js                   1.00    244.2±1.75ms     2.4 MB/sec    1.10    268.5±9.25ms     2.2 MB/sec
formatter/typescript.js                  1.00  1544.0±22.67ms     6.2 MB/sec    1.11  1718.6±40.44ms     5.5 MB/sec
formatter/vue.global.prod.js             1.00     84.4±0.62ms  1461.3 KB/sec    1.08     91.4±1.45ms  1350.4 KB/sec

This PR removes the `FormatElement::Comment` element. It was used to this point because the `group` builder had to move `comments` outside of the start/end of the group. No such inspection is necessary after the refactor which is why the element can be deleted.

This PR moves the logic for formatting a node as `verbatim` to `rome_formatter` as it isn't JS specific

Comments Formatter integration

Co-authored-by: l3ops <leops@users.noreply.github.com>

…atters (#3245)

MichaReiser · 2022-09-22T07:05:33Z

!bench_formatter

github-actions · 2022-09-22T07:35:49Z

Formatter Benchmark Results

group                                    main                                   pr
-----                                    ----                                   --
formatter/checker.ts                     1.03    442.4±2.08ms     5.9 MB/sec    1.00    431.5±2.08ms     6.0 MB/sec
formatter/compiler.js                    1.04    236.6±1.40ms     4.4 MB/sec    1.00    226.7±1.57ms     4.6 MB/sec
formatter/d3.min.js                      1.11    195.8±1.07ms  1371.2 KB/sec    1.00    176.0±1.16ms  1524.9 KB/sec
formatter/dojo.js                        1.07     12.2±0.08ms     5.6 MB/sec    1.00     11.4±0.05ms     6.0 MB/sec
formatter/ios.d.ts                       1.02    265.4±2.35ms     7.0 MB/sec    1.00    260.8±1.46ms     7.2 MB/sec
formatter/jquery.min.js                  1.11     53.9±0.47ms  1570.8 KB/sec    1.00     48.3±0.50ms  1750.8 KB/sec
formatter/math.js                        1.08    389.3±1.78ms  1703.1 KB/sec    1.00    358.9±1.99ms  1847.6 KB/sec
formatter/parser.ts                      1.07      8.2±0.03ms     5.9 MB/sec    1.00      7.6±0.03ms     6.4 MB/sec
formatter/pixi.min.js                    1.10    222.0±1.46ms  2024.5 KB/sec    1.00    201.1±1.65ms     2.2 MB/sec
formatter/react-dom.production.min.js    1.09     65.1±0.58ms  1810.9 KB/sec    1.00     59.9±0.42ms  1968.7 KB/sec
formatter/react.production.min.js        1.09      3.0±0.01ms     2.1 MB/sec    1.00      2.7±0.01ms     2.2 MB/sec
formatter/router.ts                      1.04      6.4±0.01ms     9.6 MB/sec    1.00      6.1±0.03ms    10.0 MB/sec
formatter/tex-chtml-full.js              1.09    499.2±1.69ms  1869.2 KB/sec    1.00    458.8±1.72ms  2034.0 KB/sec
formatter/three.min.js                   1.10    249.7±1.45ms     2.4 MB/sec    1.00    227.8±1.21ms     2.6 MB/sec
formatter/typescript.js                  1.02   1616.3±7.61ms     5.9 MB/sec    1.00   1577.2±8.58ms     6.0 MB/sec
formatter/vue.global.prod.js             1.09     84.6±0.47ms  1457.8 KB/sec    1.00     77.8±0.63ms  1585.5 KB/sec

MichaReiser changed the title ~~Empty File to create placeholder PR~~ refactor(rome_formatter): Comments Sep 15, 2022

This was referenced Sep 15, 2022

refactor(rome_formatter): Comments all in one #3212

Closed

feat(rome_formatter): Comments multi map #3230

Merged

MichaReiser force-pushed the refactor/comments branch from 4946e49 to 8ce92f8 Compare September 15, 2022 12:27

MichaReiser added the A-Formatter Area: formatter label Sep 15, 2022

MichaReiser added this to the 0.10.0 milestone Sep 15, 2022

This was linked to issues Sep 15, 2022

📎 Comments Formatting #2664

Closed

Comment formatting - stability issues #2768

Closed

Bug Inline comment spacing #2814

Closed

MichaReiser mentioned this pull request Sep 16, 2022

refactor(rome_formatter): Part 2: Comments API #3239

Merged

MichaReiser temporarily deployed to netlify-playground September 16, 2022 12:32 Inactive

MichaReiser marked this pull request as ready for review September 16, 2022 12:33

MichaReiser requested a review from ematipico as a code owner September 16, 2022 12:33

MichaReiser requested a review from a team September 16, 2022 12:33

MichaReiser mentioned this pull request Sep 16, 2022

refactor(rome_formatter): Part 3: Delete FormatElement::Comment #3240

Merged

MichaReiser force-pushed the refactor/comments branch from 68b96aa to 301ba39 Compare September 16, 2022 14:58

MichaReiser temporarily deployed to netlify-playground September 16, 2022 14:58 Inactive

MichaReiser had a problem deploying to netlify-playground September 19, 2022 14:50 Failure

MichaReiser had a problem deploying to netlify-playground September 19, 2022 14:51 Failure

MichaReiser mentioned this pull request Sep 19, 2022

test(rome_js_formatter): Add prettier's JSX tests #3255

Merged

MichaReiser had a problem deploying to netlify-playground September 20, 2022 11:16 Failure

MichaReiser had a problem deploying to netlify-playground September 20, 2022 12:29 Failure

MichaReiser had a problem deploying to netlify-playground September 20, 2022 16:28 Failure

MichaReiser temporarily deployed to netlify-playground September 22, 2022 06:13 Inactive

MichaReiser requested a review from leops as a code owner September 22, 2022 06:16

MichaReiser temporarily deployed to netlify-playground September 22, 2022 06:16 Inactive

MichaReiser and others added 15 commits September 22, 2022 09:03

Empty File to create placeholder PR

7ff14a9

Empty File to create placeholder PR

9977d28

feat(rome_formatter): Comments multi map (#3230)

8c0ee68

Comments infrastructure

f9a0398

Document Comments API

39abce7

refactor(rome_formatter): Move verbatim formatting to rome_formatter

a9f9ef4

This PR moves the logic for formatting a node as `verbatim` to `rome_formatter` as it isn't JS specific

Rename cst back to token

fb3e2a9

Comments Formatter integration

Documentation and naming

ba0fb8b

Update crates/rome_formatter/src/comments.rs

eae6f8b

Co-authored-by: l3ops <leops@users.noreply.github.com>

Rename soft_line_indent_or_spaced to soft_space_or_block_indent

f42e894

refactor(rome_js_formatter): Part 6: JS Comments formatting

0abbbb6

Documentation

7581c1d

refactor(rome_js_formatter): Part 7: Use comments API in JS node form…

74147a3

…atters (#3245)

chore(xtask): Part 8: Add line breaks before doc comments (#3246)

6a430dc

MichaReiser force-pushed the refactor/comments branch from f4dbc00 to 6a430dc Compare September 22, 2022 07:05

MichaReiser temporarily deployed to netlify-playground September 22, 2022 07:05 Inactive

MichaReiser merged commit a9563e4 into main Sep 22, 2022

MichaReiser deleted the refactor/comments branch September 22, 2022 07:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(rome_formatter): Comments #3227

refactor(rome_formatter): Comments #3227

MichaReiser commented Sep 15, 2022 •

edited

Loading

netlify bot commented Sep 15, 2022 •

edited

Loading

calibre-analytics bot commented Sep 16, 2022 •

edited

Loading

Browser previews

Most significant changes

github-actions bot commented Sep 16, 2022 •

edited

Loading

github-actions bot commented Sep 22, 2022

MichaReiser commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

MichaReiser commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

refactor(rome_formatter): Comments #3227

refactor(rome_formatter): Comments #3227

Conversation

MichaReiser commented Sep 15, 2022 • edited Loading

Summary

Functionality

JSX Suppressions

Fix instability issues

Formatting order

Performance

Test Plan

netlify bot commented Sep 15, 2022 • edited Loading

✅ Deploy Preview for rometools ready!

calibre-analytics bot commented Sep 16, 2022 • edited Loading

1 page tested

Browser previews

Most significant changes

github-actions bot commented Sep 16, 2022 • edited Loading

github-actions bot commented Sep 22, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

MichaReiser commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

Formatter Benchmark Results

MichaReiser commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

Formatter Benchmark Results

MichaReiser commented Sep 15, 2022 •

edited

Loading

netlify bot commented Sep 15, 2022 •

edited

Loading

calibre-analytics bot commented Sep 16, 2022 •

edited

Loading

github-actions bot commented Sep 16, 2022 •

edited

Loading