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

Request: making verbose JSX ternaries opt in or opt out. #4139

Closed
drcmda opened this issue Mar 12, 2018 · 4 comments
Closed

Request: making verbose JSX ternaries opt in or opt out. #4139

drcmda opened this issue Mar 12, 2018 · 4 comments
Labels
lang:jsx Issues affecting JSX (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.

Comments

@drcmda
Copy link

drcmda commented Mar 12, 2018

I'd like to formally request making the change to how jsx ternaries are presented optional.

We thought it's a bug at first and after some browsing i found what looks like the cause: #2208

So this is a snapshot before

screen shot 2018-03-12 at 16 10 44

And that happens when prettier is through with it:

screen shot 2018-03-12 at 16 10 51

It was easy to read and glance over before and that's how prettier handles all other non-jsx ternaries. I sincerely think that vim being unable to refactor it is too weak a reason. It's actually the first time i can think of that prettier has produced code that's harder to read and looks machine formatted.

For us this is a major change that came out of the blue, prettier auto-formatting was switched off because it would cause heavy changes for small commits, and on a personal note, it's the weirdest feeling to format by hand again. 😞

@j-f1 j-f1 added type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity. lang:jsx Issues affecting JSX (not general JS issues) labels Mar 12, 2018
@lydell
Copy link
Member

lydell commented Mar 12, 2018

  • Most editors (not just Vim) has keyboard shortcuts for moving lines.
  • There are more reasons listed here: https://prettier.io/docs/en/rationale.html#jsx
  • Have you actually tried this for a while? To me, this looked really bad when I upgraded Prettier. Then I got used to it.

For us this is a major change that came out of the blue

Yes it is a somewhat major change. That's why we do these detailed change logs: https://prettier.io/blog/2017/08/29/1.6.0.html#jsx

If you just upgrade a package any change comes out of the blue, doesn't it?

@drcmda
Copy link
Author

drcmda commented Mar 12, 2018

I read through the reasons, and i fully understand that some people may want this. We have only recently updated prettier and it would cause massive formatting now with questionable benefit. As i said, this is the first time prettier formats like a machine - we kind of trusted it to not do that like all the previous formatters would.

As for the new format itself,

  1. it's sure subjective, but imo it looks noisy and unnecessary
  2. makes visual parsing the code harder
  3. conflicts with how prettier displays ternaries in general

The last one is perhaps the only reason to consider. If it's possible, a small flag would go a long way in making this a fine addition for vim users. I understand prettier doesn't take adding options lightly, but this is a deep cut for larger code-bases.

@Pyrolistical
Copy link

Pyrolistical commented May 2, 2018

This is the main blocker for me to use prettier.

The reason why I format ternaries likes this:

x
  ? a
  : b

is it makes it easier to read when it gets nested (which should be avoid in general first of all):

z
  ? x
    ? a
    : b
  : y
    ? c
    : d

compare the last example in #2208 (comment):

z ?
  x ?
    a
  :
    b
: y ?
    c
  :
    d

i don't even know how to strawman it properly lol

@suchipi
Copy link
Member

suchipi commented May 2, 2018

I'm going to close this issue since the current formatting is the result of significant community demand and it would be a regression to change it at this point.

@suchipi suchipi closed this as completed May 2, 2018
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 31, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:jsx Issues affecting JSX (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. 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

5 participants