Decorators with multiple arrow function arguments are broken into multiple lines

[shayded-exe]

Prettier 1.19.1
Playground link

--parser typescript
--print-width 100
--single-quote
--trailing-comma all

Input:

export class Item {
  @OneToOne(() => Thing, x => x.item)
  thing!: Thing;
}

Output:

export class Item {
  @OneToOne(
    () => Thing,
    x => x.item,
  )
  thing!: Thing;
}

Expected behavior:
Unchanged

export class Item {
  @OneToOne(() => Thing, x => x.item)
  thing!: Thing;
}

The new heuristic introduced in 1.9 to break up function chains for _.flow() or _.chain() is misinterpreting the decorator as one of those calls. Either we should have an option to change the line-breaking behavior, or decorators should be exempted from the heuristic.

[alexander-akait]

decorators should be exempted from the heuristic

I disagree, why do you think so?

For me current output is better and readable

[brainkim]

@PachowStudios Hi 👋, I’m the designated guy to yell at if you have issues with 1.19 call expressions. This is an expected knock-on effect of 1.19 change, where I think we agreed it’s usually good to place multiple function expressions on multiple lines.

Either we should have an option to change the line-breaking behavior

This is a tactic of last resort for prettier I think (https://prettier.io/docs/en/option-philosophy.html)

or decorators should be exempted from the heuristic

I see your point, having decorators split on multiple lines can make it less clear that what a decorator call is decorating.

I’m going back and looking through the issue, and I think I just decided to break call expressions with multiple function literals in them, because the main issue was about function literals sorta adding noise to single-line expressions, but maybe we can revert this behavior? As it turns out I also have code which was changed from:

    this.execution = execution.then(() => undefined, () => undefined);

to

    this.execution = execution.then(
      () => undefined,
      () => undefined,
    );

I can live with both versions and don‘t think it’s a big deal, but maybe this decorator call example is reason enough to revert to the previous behavior (just for function literals at the top level of the call expression, not for operator-like calls ( i.e. pipe(map((x) => x + 1), filter((x) => x % 2 === 0)) should still be split).

I do think that even in your code, the placement of function literals on the same line adds a bit of syntactical noise/overhead, and the line-break is appreciated, but I also see your point. I dunno.

[shayded-exe]

@brainkim I appreciate the detailed response. I agree after looking at it for a bit, that my original example does benefit from being split. However, if you were to remove the third arg, then it definitely doesn't make sense to be split onto multiple lines. I've updated my original post to reflect this (your own example illustrates this as well).

I think your proposed solution of splitting the line when the arrow functions are one level nested in the expression seems like a good middle-ground.

[thorn0]

Sounds like two top-level arrow functions are readable without breaking the line, but if we have more of them, it's better to break. On the other hand, the arrow functions in your examples are very simple: x => x.item, () => undefined. But what if the return value contains a call?

[brainkim]

I think, as the complexity of the functions increase (more arguments, more complex return value), splitting functions onto multiple lines becomes more valuable. But the only heuristic I’m comfortable with are ones which consider function literal count + line-length. I don’t know if refining it to read the complexity of the functions or that the call expression is a decorator call is a good idea. I’d much rather the behavior was consistent instead to prevent surprises.

Maybe we can tweak the title to be about function literals and call splits but leave this behavior open for a vote before deciding.

[thorn0]

I still think setting apart zero-argument and single-argument functions with one-word (whatever it means) return values might be an idea worth exploring. But better to collect more feedback with code samples first.

[JClackett]

Hmmm, since 1.19.2 my TypeOrm entities have become very bloated as result of the change... definitely prefer before, line count basically doubles all over the place and i find it much harder to read/follow otherwise i'd have to put lots of whitespace or comments between to break it up more,

I feel like decorator params or similar should be treated differently to the examples stated above, but that's just me 🤷‍♂

BEFORE

  @ManyToOne(() => User, user => user.deals)
  manager: User

  @OneToMany(() => Investment, investment => investment.deal)
  investments: Investment[]

  @OneToMany(() => Approval, approval => approval.deal, { onDelete: "CASCADE" })
  approvals: Approval[]

  @OneToMany(() => DealShare, dealShare => dealShare.deal, {
    onDelete: "CASCADE",
  })
  dealShares: DealShare[]

  @OneToMany( () => PopularDeal, deal => deal.platform, { onDelete: "CASCADE"})
  popularDeals: PopularDeal[]

AFTER

Everything is so narrow!? so much more room left on the line 😢

  @ManyToOne(
    () => User,
    user => user.deals,
  )
  manager: User

  @OneToMany(
    () => Investment,
    investment => investment.deal,
  )
  investments: Investment[]

  @OneToMany(
    () => Approval,
    approval => approval.deal,
    { onDelete: "CASCADE" },
  )
  approvals: Approval[]

  @OneToMany(
    () => DealShare,
    dealShare => dealShare.deal,
    {
      onDelete: "CASCADE",
    },
  )
  dealShares: DealShare[]

  @OneToMany(
    () => PopularDeal,
    deal => deal.platform,
    {
      onDelete: "CASCADE",
    },
  )
  popularDeals: PopularDeal[]

[brainkim]

After looking at these examples, I think there’s sufficient motivation to revert the line-splitting behavior for top-level function literals. It was sorta tacked onto the functional composition logic anyways. I’ll try to do it this week. (Unless one of the maintainers disagrees that the second example is way worse)

[brainkim]

These are the tests from the test suite which fail with a simple revert. Now I remember why we decided to add this top-level function logic, because it broke fewer tests, but the tests which break I’m either agnostic about, or they’re completely contrived examples which tested some edge case at some point. One possibility is that we could just disable the function composition logic for decorators, but part me thinks it might be better to just get rid of the logic for top-level function literals entirely? I dunno. Conflicted.

Summary of all failing tests
FAIL tests/decorators-ts/jsfmt.spec.js
  ● typeorm.ts

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "typeorm.ts 1".

    - Snapshot
    + Received

    @@ -42,13 +42,10 @@
        theme: string;

        @Column()
        description: string;

    -   @OneToMany(
    -     type => Topic,
    -     topic => topic.board
    -   )
    +   @OneToMany(type => Topic, topic => topic.board)
        topics: Topic[];
      }

      ================================================================================

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {

      at Object.test (tests_config/run_spec.js:90:9)

FAIL tests/typescript_arrow/jsfmt.spec.js
  ● arrow_regression.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "arrow_regression.js 1".

    - Snapshot
    + Received

    @@ -22,16 +22,11 @@
      =====================================output=====================================
      const bar = (...varargs: any[]) => {
        console.log(varargs);
      };

    - const foo = (x: string): void =>
    -   bar(
    -     x,
    -     () => {},
    -     () => {}
    -   );
    + const foo = (x: string): void => bar(x, () => {}, () => {});

      app.get("/", (req, res): void => {
        res.send("Hello world");
      });

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {


      at Object.test (tests_config/run_spec.js:90:9)

  ● arrow_regression.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "arrow_regression.js 2".

    - Snapshot
    + Received

    @@ -23,16 +23,11 @@
      =====================================output=====================================
      const bar = (...varargs: any[]) => {
        console.log(varargs)
      }

    - const foo = (x: string): void =>
    -   bar(
    -     x,
    -     () => {},
    -     () => {}
    -   )
    + const foo = (x: string): void => bar(x, () => {}, () => {})

      app.get("/", (req, res): void => {
        res.send("Hello world")
      })

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {


      at Object.test (tests_config/run_spec.js:90:9)

FAIL tests/functional_composition/jsfmt.spec.js
  ● lodash_flow.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "lodash_flow.js 1".

    - Snapshot
    + Received

    @@ -24,24 +24,16 @@
      bar(6);

      =====================================output=====================================
      import { flow } from "lodash";

    - const foo = flow(
    -   x => x + 1,
    -   x => x * 3,
    -   x => x - 6
    - );
    + const foo = flow(x => x + 1, x => x * 3, x => x - 6);

      foo(6);

      import * as _ from "lodash";

    - const bar = _.flow(
    -   x => x + 1,
    -   x => x * 3,
    -   x => x - 6
    - );
    + const bar = _.flow(x => x + 1, x => x * 3, x => x - 6);

      bar(6);

      ================================================================================

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {

      at Object.test (tests_config/run_spec.js:90:9)

  ● lodash_flow_right.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "lodash_flow_right.js 1".

    - Snapshot
    + Received

    @@ -24,24 +24,16 @@
      bar(6);

      =====================================output=====================================
      import { flowRight } from "lodash";

    - const foo = flowRight(
    -   x => x + 1,
    -   x => x * 3,
    -   x => x - 6
    - );
    + const foo = flowRight(x => x + 1, x => x * 3, x => x - 6);

      foo(6);

      import * as _ from "lodash";

    - const bar = _.flowRight(
    -   x => x + 1,
    -   x => x * 3,
    -   x => x - 6
    - );
    + const bar = _.flowRight(x => x + 1, x => x * 3, x => x - 6);

      bar(6);

      ================================================================================

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {

      at Object.test (tests_config/run_spec.js:90:9)

FAIL tests/arrows/jsfmt.spec.js
  ● parens.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "parens.js 1".

    - Snapshot
    + Received

    @@ -19,14 +19,11 @@
      foo(c, a => b)
      foo(c, a => b, d)
      foo(a => b, d)

      =====================================output=====================================
    - promise.then(
    -   result => result,
    -   err => err
    - );
    + promise.then(result => result, err => err);

      promise.then(
        result => {
          f();
          return result;

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {

      at Object.test (tests_config/run_spec.js:90:9)

  ● parens.js

    expect(value).toMatchSnapshot()

    Received value does not match stored snapshot "parens.js 2".

    - Snapshot
    + Received

    @@ -19,14 +19,11 @@
      foo(c, a => b)
      foo(c, a => b, d)
      foo(a => b, d)

      =====================================output=====================================
    - promise.then(
    -   (result) => result,
    -   (err) => err
    - );
    + promise.then((result) => result, (err) => err);

      promise.then(
        (result) => {
          f();
          return result;

      88 |           )
      89 |         )
    > 90 |       ).toMatchSnapshot();
         |         ^
      91 |     });
      92 |
      93 |     for (const parser of parsers.slice(1)) {

      at Object.test (tests_config/run_spec.js:90:9)


Snapshot Summary
 › 7 snapshots failed from 4 test suites. Inspect your code changes or run `yarn test -u` to update them.

Test Suites: 4 failed, 945 passed, 949 total
Tests:       7 failed, 6216 passed, 6223 total
Snapshots:   7 failed, 4848 passed, 4855 total
Time:        188.089s
Ran all test suites.
error Command failed with exit code 1.

What do y’all think? Also if you’d like to play with this yourself you can just do this haha:

diff --git a/src/language-js/utils.js b/src/language-js/utils.js
index 6fb301d5..25c8e5ca 100644
--- a/src/language-js/utils.js
+++ b/src/language-js/utils.js
@@ -849,21 +849,17 @@ function isFunctionCompositionArgs(args) {
   if (args.length <= 1) {
     return false;
   }
-  let count = 0;
+
   for (const arg of args) {
-    if (isFunctionOrArrowExpression(arg)) {
-      count += 1;
-      if (count > 1) {
-        return true;
-      }
-    } else if (isCallOrOptionalCallExpression(arg)) {
+    if (isCallOrOptionalCallExpression(arg)) {
       for (const childArg of arg.arguments) {
         if (isFunctionOrArrowExpression(childArg)) {
           return true;
         }
       }
     }
-  }
+  k
+
   return false;
 }

[shayded-exe]

@brainkim the functionality is useful for it's intended purpose. My vote is to just disable it for decorators.