Break multiline parenthesized logical expressions

[alex12058]

Description

This PR adds line breaks to multiline parenthesized logical expressions. I'm not 100% sure the changes in this PR are the best way to add line breaks to logical expressions as it imports needsParams into src/language-js/print/binaryish.js so it can check if the logcial expression will have params added.

// Input
const funnelSnapshotCard =
  (report === MY_OVERVIEW && !ReportGK.xar_metrics_active_capitol_v2) ||
  (
    report === COMPANY_OVERVIEW &&
    !ReportGK.xar_metrics_active_capitol_v2_company_metrics
  ) ? (
    <ReportMetricsFunnelSnapshotCard metrics={metrics} />
  ) : null;

// Prettier stable
const funnelSnapshotCard =
  (report === MY_OVERVIEW && !ReportGK.xar_metrics_active_capitol_v2) ||
  (report === COMPANY_OVERVIEW &&
    !ReportGK.xar_metrics_active_capitol_v2_company_metrics) ? (
    <ReportMetricsFunnelSnapshotCard metrics={metrics} />
  ) : null;

// This PR
const funnelSnapshotCard =
  (report === MY_OVERVIEW && !ReportGK.xar_metrics_active_capitol_v2) ||
  (
    report === COMPANY_OVERVIEW &&
    !ReportGK.xar_metrics_active_capitol_v2_company_metrics
  ) ? (
    <ReportMetricsFunnelSnapshotCard metrics={metrics} />
  ) : null;

Checklist

• I’ve added tests to confirm my change works.
• (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
• (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
• I’ve read the contributing guidelines.

✨Try the playground for this PR✨

Relates to #9574. Breaks and indents nested conditions if they are multiline.

[fisker]

I like this,

I havn't seen any tests on ?? operator, can you add some?

[alex12058]

Sure. I added some basic tests in tests/format/js/logical_expressions/indent_multiline.js that includes tests for the ?? operator.

[fisker]

LGTM

[fisker]

Wow, this prints this real-world case exactly as I expect. https://github.com/prettier/prettier/blob/main/src/language-js/print/angular.js#L68-L77

Prettier pr-11103
Playground link

--parser babel

Input:

{{
      const shouldNotPrintColon =
        isNgForOf(node, index, parentNode) ||
        (((index === 1 &&
          (node.key.name === "then" || node.key.name === "else")) ||
          (index === 2 &&
            node.key.name === "else" &&
            parentNode.body[index - 1].type ===
              "NGMicrosyntaxKeyedExpression" &&
            parentNode.body[index - 1].key.name === "then")) &&
          parentNode.body[0].type === "NGMicrosyntaxExpression");
}}

Output:

{
  {
    const shouldNotPrintColon =
      isNgForOf(node, index, parentNode) ||
      (
        (
          (
            index === 1 &&
            (node.key.name === "then" || node.key.name === "else")
          ) ||
          (
            index === 2 &&
            node.key.name === "else" &&
            parentNode.body[index - 1].type ===
              "NGMicrosyntaxKeyedExpression" &&
            parentNode.body[index - 1].key.name === "then"
          )
        ) &&
        parentNode.body[0].type === "NGMicrosyntaxExpression"
      );
  }
}

[fisker]

@thorn0 Can you review?

[fisker]

This one is so ugly, but out scope this PR

const extraRendererAttrs = (
    (
      attrs.rendererAttrs &&
      this.utils.safeParseJsonString(attrs.rendererAttrs)
    ) ||
    Object.create(null)
  ) as FieldService.RendererAttributes;

should be expected.

[thorn0]

@fisker This change leads to big diffs. Ask the rest of the team about it.

[kachkaev]

Could be OK in a new major version, especially together with #3806. Something like:

{
  {
    const shouldNotPrintColon =
      isNgForOf(node, index, parentNode)
      || (
          (
            (
              index === 1
              && (node.key.name === "then" || node.key.name === "else")
            )
            || (
                index === 2
                && node.key.name === "else"
                && parentNode.body[index - 1].type ===
                     "NGMicrosyntaxKeyedExpression"
                && parentNode.body[index - 1].key.name === "then"
              )
            )
          && parentNode.body[0].type === "NGMicrosyntaxExpression"
        );
  }
}

If these changes are combined, there’ll be only one instance of a large diff.

[fisker]

Big 👍 for this change.

[kachkaev]

This PR is against main branch, I guess it needs to be repointed to next.

[fisker]

It's fine to merge into main, we do format changes in minor releases. (We probably won't release 2.x though)

[thorn0]

The current formatting is okay as long as there is no lines starting with two or more parens. But #11103 (comment) is really a good example of how unreadable it can be. I wonder if it's possible to change only cases like that one. In the diffs I see a lot of expressions for which the current formatting is totally okay and this change doesn't look like an improvement. E.g.:

     if (
       hasDeselectedButton ||
-      (this.state.elementType !== "selection" &&
-        this.state.elementType !== "text")
+      (
+        this.state.elementType !== "selection" &&
+        this.state.elementType !== "text"
+      )
     ) {
       return;
     }

It'd be great to find a way not to touch simple expressions like that and use the proposed expanded formatting only where it's really needed.

[rattrayalex]

I haven't looked into this PR in depth, but in case it's useful, I wrote a simple algorithm to gauge expression complexity in another branch: https://github.com/rattrayalex/prettier/blob/postfix-ternaries/src/language-js/utils/index.js#L622-L647