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

Break long object literal members over two lines #2425

Closed
Tracked by #2403
MichaReiser opened this issue Apr 13, 2022 · 4 comments · Fixed by #2627
Closed
Tracked by #2403

Break long object literal members over two lines #2425

MichaReiser opened this issue Apr 13, 2022 · 4 comments · Fixed by #2627
Assignees
Labels
A-Formatter Area: formatter good first issue Good for newcomers I-Easy Implementation: easy task, usually a good fit for new contributors

Comments

@MichaReiser
Copy link
Contributor

MichaReiser commented Apr 13, 2022

Rome doesn't break object literal members over multiple lines even if they exceed the print width but Prettier breaks the initializer onto the new line Playground

Input

let a = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
   "can-someone-explain": that(),
 };
 
 // Doesn't apply for types :O
 type A = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": OrMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig,
 };

Prettier

let a = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line":
    orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
  "can-someone-explain": that(),
};

// Doesn't apply for types :O
type A = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": OrMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig;
};

Rome

let a = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
  "can-someone-explain": that(),
};

// Doesn't apply for types :O
type A = {
  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": OrMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig;
};

Expected

Rome's output to match Prettier's.

@MichaReiser MichaReiser changed the title Break long object literal and object literal type members Break long object literal members over two lines Apr 13, 2022
@MichaReiser MichaReiser added good first issue Good for newcomers A-Formatter Area: formatter labels Apr 13, 2022
@ematipico ematipico added the I-Easy Implementation: easy task, usually a good fit for new contributors label May 5, 2022
@denbezrukov
Copy link
Contributor

Hi,
I'd like to try to fix this issue. 🙏🏽

@denbezrukov
Copy link
Contributor

denbezrukov commented May 24, 2022

Hm,
My first guess was to try group_elements and soft_line_indent_or_space for FormatNodeRule<JsPropertyObjectMember> in property_object_member.rs file.

Ok(group_elements(formatted![
    formatter,
    [
        name.format(),
        colon_token.format(),
        soft_line_indent_or_space(formatted![formatter, [value.format()]]?),
    ]
]?))

Unfortunately, it changes formatting for any member value.
Prettier doesn't break all object literal member over two lines. Rome playground and Prettier playground.

It seems that prettier uses the same logic for object member and assignment (other cases?). I'm going to check all variants from JsAnyExpression. But I'm not sure that is the best options to solve the problem.

  1. Add soft_line_indent_or_space only for expressions which prettier breaks over two line in property_object_member.rs.
  2. Wrap individual expression in soft_block_indent from expressions directory.

@denbezrukov
Copy link
Contributor

denbezrukov commented May 26, 2022

It seems that we need to implement something like prettier does for property. It uses assignment which chooses layout for object member.

Am I on the right way?😬
Does Rome have this layout function/enum?
What solution is better for this issue? Implement layout function only for object member or for assignment too?

@ematipico
Copy link
Contributor

It uses assignment which chooses layout for object member.

Assignment layout is just an internal signal that they use to print arrow functions, it's not used inside their printer.

As you can see, each option create a different IR.

Am I on the right way?😬

I haven't looked my at their code unfortunately, so I am not sure. Even if the examples shows an assignment, the issue is about the members of the objects. Check this example, where I used the object inside a function. The formatting is the same.

What solution is better for this issue? Implement layout function only for object member or for assignment too?

I would focus, for now, on the object literal part and ignore the assignment. I believe we have another issue to track it. Do you think the issues are somehow related? If so, we can think about creating a function that will be used by both nodes. But for now let's stick to the object literal.

If you check Prettier's IR, they use group IDs to add the breaking logic to a group:

  group(indent(line), { id: Symbol.for("assignment #1") }),
  lineSuffixBoundary,
  indentIfBreak( ... )

We can achieve something similar using our internal function called if_group_with_id_fits_on_line and if_group_with_id_breaks

