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

feat(rome_js_formatter): Template formatting #3063

Merged
merged 3 commits into from Aug 16, 2022

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Aug 16, 2022

This PR improves Rome's formatting of JsTemplates and TsTemplates to closer match Prettier's formatting.

It mainly implements:

  • simple expressions that never break even if the template literal, as a result thereof, exceeds the line width
  • Aligning expressions in template literals with the last template chunk

This PR does not implement Prettier's custom formatting of Jest specs, and it doesn't implement custom comments formatting.

Closes #2446

Tests

I manually verified the snapshot changes. There are some remaining differences but they are rooted in the fact that some, expression formatting isn't compatible with prettier yet (mainly binary expression, call arguments), or that prettier supports formatting HTML, graphql, etc.

File Based Average Prettier Similarity: 79.75% -> 80.03%
Line Based Average Prettier Similarity: 76.52% -> 76.85%

@MichaReiser MichaReiser requested a review from a team August 16, 2022 09:10
@MichaReiser MichaReiser temporarily deployed to aws August 16, 2022 09:10 Inactive
@MichaReiser MichaReiser added this to the 0.9.0 milestone Aug 16, 2022
@MichaReiser MichaReiser added the A-Formatter Area: formatter label Aug 16, 2022
@MichaReiser MichaReiser mentioned this pull request Aug 16, 2022
17 tasks
@github-actions
Copy link

github-actions bot commented Aug 16, 2022

@IWANABETHATGUY
Copy link
Contributor

Congratulation! We reached 80%!!!!

@cloudflare-pages
Copy link

cloudflare-pages bot commented Aug 16, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 9461237
Status:⚡️  Build in progress...

View logs

crates/rome_formatter/src/printer/printer_options/mod.rs Outdated Show resolved Hide resolved
Comment on lines 214 to 226
for _ in 1..level {
indented = FormatElement::Indent(vec![indented].into_boxed_slice());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It nests indents for as long until it reaches the desired indent level, as explained in the comment at the top of the format width

Copy link
Contributor

Choose a reason for hiding this comment

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

And we start from 1 because we checked 0 before? If level is 1 the indentation is now 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to go from 0..level, and remove the indent around the initial element. That's probably easier to understand

This PR improves Rome's formatting of `JsTemplate`s and `TsTemplate`s to closer match Prettier's formatting.

It mainly implements:

* simple expressions that never break even if the template literal, as a result thereof, exceeds the line width
* Aligning expressions in template literals with the last template chunk

This PR does not implement Prettier's custom formatting of `Jest` specs, and it doesn't implement custom comments formatting.

## Tests

I manually verified the snapshot changes. There are some remaining differences but they are rooted in the fact that some, expression formatting isn't compatible with prettier yet (mainly binary expression, call arguments)
@MichaReiser MichaReiser temporarily deployed to aws August 16, 2022 11:37 Inactive
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Could you please add/update some example that reflects this new layout approach? By updating I mean just adding some comment saying the expected layout

Comment on lines 214 to 226
for _ in 1..level {
indented = FormatElement::Indent(vec![indented].into_boxed_slice());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

And we start from 1 because we checked 0 before? If level is 1 the indentation is now 2?

@@ -18,15 +18,15 @@ output
`;

// don't break
const bar =`but where will ${this.fanta} wrap ${baz} ${"hello"} template literal? ${bar.ff.sss} long long long long ${foo.[3]} long long long long long long`;
const bar =`but where will ${this.fanta} wrap ${baz} ${"hello"} template literal? ${bar.ff.sss} long long long long ${foo[3]} long long long long long long`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the source of this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it wasn't valid Javascript ${foo.[3]} is not valid

@@ -7,7 +7,7 @@ repository = "https://github.com/rome/tools"
license = "MIT"

[package.metadata.wasm-pack.profile.dev]
# wasm-opt = ['-O1']
wasm-opt = ['-O1']
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was unintentionall that I commited this commented out :D

This is needed because our playground stack overflows, even for very small samples if we don't perform at least some wasm optimisations.

@MichaReiser MichaReiser temporarily deployed to aws August 16, 2022 13:07 Inactive
@MichaReiser MichaReiser temporarily deployed to aws August 16, 2022 13:12 Inactive
@MichaReiser MichaReiser merged commit 2a40ff3 into main Aug 16, 2022
@MichaReiser MichaReiser deleted the feat/template-formatting branch August 16, 2022 13:19
IWANABETHATGUY pushed a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022
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.

Template literal formatting
3 participants