feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression #2698

denbezrukov · 2022-06-11T19:14:51Z

Summary

This PR fixes #2410
This is the initial draft for JsAnyAssignmentLike.
I'd like to propose JsAnyAssignmentLike node union to handle different cases.
Prettier uses printAssignment for:

ObjectProperty
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/printer-estree.js#L388
AssignmentExpression https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L79
VariableDeclarator https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L90
TSTypeAliasDeclaration
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/type-annotation.js#L88
ClassAccessorProperty, TSAbstractPropertyDefinition
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/class.js#L234

Open questions

I'm not sure about the name JsAnyAssignmentLike because some cases aren't assignment.

Test Plan

Add new spec tests cases for JsAssignmentExpression.

…pertyObjectMember and JsAssignmentExpression

denbezrukov · 2022-06-11T19:21:32Z

crates/rome_js_formatter/src/utils/assignment_like.rs

+                let width = write_member_name(&property.name()?, f)?;
+                let text_width_for_break =
+                    (f.context().tab_width() + MIN_OVERLAP_FOR_BREAK) as usize;
+                Ok(width < text_width_for_break)


Prettier checks short name only for ObjectMember.
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L413

True, I noticed that too yesterday

denbezrukov · 2022-06-11T19:24:05Z

If we choose this approach we'll need to add more variants for layout:

https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L37-L74

…yout

denbezrukov · 2022-06-12T19:42:23Z

crates/rome_js_formatter/src/utils/assignment_like.rs

+}
+
+pub(crate) fn is_break_after_operator(right: &JsAnyExpression) -> SyntaxResult<bool> {
+    if has_leading_newline(right.syntax()) && right.syntax().has_leading_comments() {


I'm not sure that it works properly, because after format comments can be(???) in another place.
https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L126

ematipico · 2022-06-13T12:17:54Z

I'm not sure about the name JsAnyAssignmentLike because some cases aren't assignment.

I think that's fine, as long as we document the data structure and explain what kind of situations it handles.

# Conflicts: # crates/rome_js_formatter/tests/specs/prettier/js/assignment/lone-arg.js.snap

denbezrukov · 2022-06-17T08:22:27Z

@ematipico Could you help me please? 🙏🏽
I saw #2721.
Should I close this PR?

ematipico · 2022-06-17T09:46:47Z

@ematipico Could you help me please? 🙏🏽
I saw #2721.
Should I close this PR?

Oh, please don't. I would actually close mine. The problem is that I didn't see what issues this PR is closing.

Could you please update the description? This is really important to us, it avoids double work.

I will help of course!

denbezrukov · 2022-06-17T10:09:34Z

Could you please update the description? This is really important to us, it avoids double work.

I'm sorry for that. I didn't check the issues before opening PR. My bad. 😓

I would actually close mine.

I'm going to copy has_new_line_before_comment and spec test cases (:

…ome#2721

ematipico

Thank you for the great contribution!

About the next steps, here's what I had in mind. As you explained, prettier uses this way of formatting for many things, and eventually we will need to check the "left" and "right" part of an assignment like.

Which means that "left" and "right" should be grouped inside another unions, because the "left" part of an assignment like might differ based on what we are formatting. My idea was to create a LeftAssignmentLike:

declare_node_union! {
    pub(crate) LeftAssignmentLike = JsAnyAssignmentPattern | JsAnyObjectMemberName
}

If you're OK which this plan, I will work on #2416 and you can choose another one (maybe VariableDeclarator?). Feel free to comment in the issue and I will assign that to you

crates/rome_js_formatter/src/utils/assignment_like.rs

crates/rome_js_formatter/src/js/objects/property_object_member.rs

crates/rome_js_formatter/src/utils/assignment_like.rs

ematipico · 2022-06-17T11:14:44Z

crates/rome_js_formatter/src/utils/assignment_like.rs

+                let width = write_member_name(&property.name()?, f)?;
+                let text_width_for_break =
+                    (f.context().tab_width() + MIN_OVERLAP_FOR_BREAK) as usize;
+                Ok(width < text_width_for_break)


True, I noticed that too yesterday

crates/rome_js_formatter/src/utils/assignment_like.rs

ematipico · 2022-06-17T11:29:32Z

If we choose this approach we'll need to add more variants for layout:

* [prettier/prettier@`2316e2f`/src/language-js/print/assignment.js#L37-L74](https://github.com/prettier/prettier/blob/2316e2fecd171335fc00f3ec97b7e46cc8964153/src/language-js/print/assignment.js#L37-L74)

I can definitely see that. And we should add variants while we implement other assignments. I already started implementing Chain and ChainTail

denbezrukov · 2022-06-17T12:10:54Z

If you're OK which this plan, I will work on #2416 and you can choose another one (maybe VariableDeclarator?). Feel free to comment in the issue and I will assign that to you

It sounds great. 👍👍👍

VariableDeclarator - #2423 ? I've already taken it. After this PR I'll work on it.

denbezrukov · 2022-06-17T12:16:13Z

I can definitely see that. And we should add variants while we implement other assignments. I already started implementing Chain and ChainTail

I noticed that to implement this variant we need to know layout outside format_with in format_assignment_like function.
Because chain variants don't have outer group. Do you have any idea how to get this?

https://github.com/prettier/prettier/blob/877ae8ec16369dc0cf79ee36019bfdce40429eae/src/language-js/print/assignment.js#L63-L70

ematipico

Great! Thank you very much! This contribution is great! We can rebase and merge it :)

# Conflicts: # crates/rome_js_formatter/src/utils/mod.rs # crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/edge_case.js.snap # crates/rome_js_formatter/tests/specs/prettier/js/method-chain/bracket_0.js.snap # crates/rome_js_formatter/tests/specs/prettier/js/new-expression/with-member-expression.js.snap # crates/rome_js_formatter/tests/specs/prettier/js/template-literals/expressions.js.snap # crates/rome_js_formatter/tests/specs/prettier/js/template/graphql.js.snap # crates/rome_js_formatter/tests/specs/prettier/typescript/as/assignment.ts.snap

ematipico · 2022-06-17T14:01:04Z

I can definitely see that. And we should add variants while we implement other assignments. I already started implementing Chain and ChainTail

I noticed that to implement this variant we need to know layout outside format_with in format_assignment_like function. Because chain variants don't have outer group. Do you have any idea how to get this?
* [prettier/prettier@`877ae8e`/src/language-js/print/assignment.js#L63-L70](https://github.com/prettier/prettier/blob/877ae8ec16369dc0cf79ee36019bfdce40429eae/src/language-js/print/assignment.js#L63-L70)

That's because we assumed that each variant had a group, and we should not. I think the best implementation is:

fn write_assignment_like() -> FormatResult<()> {
	match layout {}
}

And each variant write!

denbezrukov · 2022-06-17T14:27:13Z

I can definitely see that. And we should add variants while we implement other assignments. I already started implementing Chain and ChainTail

I noticed that to implement this variant we need to know layout outside format_with in format_assignment_like function. Because chain variants don't have outer group. Do you have any idea how to get this?
* [prettier/prettier@`877ae8e`/src/language-js/print/assignment.js#L63-L70](https://github.com/prettier/prettier/blob/877ae8ec16369dc0cf79ee36019bfdce40429eae/src/language-js/print/assignment.js#L63-L70)
That's because we assumed that each variant had a group, and we should not. I think the best implementation is:
fn write_assignment_like() -> FormatResult<()> {
	match layout {}
}
And each variant write!

Yes, but unfortunately we need to call write_left to know if left part is short or not :(

ematipico · 2022-06-17T14:59:36Z

I solved like this:

        match self {
            JsAnyAssignmentLike::JsPropertyObjectMember(_) => {
                write!(f, [group_elements(&format_content)])
            }
            JsAnyAssignmentLike::JsAssignmentExpression(_) => {
                write!(f, [&format_content])
            }
        }

At the very end of the function

denbezrukov · 2022-06-17T15:16:02Z

I solved like this:

        match self {
            JsAnyAssignmentLike::JsPropertyObjectMember(_) => {
                write!(f, [group_elements(&format_content)])
            }
            JsAnyAssignmentLike::JsAssignmentExpression(_) => {
                write!(f, [&format_content])
            }
        }

At the very end of the function

Hmm, but there are cases then assignment expression has group in prettier. It seems that only chain layout variants should be without group. Is it possible to write in temporary buffer before wrapping in a group?

ematipico · 2022-06-17T15:28:57Z

Sorry, I guess I assumed too many things here 😅

I had to move all the code inside write_assignment_like inside the format function in order to make things work. This is actually way better:

the implementation needs to have access to the parent. In this example path is the parent node. This way, we can access to the parent without too many traverse.
most of the functions we have can be moved inside impl JsAnyAssignmentLike
the only small issue is is_break_after_operator because it's used somewhere else

ematipico · 2022-06-17T15:36:19Z

@denbezrukov Please check #2728

feat(rome_js_formatter): add JsAnyAssignmentLike node union for JsPro…

472d89b

…pertyObjectMember and JsAssignmentExpression

denbezrukov marked this pull request as draft June 11, 2022 19:15

denbezrukov changed the title ~~feat(rome_js_formatter): add JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression~~ feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression Jun 11, 2022

denbezrukov commented Jun 11, 2022

View reviewed changes

add checking for leading own line comment for break after operator la…

f4864e2

…yout

This comment was marked as resolved.

Sign in to view

update snapshots

51aadf6

denbezrukov commented Jun 12, 2022

View reviewed changes

denbezrukov and others added 5 commits June 13, 2022 21:27

remove unnecessary space token

de96ecb

Merge remote-tracking branch 'origin/main' into assigment-like

2f0dde4

update snapshots

f4481e8

Merge remote-tracking branch 'origin/main' into assigment-like

7be596c

# Conflicts: # crates/rome_js_formatter/tests/specs/prettier/js/assignment/lone-arg.js.snap

Merge remote-tracking branch 'origin/main' into assigment-like

c2bd3e5

denbezrukov marked this pull request as ready for review June 17, 2022 07:21

add spec snapshots

6dd43ba

MichaReiser requested a review from ematipico June 17, 2022 09:15

add spec snapshots

95cb8ef

denbezrukov added 2 commits June 17, 2022 13:13

copy has_new_line_before_comment functions and test spec cases from r…

710e47c

…ome#2721

add comments for assignment like layout

fcf0c38

ematipico reviewed Jun 17, 2022

View reviewed changes

ematipico mentioned this pull request Jun 17, 2022

feat(rome_js_formatter): apply existing layout strategy to assignments #2721

Closed

add comment for is_left_short

60dbefa

denbezrukov added 2 commits June 17, 2022 15:43

implement Format trait for JsAnyAssignmentLike

c7f823a

rename format_assignment_like -> write_assignment_like

bd6aa80

ematipico approved these changes Jun 17, 2022

View reviewed changes

denbezrukov added 3 commits June 17, 2022 16:10

update snapshots

f2869ae

update snapshots

9938fe7

ematipico merged commit 6cc5a3c into rome:main Jun 17, 2022

denbezrukov deleted the assigment-like branch June 17, 2022 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression #2698

feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression #2698

denbezrukov commented Jun 11, 2022 •

edited

denbezrukov Jun 11, 2022

ematipico Jun 17, 2022

denbezrukov commented Jun 11, 2022 •

edited

This comment was marked as resolved.

denbezrukov Jun 12, 2022 •

edited

ematipico commented Jun 13, 2022

denbezrukov commented Jun 17, 2022 •

edited

ematipico commented Jun 17, 2022 •

edited

denbezrukov commented Jun 17, 2022 •

edited

ematipico left a comment

ematipico Jun 17, 2022

ematipico commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

ematipico left a comment

ematipico commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

ematipico commented Jun 17, 2022 •

edited

denbezrukov commented Jun 17, 2022 •

edited

ematipico commented Jun 17, 2022

ematipico commented Jun 17, 2022

feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression #2698

feat(rome_js_formatter): JsAnyAssignmentLike node union for JsPropertyObjectMember and JsAssignmentExpression #2698

Conversation

denbezrukov commented Jun 11, 2022 • edited

Summary

Open questions

Test Plan

denbezrukov Jun 11, 2022

Choose a reason for hiding this comment

ematipico Jun 17, 2022

Choose a reason for hiding this comment

denbezrukov commented Jun 11, 2022 • edited

This comment was marked as resolved.

denbezrukov Jun 12, 2022 • edited

Choose a reason for hiding this comment

ematipico commented Jun 13, 2022

denbezrukov commented Jun 17, 2022 • edited

ematipico commented Jun 17, 2022 • edited

denbezrukov commented Jun 17, 2022 • edited

ematipico left a comment

Choose a reason for hiding this comment

ematipico Jun 17, 2022

Choose a reason for hiding this comment

ematipico commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

ematipico left a comment

Choose a reason for hiding this comment

ematipico commented Jun 17, 2022

denbezrukov commented Jun 17, 2022

ematipico commented Jun 17, 2022 • edited

denbezrukov commented Jun 17, 2022 • edited

ematipico commented Jun 17, 2022

ematipico commented Jun 17, 2022

denbezrukov commented Jun 11, 2022 •

edited

denbezrukov commented Jun 11, 2022 •

edited

denbezrukov Jun 12, 2022 •

edited

denbezrukov commented Jun 17, 2022 •

edited

ematipico commented Jun 17, 2022 •

edited

denbezrukov commented Jun 17, 2022 •

edited

ematipico commented Jun 17, 2022 •

edited

denbezrukov commented Jun 17, 2022 •

edited