denbezrukov added a commit to denbezrukov/tools that referenced this issue May 30, 2022
denbezrukov added a commit to denbezrukov/tools that referenced this issue Jun 1, 2022
MichaReiser pushed a commit that referenced this issue Jun 6, 2022
…ines #2425 (#2627)

 This is the initial draft for issue "Break long object literal members over two lines".
 Prettier uses [`printAssigment`](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L25-L75) for formatting [AssignmentExpression](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L77), [VariableDeclarator](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L89), [ClassProperty](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/class.js#L195) and [Property](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/property.js#L92).  `printAssigment` can return different IR swhich are based on a layout variants from [`chooseLayout`](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L93) function.
 
-  `"break-after-operator"`
- `"never-break-after-operator"`
- `"fluid"`
- `"break-lhs"`
 - `"chain"`
 - `"chain-tail"`
  - `"chain-tail-arrow-chain"`
   - `"only-left"`

We're going to focus on [`chooseLayout`](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L93-L174) function and branches which are relevant for object literal members. 

- ["only-left"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L97-L99) branch isn't relevant for `JsPropertyObjectMember` because when object member has only left part it's `JsShorthandPropertyObjectMember`.  

- ["chain", "chain-tail-arrow-chain", "chain-tail"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L105-L121) these variants can be only if `shouldUseChainFormatting` is true. It seems that `shouldUseChainFormatting` is true only for match `isAssignmentOrVariableDeclarator`.

- ["break-after-operator"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L122-L129) I'm not sure about this conditional because technically it can be relevant for object member, but I looked prettier [commit](prettier/prettier@0a647a0) when this condition was added and it was for assignment.

- ["never-break-after-operator"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L131-L139) Prettier doesn't add break line if right node is require call. it works for object member. [Prettier playground](https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEEBGArTAXk2AB0pNMdqbbqlMoIYAlOARwFcBLAJzgAUASgA05SnUk4G-Ln0GjyAXwDcIESAgAHGN3TJQAQ168IAdwAKxhGmQhDAGzOGAnrY05ehsAGs4MAGVDAFs4ABluKDhkADNHNDgPL19-AK1vSIBzZBheTkSQOGCcOAATUrKwwyhMzkNMuAAxCF5gwxhdGrtDThgIdRAACxhghwB1Qe54NHSwOACbKe4ANymXOzA0dxBIhN4YCy9Mttj4grw0AA8ArIc4AEVOZmikOIcEjXTePbscQxKHAMtLxIjAxtxSjBBsgAEwABk+pgSYy8WjswLge2W0Q0XGeh20thQhjQAFoomUygNZDx+Id6idXmcNAlgtwcnkCmhbg8nvBTu8CjB-uDIdCkDCNLlDNwHFkAMIQYKMwpoACsA04CQAKv8iW8PiBlvkAJJQCqwAJgEE6ACC5oCMBcdwFCSUSiAA). I haven't found test for this case. 

- ["break-lhs"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L141-L148) this variant is for [destructuring](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L227), types ([alias](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L252), [annotation](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L278)) and [arrow function declaration](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L300).

- ["break-after-operator"](https://github.com/prettier/prettier/blob/feb6b519ee0e23eeb272227825ad84ecf1af05ab/src/language-js/print/assignment.js#L176-L223) It uses for right nodes which are:

    1.  `isBinaryish` and `!shouldInlineLogicalExpression`. I haven't added `shouldInlineLogicalExpression` checking but there is a difference in IR between [them](https://prettier.io/playground/#N4Igxg9gdgLgprEAuc0DOMAEBDTBeTYAHSkx2woBNqbbqKGlMYAnAVzkwDIvMAKADyUAlgDcA9AD4AlABoSZBtjorKS7E1YduvAIwAmAMzHj8qAF8QskBAAOMYemShsLFhADuABVcI0yEGwAGw9sAE9-awAjFmwwAGs4GABlbABbOAAZYSg4ZAAzYLQ4aNiEpOTbOJyAc2QtEpA4NKi4ajbM7CgatmwauAAxCBY07BgHboDsNhgIKxAACxg0oIB1BeF4NCqwOGS-TbFNsICwNEiQHOKWGC9YmtGCosaAKzQAD2TaoLgARTYIPAnkFitYqixrgEothWkF5rYWDkYKthJQYAtkAAWAAMYPcxVWsVsAQRcGuojy1gAjgD4Hc7P4UNg0ABaXJtNrzFhwGnCbl3PqPJCFEGNYppYT1dhi75-Wl5YXPawwGEotEYpD6ZWxYRBWoAYQgaSFTTQAFZ5mxigAVGGMkWgkCiDgASSglAQKTAiPsAEF3ckYGEfsDiuZzEA). 
    2. `StringLiteralTypeAnnotation`,  
    3. `SequenceExpression`
    4. `ConditionalExpression` + `isBinaryish` and `!shouldInlineLogicalExpression`.
    5. `StringLiteral`
    6. [isPoorlyBreakableMemberOrCallChain](https://github.com/prettier/prettier/blob/feb6b519ee0e23eeb272227825ad84ecf1af05ab/src/language-js/print/assignment.js#L329-L370)



- ["never-break-after-operator"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L162-L171) the variant is for `TemplateLiteral`, `TaggedTemplateExpression`, `BooleanLiteral`, `NumericLiteral`, `ClassExpression` or for a member which key is [short](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L412-L430).

- ["fluid"](https://github.com/prettier/prettier/blob/67c2df0ffe8c1e8bf1425fdd248ee5dd4eb5699f/src/language-js/print/assignment.js#L173) for other cases.
@MichaReiser MichaReiser moved this to Done in Rome 2022 Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Formatter Area: formatter good first issue Good for newcomers I-Easy Implementation: easy task, usually a good fit for new contributors
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants