Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jsx-wrap-multilines #1009

Closed
zorzysty opened this issue Mar 15, 2017 · 6 comments
Closed

jsx-wrap-multilines #1009

zorzysty opened this issue Mar 15, 2017 · 6 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.

Comments

@zorzysty
Copy link

zorzysty commented Mar 15, 2017

In it's current version prettier is conflicting with jsx-wrap-multilines rule that is used by some of the most popular react style guides including Airbnb's.

const header = this.props.header
  ? (
	<HeaderElement
	  className="myclass myclass2"
	  isExpanded={this.state.isExpanded}
	>
	  {this.props.header}
	</HeaderElement>
  )
  : null;

outputs

const header = this.props.header
  ? <HeaderElement
      className="myclass myclass2"
      isExpanded={this.state.isExpanded}
    >
      {this.props.header}
    </HeaderElement>
  : null;

https://prettier.github.io/prettier/#%7B%22content%22%3A%22const%20header%20%3D%20this.props.header%5Cn%20%20%3F%20(%5Cn%5Ct%3CHeaderElement%5Cn%5Ct%20%20className%3D%5C%22myclass%20myclass2%5C%22%5Cn%5Ct%20%20isExpanded%3D%7Bthis.state.isExpanded%7D%5Cn%5Ct%3E%5Cn%5Ct%20%20%7Bthis.props.header%7D%5Cn%5Ct%3C%2FHeaderElement%3E%5Cn%20%20)%5Cn%20%20%3A%20null%3B%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3A%22none%22%2C%22bracketSpacing%22%3Atrue%2C%22jsxBracketSameLine%22%3Afalse%2C%22parser%22%3A%22babylon%22%2C%22doc%22%3Afalse%7D%7D

@rattrayalex rattrayalex added status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. labels Mar 15, 2017
@vjeux
Copy link
Contributor

vjeux commented Mar 15, 2017

The reason why () are added is to prevent

return
  <div />;

from the semi-colon automatic insertion feature of js where it adds a ; after return. Then, because people see parenthesis added for return, they are thinking it's actually useful/needed for other places, but it's not the case.

It seems like adding parenthesis around ternaries is just adding noise and not actually useful.

@zorzysty
Copy link
Author

zorzysty commented Mar 16, 2017

@vjeux I agree with you. Those parentheses aren't useful in ternaries and prettier shouldn't be adding them by default.

The option would be nice to have since this rule is so popular, and its default settings are requiring parentheses for both returns and ternaries like in the example.

As a workaround, for those who are using Airbnb style guide and want to integrate prettier, I'd recommend adding this line to the rules:
"react/jsx-wrap-multilines": ["error", {"declaration": false, "assignment": false}],

@vjeux
Copy link
Contributor

vjeux commented May 21, 2017

I'm going to close this one in order to concentrate all the ternary discussions in #737

@vjeux vjeux closed this as completed May 21, 2017
@tunnckoCore
Copy link

@vjeux it's not only about ternaries..

following

const foo = ({ bar }) => <h1>Hello {bar}</h1>

is converted to

const foo = ({ bar }) =>
  (<h1>
    Hello {bar}
  </h1>)

current config of that rule

module.exports = {
  rules: {
    'react/jsx-wrap-multilines': [
      'error',
      { arrow: true, return: true, declaration: true }
    ]
  }
}

@vjeux
Copy link
Contributor

vjeux commented Jul 28, 2017

@charlike prettier has never outputted that, it must be an eslint rule with autofix

@tunnckoCore
Copy link

Hm, probably, after couple of more tests it feels like that, because in some cases it work. Sorry ✋

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

No branches or pull requests

4 participants