feat(rome_js_formatter): Hard line breaks in object expressions. #2458

cpojer · 2022-04-17T01:32:29Z

Summary

This is the initial draft for respecting a newline in an object expression (or import specifier) in between an opening curly brace { and the first item in the member list. This should make Rome match prettier in this situation. Again, I have no idea what I'm doing :)

Open questions to get this over the line:

For some reason formatter::js_module::specs::js::module::arrow::arrow_nested fails and I'm unsure why. Please help!
See some inline questions.

Test Plan

Added tests.

cpojer · 2022-04-17T01:33:06Z

crates/rome_js_formatter/src/formatter.rs

+    /// Formats a group delimited by an opening and closing token, placing the
+    /// content in an [indent] group with [hard_line_break] tokens at the
+    /// start and end
+    pub(crate) fn format_delimited_block_hard_line_break(


This is mostly a copy of format_delimited_soft_block_spaces and honestly I don't really understand the name of that function, so I'm not sure if the name of this function here makes sense. Please let me know if there is a better one.

cpojer · 2022-04-17T01:33:48Z

crates/rome_js_formatter/tests/specs/js/module/object/getter_setter.js.snap

+let a = {
+	get foo() {},
+};
+let b = {
+	set foo(a) {},
+};


This matches the input in getter_setter.js.

That file is the source code, you should check the file that finishes with .prettier-snap.

Check their playground

cpojer · 2022-04-17T01:36:05Z

crates/rome_js_formatter/src/js/expressions/object_expression.rs

+        let has_newlines = has_newline(&members.syntax());
        let members = members.format(formatter)?;
-
        if members.is_empty() {
            formatter.format_delimited_soft_block_indent(&l_curly_token?, members, &r_curly_token?)
+        } else if has_newlines == true {
+            formatter.format_delimited_block_hard_line_break(
+                &l_curly_token?,
+                members,
+                &r_curly_token?,
+            )
        } else {
            formatter.format_delimited_soft_block_spaces(&l_curly_token?, members, &r_curly_token?)


We have about 10 callsites for format_delimited_soft_block_spaces. So far I'm only adding this feature to two of them because I'm looking for guidance on the optimal approach:

Use the above approach and add the newline check and explicit call to format_delimited_block_hard_line_break to every callsite.

Rename format_delimited_soft_block_spaces to ??? and pass a has_newlines boolean to the call, then decide on using soft or hard breaks within that single function.

Option 1 is more explicit, option 2 is less code. Which fits better with Rome?

The functions format_delimited_* were an attempt to group common patterns that we were starting to see across the board. If you think that this is going to be a common pattern, then it makes sense to make it generic, instead if this is just a single case, then it should stay isolated.

The has_newlines argument won't be needed, as explained in my comment: https://github.com/rome/tools/pull/2458/files#r851766933

MichaReiser

Thanks Christoph.

Regarding the failing :arrow_nested test.

The test is failing because the formatter output isn't stable. Meaning, that re-formatting the formatted output yields a different result. Why or how it can be avoided I don't know right now. @leops and @ematipico have more experience with this kind of issues.

crates/rome_js_formatter/src/formatter.rs

crates/rome_js_formatter/src/utils/mod.rs

crates/rome_js_formatter/src/js/expressions/object_expression.rs

cpojer · 2022-04-18T00:13:59Z

Thanks for the feedback so far. Remaining questions:

The unstable printing test issue.
Are you happy with the has_leading_newline + format_delimited_block_indent calls or do you somehow want to pull responsibility into format_delimited_block_indent so it can be handled generically? The decision on that will determine the final state of this PR.

leops · 2022-04-19T09:45:06Z

After looking into it, the stability issue seem to be caused by the has_hard_line_breaks check in the formatting of call expression (at crates\rome_js_formatter\src\utils\call_expression.rs:L480): replacing the soft line breaks with hard ones in the formatting of the object expression causes the outer call chain to emit an additional group that cause the entire statement to print differently.
This check was originally intended to approximately replicate the logic of the printer but the approximation becomes evident in the presence of soft line breaks, so I think we should get rid of this logic in favor of something closer to what Prettier does (and doesn't need to inspect FormatElement) since we're aiming for better compatibility. But that's outside of the scope of this PR ...

MichaReiser · 2022-04-21T06:29:23Z

After looking into it, the stability issue seem to be caused by the has_hard_line_breaks check in the formatting of call expression (at crates\rome_js_formatter\src\utils\call_expression.rs:L480): replacing the soft line breaks with hard ones in the formatting of the object expression causes the outer call chain to emit an additional group that cause the entire statement to print differently. This check was originally intended to approximately replicate the logic of the printer but the approximation becomes evident in the presence of soft line breaks, so I think we should get rid of this logic in favor of something closer to what Prettier does (and doesn't need to inspect FormatElement) since we're aiming for better compatibility. But that's outside of the scope of this PR ...

Is anyone looking into this? If not, can we track this as an issue?

leops · 2022-04-21T07:20:40Z

Is anyone looking into this? If not, can we track this as an issue?

I don't think anyone is working on this right now, I can create an independent issue for it. Since it seems to be blocking for this PR I'm not really sure how we want to handle that, do would we hold on to merging this until the issue is fixed or merge this change now and fix the stability issue later ?

cpojer · 2022-04-21T07:21:59Z

We cannot merge this PR unless we disable the test. It would be great to fix this issue, and then I can rebase my work on that.

@MichaReiser Still looking for guidance on my second question about whether the implementation in this PR is acceptable or whether things should be moved around.

MichaReiser · 2022-04-21T08:45:12Z

We cannot merge this PR unless we disable the test. It would be great to fix this issue, and then I can rebase my work on that.

@MichaReiser Still looking for guidance on my second question about whether the implementation in this PR is acceptable or whether things should be moved around.

format_delimited_block_indent is used for example for the {} of classes where prettier doesn't preserve the line breaks. There are more call sites where this is the case as well. That's why I think your current implementation that explicitly handles it for objects is the right choice.

cpojer · 2022-04-21T15:58:47Z

Ok this PR is ready for review. It updates a lot of snapshots to look like the input code (I initially got confused and was comparing the snapshot changes to the previous snapshot version, which obviously has changed!). I also changed how TypeScript interface declarations are printed since in Prettier they will always break, even with just one prop (thanks @MichaReiser).

The only blocker before merging besides review comments is the failing arrows test. I think it would also be great to fix the test infra so that it keeps running instead of crashing on the first failing test like this one. It's quite confusing otherwise when it doesn't make it through the whole test run.

MichaReiser

Thanks, @cpojer for updating your PR and sorry for the merge conflicts.

The code looks good to me but this PR now goes further than what prettier does. Prettier only preserves line breaks for object literals and object type literals, but not for imports/exports/assertions/etc...

I would recommend keeping compatibility with Prettier for now.

crates/rome_js_formatter/src/js/assignments/object_assignment_pattern.rs

crates/rome_js_formatter/src/js/bindings/object_binding_pattern.rs

crates/rome_js_formatter/src/js/module/export_named_clause.rs

crates/rome_js_formatter/src/js/module/import_assertion.rs

crates/rome_js_formatter/src/js/module/named_import_specifiers.rs

cpojer · 2022-04-22T06:06:36Z

Oh that's unfortunate. Thank you so much for double checking. Subjectively, what I had is better for consistency but objectively it's better to align with prettier. I updated the PR and reverted the changes.

MichaReiser · 2022-04-22T06:08:59Z

Oh that's unfortunate. Thank you so much for double checking. Subjectively, what I had is better for consistency but objectively it's better to align with prettier. I updated the PR and reverted the changes.

Thanks. Would you mind having a look at the lint and test failures. We should then be good to merge

cpojer · 2022-04-22T06:12:17Z

Done (I think!).

ematipico · 2022-04-22T11:08:19Z

Tests still failing 😔

It seems there are some snapshots that break when reformatting... And few needs to be updated again.

This happens because if a change affects prettier test suite AND our internal suite, we need to run and update the snapshots twice.

Not sure why this happens but for now that's how it is (sorry)

cpojer · 2022-04-22T11:13:29Z

@ematipico how do I update the snapshots? When I run cargo test the only failing test is formatter::js_module::specs::js::module::arrow::arrow_nested like we discussed.

ematipico · 2022-04-22T11:40:19Z

cargo insta review

Instead if you keep getting a diff error, it means that what it is formatted the first time doesn't match the output of the second pass of the formatter (on the emitted code of the first time)

cpojer · 2022-04-28T23:11:48Z

@ematipico Rebased and resolved conflicts, now the arrow test is failing with a different issue it looks like.

ematipico

It seems that the algorithm is not working as intended.

Now the object expressions always break, regardless if they have new lines or not.

I strongly suggest to create a new test case in our test suite, ideally the code that Micha attached to the issue. This will help me to identify the new file and see if it matches the intended behaviour

ematipico · 2022-04-29T08:24:07Z

crates/rome_js_formatter/tests/specs/js/module/object/getter_setter.js.snap

+let a = {
+	get foo() {},
+};
+let b = {
+	set foo(a) {},
+};


That file is the source code, you should check the file that finishes with .prettier-snap.

Check their playground

cpojer · 2022-04-29T09:31:46Z

As explained on Discord, this is very confusing but the code here is correct except for the example where it breaks the outer parameters list in a function call into newlines which prettier doesn't do.

The input code for all of these examples has those newlines in place, so the snapshots are being updated from not printing those newlines to printing them. When you look at the snapshot changes, it looks like things are broken, but when you compare the source with the new snapshot versions you'll see that it is correct. I got confused about the exact same thing before I wrapped my head around that.

ematipico · 2022-05-03T09:24:31Z

The reason why this PR keeps failing is because we don't yet format call arguments as prettier does.

Prettier uses #2529 (conditional groups) and an extensive heuristic to how to print them. The work in tracked in #2421 , so I guess we should put this PR on hold until we have everything we need in place.

This PR simplifies the member printing logic to make sure its output is stable. This results in a small regression ``` Before: **File Based Average Prettier Similarity**: 69.29% **Line Based Average Prettier Similarity**: 64.28% After: **File Based Average Prettier Similarity**: 69.13% **Line Based Average Prettier Similarity**: 64.06% ``` But unblocks my work on refactoring the printer behaviour (which seems to trigger this now a lot), and #2458

cpojer · 2022-05-22T12:09:00Z

@MichaReiser I redid this PR with the new formatting infra, please check out whether this matches expectations!

cpojer · 2022-05-22T12:28:55Z

Not sure if I did it right (env REPORT_PRETTIER=1 cargo test -p rome_js_formatter --test prettier_tests), but here is the diff compared to main:

File Based Average Prettier Similarity: 69.38% → 70.89%
Line Based Average Prettier Similarity: 64.39% → 65.39%

MichaReiser

This is such an awesome contribution. Thank you @cpojer .

cpojer commented Apr 17, 2022

View reviewed changes

MichaReiser reviewed Apr 17, 2022

View reviewed changes

crates/rome_js_formatter/src/formatter.rs Outdated Show resolved Hide resolved

crates/rome_js_formatter/src/utils/mod.rs Outdated Show resolved Hide resolved

ematipico reviewed Apr 17, 2022

View reviewed changes

crates/rome_js_formatter/src/js/expressions/object_expression.rs Outdated Show resolved Hide resolved

cpojer force-pushed the hard-line-break-object-expressions branch from b029f4c to 8e21da3 Compare April 18, 2022 00:09

leops mentioned this pull request Apr 21, 2022

📎 Improve the formatting of call chains #2475

Closed

cpojer force-pushed the hard-line-break-object-expressions branch 3 times, most recently from 4507fc0 to 5390c57 Compare April 21, 2022 15:54

cpojer force-pushed the hard-line-break-object-expressions branch 2 times, most recently from 5fee185 to e2d4d9c Compare April 21, 2022 16:46

MichaReiser suggested changes Apr 22, 2022

View reviewed changes

cpojer force-pushed the hard-line-break-object-expressions branch from e2d4d9c to 861a683 Compare April 22, 2022 06:05

cpojer force-pushed the hard-line-break-object-expressions branch from 861a683 to 286c2b9 Compare April 22, 2022 06:10

cpojer changed the title ~~feat(rome_js_formatter): Initial version of hard line breaks in object expressions.~~ feat(rome_js_formatter): Hard line breaks in object expressions. Apr 22, 2022

cpojer force-pushed the hard-line-break-object-expressions branch from 1245005 to 3f8eb9c Compare April 22, 2022 11:47

cpojer force-pushed the hard-line-break-object-expressions branch from 3f8eb9c to 7a6a006 Compare April 28, 2022 23:08

ematipico suggested changes Apr 29, 2022

View reviewed changes

cpojer force-pushed the hard-line-break-object-expressions branch 2 times, most recently from 4ee69b5 to 0539c50 Compare May 3, 2022 01:18

ematipico approved these changes May 3, 2022

View reviewed changes

ematipico added the PR: on hold A PR that needs some upstream work before getting merged. label May 3, 2022

cpojer force-pushed the hard-line-break-object-expressions branch 2 times, most recently from ea7b3cc to 0cfc66c Compare May 7, 2022 01:10

MichaReiser mentioned this pull request May 13, 2022

fix(formatter): Tighten has_line_breaks method #2581

Merged

MichaReiser mentioned this pull request May 13, 2022

fix(formatter): Stable member chain printing #2582

Merged

Initial version of hard line breaks in object expressions.

3d50ac9

cpojer force-pushed the hard-line-break-object-expressions branch from 0cfc66c to 3d50ac9 Compare May 22, 2022 12:05

cpojer removed the PR: on hold A PR that needs some upstream work before getting merged. label May 22, 2022

MichaReiser approved these changes May 23, 2022

View reviewed changes

MichaReiser merged commit e044cfc into rome:main May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_formatter): Hard line breaks in object expressions. #2458

feat(rome_js_formatter): Hard line breaks in object expressions. #2458

cpojer commented Apr 17, 2022

cpojer Apr 17, 2022

cpojer Apr 17, 2022

ematipico Apr 29, 2022

cpojer Apr 17, 2022

ematipico Apr 17, 2022

MichaReiser left a comment

cpojer commented Apr 18, 2022

leops commented Apr 19, 2022

MichaReiser commented Apr 21, 2022

leops commented Apr 21, 2022

cpojer commented Apr 21, 2022

MichaReiser commented Apr 21, 2022

cpojer commented Apr 21, 2022

MichaReiser left a comment

cpojer commented Apr 22, 2022

MichaReiser commented Apr 22, 2022

cpojer commented Apr 22, 2022

ematipico commented Apr 22, 2022

cpojer commented Apr 22, 2022

ematipico commented Apr 22, 2022

cpojer commented Apr 28, 2022

ematipico left a comment

ematipico Apr 29, 2022

cpojer commented Apr 29, 2022

ematipico commented May 3, 2022 •

edited

Loading

cpojer commented May 22, 2022

cpojer commented May 22, 2022

MichaReiser left a comment

feat(rome_js_formatter): Hard line breaks in object expressions. #2458

feat(rome_js_formatter): Hard line breaks in object expressions. #2458

Conversation

cpojer commented Apr 17, 2022

Summary

Test Plan

cpojer Apr 17, 2022

Choose a reason for hiding this comment

cpojer Apr 17, 2022

Choose a reason for hiding this comment

ematipico Apr 29, 2022

Choose a reason for hiding this comment

cpojer Apr 17, 2022

Choose a reason for hiding this comment

ematipico Apr 17, 2022

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

cpojer commented Apr 18, 2022

leops commented Apr 19, 2022

MichaReiser commented Apr 21, 2022

leops commented Apr 21, 2022

cpojer commented Apr 21, 2022

MichaReiser commented Apr 21, 2022

cpojer commented Apr 21, 2022

MichaReiser left a comment

Choose a reason for hiding this comment

cpojer commented Apr 22, 2022

MichaReiser commented Apr 22, 2022

cpojer commented Apr 22, 2022

ematipico commented Apr 22, 2022

cpojer commented Apr 22, 2022

ematipico commented Apr 22, 2022

cpojer commented Apr 28, 2022

ematipico left a comment

Choose a reason for hiding this comment

ematipico Apr 29, 2022

Choose a reason for hiding this comment

cpojer commented Apr 29, 2022

ematipico commented May 3, 2022 • edited Loading

cpojer commented May 22, 2022

cpojer commented May 22, 2022

MichaReiser left a comment

Choose a reason for hiding this comment

ematipico commented May 3, 2022 •

edited

Loading