Respect paren + newline

[ryanflorence]

Hi friends!

Been using prettier full time for a while now and it's prettier great. I'd like to request one change that I think makes refactoring less error prone: respect ( + newline the same way if { + newline is treated.

// prettier maintains the block
if (foo) {
  doStuff()
}

// but changes this:
if (foo)
  doStuff()

// to this:
if (foo) doStuff()

I think doing the same for multi-line expressions (which happen all the time in JSX) would be equally beneficial. Since using prettier, I am getting a lot more syntax errors as I refactor my code because it puts the code on the same lines as the syntax. JSX is the most affected part in my code since it's expression heavy, and also the most likely to be moved around as you build and refactor a UI.

Some examples

For example, consider this code:

const Slider = ({ isOpen, title, children }) => (
  <div>
    <h1>{title}</h1>
    {isOpen && (
      <div>
        {children}
      </div>
    )}
  </div>
);

Prettier changes it to:

const Slider = ({ isOpen, title, children }) =>
  <div>
    <h1>{title}</h1>
    {isOpen &&
      <div>
        {children}
      </div>}
  </div>;

If I am refactoring and move the <div/> somewhere else, maybe I'm inlining it into a different component, that semi-colon comes along with the div leading to syntax errors or in this case just a printed ; in the UI if you don't catch it.

Additionally, the closing } in the isOpen block gets put on the same line as the closing div, so refactoring that block is more likely to result in accidental syntax errors:

If parens were respected like if block curlies

However, if the parens were respected (just like if block's curlies) we can just move stuff around w/o spending extra time adjusting syntax.

Ternaries are also much easier to work with if the parens are respected:

As compared to not respecting them:

I am not proposing prettier insert parens and new lines but rather, do what it does today, except when a developer puts a paren + newline in, respect it the same way an if { is respected.

Thanks! 💙💙💙

[vjeux]

Fwiw, this is the way I move things around.

[ryanflorence]

@vjeux some people can't use a mouse. Also, respecting parens still allows you to do the same thing you are right now.

[mjackson]

I just wanted to chime in here and say that, for whatever it's worth, this is the single biggest thing keeping me from using prettier from day to day. If prettier would respect my use of ( + \n, I'd definitely use it.

[thelastnode]

+1!

This would also help with making collection.map(x => ...) more legible:

{things.map(
  thing =>
    somePredicate ? <div>Some stuff</div> : <div>Some other stuff</div>
)}

// vs

// closer to an if-else, easier to logically group
{things.map(thing =>
  somePredicate ? (
    <div>Some stuff</div>
  ) : (
    <div>Some other stuff</div>
  )
)}

I've also noticed that diffs are easier to read in this style, especially with GitHub intra-line highlighting.

[devinrhode2]

Why wouldn't prettier make the unilateral decision to have delimiters added or removed in certain situations? I thought the goal of prettier was to alleviate engineers from spending time on this sort of stuff. And maybe it's easier/cleaner/simpler to implement.

There are good reasons to stick to @ryanflorence's style in the examples he presents. Can anybody come up with counter examples where you'd really want the current style?

[suchipi]

I've generally only run into this issue when dealing with JSX, but that might just be because ternaries and wrapping parens are more common in JSX.

That said, it does feel like the "one last thing" that makes prettier feel not-quite-ready-yet, at least for projects using JSX.

[aaronjensen]

@ryanflorence does your atom vim emulation have the tag text object? dat is a nice way to move tags around in vim or vim emulated editors. At least in spacemacs, it doesn't work to vat and move the lines up/down like you do in your picture as it drags the curly along w/ it, but deleting it then pasting elsewhere works fine. Obviously breaking muscle memory is hard, I've seen you move text around in that way in talks and such, so I know you prefer it 😄

I personally prefer ensuring consistent (removal of unnecessary) paren usage, but I wouldn't be angry if prettier respected some of them in certain situations.

[lydell]

I like and use Prettier a lot, but when reading this I realized that I often mess up my }s and ;s in JSX: Sometimes leaving a ; behind in the output, and sometimes concentrating hard on matching up those brackets. Not sure what the best solution is, but there's definitely a problem.

[btnwtn]

This is especially painful with non-mouse based text editors like vim and emacs.

[taylorham]

This change would also have the added benefit of keeping clear and consistent indentation with closing parens/braces. When this is off my spidey-sense tingles and tells me I must have forgotten a closer somewhere.

Per @ryanflorence's example:

const Slider = ({ isOpen, title, children }) => 
  <div>
    <h1>{title}</h1>
    {isOpen &&
      <div>
        {children}
      </div>}
      {/* broken indentation */}
  </div>;
  // broken indentation

[EnoahNetzach]

Fwiw, even with a mouse it's still easier to select maintaining parens + \n: from the end of the opening paren to the last line, no need to be precise with starting or ending point, no extra parens copied.

[AlteredConstants]

For those unfamiliar, the first example is actually the result of a recent change which some people were not thrilled about, myself included. The idea seems to be to remove parens everywhere possible, consistently, but I'm unclear on the motivation for that. At least for me, this seems to ignore important semantics/conventions (e.g. Airbnb style) which this issue and @taylorham's "spidey-sense" are specific manifestations of.

I'd personally still like to see parens auto-included for all multi-line expressions (with possible, unconditional exceptions?) rather than conditionally maintaining them, but I would appreciate the consistency with curly-brace blocks in this proposal.

[ryanflorence]

@vjeux how do these things get decided? What does the "needs discussion" label mean? I have no idea if 50 👍's on the issue means anything, but I'm hopeful. I'm really excited about prettier with this one small adjustment.

[suchipi]

For semicolons, it seemed like the pattern was "people discussed it for ages, then someone made a PR that just added it, and it got merged"...

[skovhus]

To me Ryans suggestion will also make it easier to review changes, as the diff for moving a block around will not contain noise bracket modifications.

[ericclemmons]

It sounds like my issue #2180 is similar.

[vjeux]

Could someone try to implement parenthesis with ?: as displayed in the original post within prettier to see how it would look?

[suchipi]

@vjeux I haven't looked at prettier's codebase enough to know what things are hard and what are easy, but would this heuristic work?

When I'm about to print a JSX expression
If the expression is currently multiline
Wrap it in parentheses

or if that's too hard:

When I'm about to print a JSX expression
If the expression is not a self-closing tag
Wrap it in parentheses

This would cover JSX arrow expressions and JSX expressions in ternaries, which seem to be the most common case.

@ryanflorence is asking for "leave parens around if I put them there", but my understanding is that this is nontrivial to do in prettier (particularly because parens aren't a node in the AST). Is that correct?

This isn't the first time I wished Babylon had a nonstandard node for parens just so you could differentiate this type of situation. If it did, then implementing what Ryan is asking for would probably be what had happened by default.

[vjeux]

I don't think that the solution of putting parenthesis if the user inputted them or dropping them if not is great. It's going to introduce inconsistencies within a codebase and is just a crutch because we want to support two different opinions.

I've been quiet on this because I was focused on other things (TypeScript, CSS, GraphQL, CSS-in-JS, Website, Facebook rollout) but now that those are in order, it's time to look into parenthesis and JSX as it's the last thing that people are heavily in disagreement with the current way prettier is printing code.

I would like to investigate how hard it would be to implement the following style for JSX and what are the unforeseen consequences.

cond
  ? (
    <div>
      Text 1
    </div>
  ) : (
    <div>
      Text 2
    </div>
  )

Also I want to revisit dropping the parenthesis for arrow functions and figure in what other context JSX is being printed and if we need to do something about it.

[ryanflorence]

@vjeux a few "precendents" I see.

I don't think that the solution of putting parenthesis if the user inputted them or dropping them if not is great. It's going to introduce inconsistencies within a codebase

Prettier already allows for this type of inconsistency, it leaves both of these alone:

if (bool) {
  doStuff();
}

if (bool) doStuff();

But indeed, I'd be even happier if adding parens was the prettier opinion 😍

• Makes diffs easier to review
• Has nice "open/close" symmetry which leads to fewer refactoring syntax errors
• Removes the "broken indentation" look.
• Makes complex expressions look exactly like if else if else blocks with same number of lines, same position of lines, with a lot less syntax. A declarative if block, essentially.
• Has nice symmetry to component render method returns (that inserts parens if you leave them off)
• Gives editors some syntax to help you navigate around

I tried to dig in a little but I have very little experience with ASTs. I didn't really get anywhere and some other stuff is on my plate now.

I would like to investigate how hard it would be...

Are you doing this investigation or do we need to try to find somebody?

Also I want to revisit dropping the parenthesis for arrow functions

👍

I'm fine with this:

cond
  ? (
    <div>
      Text 1
    </div>
  ) : (
    <div>
      Text 2
    </div>
  )

But would rather save some indentation and get it symmetrical with an if/else block

cond ? (
  <div>
    Text 1
  </div>
) : (
  <div>
    Text 2
  </div>
)

And need to account for more complex ternaries:

cond ? (
  <div>
    Text 1
  </div>
) : cond2 ? (
  <div>
    Text 2
  </div>
) : (
  <div>
    Text 3
  </div>
)

That has perfect symmetry to if/else if/else.

cond ? (           if (cond) {
  <div/>             return <div/>
) : cond2 ? (      } else if (cond2) {
  <div/>             return <div/>
) : (              } else {
  <div/>             <div/>
)                  }

[suchipi]

For what it's worth, I would usually write this:

cond ? (
  <div>
    Text 1
  </div>
) : (
  <div>
    Text 2
  </div>
)

(cond on same line as as ?).

But I don't want to bikeshed too much.

Here's the places where I (personally) find myself wanting parens around JSX:

• Arrow function with no block body that returns JSX. Parens go around returned expression

const Something = ({ foo }) => (
  <div>
    {foo}
  </div>
);

• Arrow function with block body that returns JSX. Parens go around returned expression

const Something = ({ foo }) => {
  const calculatedFoo = foo + 2;
  return (
    <div>
      {calculatedFoo}
    </div>
  );
};

• Render method returning JSX. Parens go around returned expression

// (in class body)
render() {
  return (
    <div>
      {this.props.foo}
    </div>
  );
}

• Multiline JSX expression in a variable declaration made at some point. Parens go around the declarator's init

// (in class body)
render() {
  const formattedFoo = (
    <div className="bla">
      {foo}
    </div>
  );

  return (
    <div>
      {formattedFoo}
    </div>
  );
}

When using a ternary within JSX (even if the ternary's consequent or alternate are not JSX)

// in render method or functional component
const { isFoo, bar } = this.props;

return (
  <div>
    {isFoo ? (
      "Foo ()"
    ) : (
      <span>{bar}</span>
    )}
  </div>
);

When using a ternary in a React render method where the outermost expression is not JSX (and the ternary's consequent or alternate may or may not be JSX)

// in render method or functional component
const { isFoo, bar } = this.props;

return isFoo ? (
  "Foo ()"
) : (
  <span>{bar}</span>
);

That last one is hard to detect, I know...

There might be more edge cases but these are what I can think of off the top of my head right now.

[vjeux]

Are you doing this investigation or do we need to try to find somebody?

I'm heading to a ~2-week vacations in France, so hopefully trying to find somebody. But I'll be actively involved.

[vjeux]

The issue with

cond ? (
  <div>
    Text 1
  </div>
) : (
  <div>
    Text 2
  </div>
)

is that it's different from the way we print ternaries everywhere else inside of prettier. But that's worth exploring.

[suchipi]

I'd personally be fine with changing all ternary printing for the sake of this use case... but I recognize that change might carry some heavy weight, so it's probably not the right thing to do 😉

I might be able to take a stab at this (not at changing all ternaries- at the JSX examples described) on July 4th.

[lydell]

I agree with @suchipi in that it’s worth testing out always printing multiline ternaries the @ryanflorence way, regardless of whether there’s JSX inside or not. Might turn out really nice (or not, but we’ll see).

[EnoahNetzach]

I'd say not if one or both expressions could be inlined (various variations):

cond
  ? aaaaa
  : bbbbb

cond ? a : b

cond
  ? aaaaa
  : (
    <div>
      text
    </div>
  )

cond ? a : (
  <div>
    text
  </div>
)

cond
  ? (
    <div>
      text
    </div>
  ) : bbbb

etc..

Effectively treating ?, : as breakable before, but not after; and parens breakable after, but not before.

[suchipi]

I've made a PR that:

• Adds wrapping parens back to JSX Arrow functions (when the JSX is multiline):

// Before
const MyThing = () =>
  <div>
    <span>Hi</span>
  </div>;

// After
const MyThing = () => (
  <div>
    <span>Hi</span>
  </div>
);

• Uses different formatting for ?: when using JSX

// Before
const MyThing = () =>
  <div>
    {isGood
      ? <div>
          <span>"yeah it's good"</span>
        </div>
      : isOkay ? <div>it's okay</div> : null}
  </div>;

// After:
const MyThing = () => (
  <div>
    {isGood ? (
      <div>
        <span>
          "yeah it's good"
        </span>
      </div>
    ) : isOkay ? (
      <div>it's okay</div>
    ) : (
      null
    )}
  </div>
);

• Adds wrapping parens when JSX is used in a logical expression:

// Before:
<div> 
  {a || <span />} 
</div>;

// After:
<div>
  {a || (
    <span />
  )}
</div>;

[drcmda]

@vjeux

Is there a flag to switch this new formatting off? I thought it's a bug at first and landed here. I don't use vim and i have no problems refactoring. All i see is that suddenly it stretches code into something that i wouldn't call an improvement.

It was easy to read and glance over before, now it looks odd and noisy to me. It's actually the first time i can think of that prettier has produced code that's harder to read and looks machine translated.

For us this is a major change that came out of the blue, we had to switch off prettier auto-formatting and it's the weirdest feeling to format by hand again. 🙁

[rattrayalex]

@drcmda please move your comment to a new issue.

(FWIW, I expect that issue will be closed, but it's helpful to be able to track community reaction – if lots of people agree with you, we may consider).