tools icon indicating copy to clipboard operation
tools copied to clipboard

feat(rome_js_formatter): Binary expression formatting

Open MichaReiser opened this issue 3 years ago • 1 comments
trafficstars

Summary

Part of #3046

This PR refactors our JsAnyBinaryLikeExpression formatting to closer match prettier's formatting. The main change is that our old implementation used to group the operator with the left hand side:

a && b && c

became

group("a &&", [group("b &&", c)])

whereas prettier (and this PR) always groups the operator with the right-hand side

"a", group("&& b", group(" && c"))

This PR further enables the NeedsParentheses implementation for binary expressions.

Unsupported case

The new implementation doesn't support the issue-7024 test case

const radioSelectedAttr =
  (isAnyValueSelected &&
    node.getAttribute(radioAttr.toLowerCase()) === radioValue) ||
  ((!isAnyValueSelected && values[a].default === true) || a === 0);

The issue is that removing the parentheses from ((!isAnyValueSelected && values[a].default === true) || a === 0) rebalances the tree. Prettier solves this problem by rebalancing logical expressions ahead of formatting. This is something Rome doesn't support today which is why I disabled parentheses removal from logical expression if the child and parent have the same operator but the child is the right side expression.

Test Plan

I manually reviewed the snapshot changes.

Average compatibility: 81.64 -> 82.59 Compatible lines: 78.42 -> 79.70

MichaReiser avatar Aug 17 '22 16:08 MichaReiser

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: db28af0
Status: ✅  Deploy successful!
Preview URL: https://dc29ad6a.tools-8rn.pages.dev
Branch Preview URL: https://feat-binary-expressions.tools-8rn.pages.dev

View logs

What does "re-balance a tree" mean?

ematipico avatar Aug 18 '22 07:08 ematipico

What does "re-balance a tree" mean?

To change the tree's shape without changing the semantic meaning. In this particular case, it means that a tree that has nested logical expressions in the right side to the left side so that the right side.

  &&
a    &&
   b    c 

// becomes

    && 
 &&    c
a    b

The transformed version works well with our flattening logic where it will miss the former completely.

https://github.com/prettier/prettier/blob/d2bcaea121d244a00100e5c31e2e8ca9fe5f1ced/src/language-js/parse/postprocess/index.js#L207-L225

MichaReiser avatar Aug 18 '22 07:08 MichaReiser