feat(rome_js_formatter,rome_cli): Add semicolons option #3702
Conversation
✅ Deploy Preview for docs-rometools ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
MichaReiser
force-pushed
the
feat/add-semicolons-options
branch
from
889f435
to
b0635d7
Compare
|
Appreciate the much clearer naming than in Prettier 👍 ( Since not printing semicolons increases the risk of introducing a bug that causes the formatter to print code that behaves differently from the input, what do you think of testing this by doing a |
MichaReiser
force-pushed
the
feat/add-semicolons-options
branch
from
b0635d7
to
21995a5
Compare
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
I like the idea and tried to implement this real quick, but it turned out to be a bit more complicated: If a statement needing a semicolon is at the start of a block, then the ((b) => (c) => (d) => {
return 3;
})(x);
// vs
;((b) => (c) => (d) => {
return 3;
})(x);The logic then very quickly becomes rather involved, particularly because semicolons must be skipped too. |
|
Comparing feat(rome_js_formatter,rome_cli): Add semicolons option Snapshot #6 to median since last deploy of rome.tools.
1 page tested
|
| Chrome Desktop | iPhone, 4G LTE | Motorola Moto G Power, 3G connection |
|---|---|---|
 |
 |
 |
Most significant changes
 |
Value | Budget |
|---|---|---|
| Total JavaScript Size in Bytes Chrome Desktop |
1.33 MB  from 86.8 KB |
 |
| Total JavaScript Size in Bytes iPhone, 4G LTE |
1.33 MB from 86.8 KB |
|
| Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection |
1.33 MB from 86.8 KB |
|
| Cumulative Layout Shift Chrome Desktop |
0.044  from 0.005 |
|
| Cumulative Layout Shift iPhone, 4G LTE |
0.082  from 0.01 |
|
16 other significant changes: Cumulative Layout Shift on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, 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, Total Image Size in Bytes on Chrome Desktop, Total Image Size in Bytes on iPhone, 4G LTE, Total Image Size in Bytes on Motorola Moto G Power, 3G connection, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection
Calibre: Site dashboard | View this PR | Edit settings | View documentation
|
!bench_formatter |
Formatter Benchmark Results |
There was a problem hiding this comment.
I left some feedback.
Could you please add two new test cases inside the format command?
- an invalid case where we print the error message for invalid value for
--semicolons - a valid case where we pass
--semicolons=as-neededand the correct formatted code is printed
MichaReiser
force-pushed
the
feat/add-semicolons-options
branch
from
8134a8f
to
59cfea8
Compare
MichaReiser
force-pushed
the
feat/add-semicolons-options
branch
from
59cfea8
to
27ace9c
Compare
| @@ -34,6 +34,11 @@ export enum LoadingState { | |||
| Error, | |||
| } | |||
|
|
|||
| export enum Semicolons { | |||
There was a problem hiding this comment.
Nit: Isn't it possible to import this type from @rometools/wasm-web instead ?
There was a problem hiding this comment.
It probably should be, but the types are not exported.
There was a problem hiding this comment.
Probably because you need to run the script to create the wasm-web package. This type should not be here, could you please delete it?
There was a problem hiding this comment.
I re-ran the script but the problem is that wasm-web only exports very few types, options and configuration aren't one of them and I don't feel like playing infer trickeries to get to the type. Let's tackle this separately. I continued what all other options did before.
Add an option to configure whether the formatter should always print semicolons for statements or only as needed.
| @@ -34,6 +34,11 @@ export enum LoadingState { | |||
| Error, | |||
| } | |||
|
|
|||
| export enum Semicolons { | |||
There was a problem hiding this comment.
Probably because you need to run the script to create the wasm-web package. This type should not be here, could you please delete it?
MichaReiser
force-pushed
the
feat/add-semicolons-options
branch
from
27ace9c
to
c470039
Compare
Summary
Adds the new
--semicolonsoption that can either be:always(default): Always inserts semicolons at the end of statement or class and type members.as-needed(same as Prettier's--no-semi): Only inserts semicolons where it is necessary to not change the semantics of the program.Test Plan
I added a handful of tests myself and copied over the prettier tests.
I plan to ship a nightly and ask people to try Rome in their repository and report any issue to get better test coverage.