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

No way to format ternary JSX expressions? #1564

Closed
s100 opened this issue Oct 26, 2020 · 10 comments
Closed

No way to format ternary JSX expressions? #1564

s100 opened this issue Oct 26, 2020 · 10 comments
Milestone

Comments

@s100
Copy link

s100 commented Oct 26, 2020

What version of this package are you using?

standard@15.0.0 with eslint@7.11.0

What operating system, Node.js, and npm version?

Windows 10, Node.js 10.17.0, npm 6.11.3

What happened?

Here's my source file:

import * as React from 'react'

export const y = new Date().getTime() % 2
  ? (
    <span>
      ghi
    </span>
  )
  : 'jkl'

Standard gives me:

test.js:5:1: Expected indentation of 6 spaces but found 4. (indent)
test.js:5:11: Expected indentation of 6 space characters but found 4. (react/jsx-indent)
test.js:7:1: Expected indentation of 6 spaces but found 4. (indent)
test.js:8:1: Expected indentation of 4 spaces but found 2. (indent)

OK so it looks like the second expression in a ternary needs a little extra indentation. I'll fix my test file:

import * as React from 'react'

export const y = new Date().getTime() % 2
  ? (
      <span>
        ghi
      </span>
    )
  : 'jkl'

Standard now gives me:

test.js:5:7: Expected indentation of 4 space characters but found 6. (react/jsx-indent)

If I use --fix, Standard changes my code to look like this:

import * as React from 'react'

export const y = new Date().getTime() % 2
  ? (
      <span>
      ghi
      </span>
    )
  : 'jkl'

which doesn't look right, and, Standard still gives me errors:

test.js:5:7: Expected indentation of 4 space characters but found 6. (react/jsx-indent)
test.js:5:13: Expected indentation of 8 space characters but found 6. (react/jsx-indent)

What did you expect to happen?

At least one of these code samples should be considered correct by Standard. Or, if there's an alternate correct formatting which I'm missing, I would be interested to know what it is.

Are you willing to submit a pull request to fix this bug?

No. I wouldn't know where to start.

@feross feross added this to the standard 16 milestone Oct 28, 2020
@feross
Copy link
Member

feross commented Oct 28, 2020

Agreed – this is a bug. Working on a fix.

@feross
Copy link
Member

feross commented Oct 28, 2020

Fixed in 15.0.1.

@feross feross closed this as completed Oct 28, 2020
@feross
Copy link
Member

feross commented Oct 28, 2020

Duplicate of standard/eslint-config-standard#177

@s100
Copy link
Author

s100 commented Oct 28, 2020

With standard@15.0.1, I find that this is considered correct:

import * as React from 'react'

export const y = new Date().getTime() % 2
  ? (
    <span>
      ghi
    </span>
    )
  : 'jkl'

Thank you for the prompt fix! We are now unblocked. 🚀 (Although, subjectively, I think this formatting looks rather strange.)

@feross
Copy link
Member

feross commented Oct 28, 2020

@s100 Great! What formatting would you prefer?

@s100
Copy link
Author

s100 commented Oct 29, 2020

Thanks for asking! My preferred formatting would be:

export const y = new Date().getTime() % 2
  ? (
    <span>
      ghi
    </span>
  )
  : 'jkl'

The reason I prefer this is:

  1. The indentation of the line where the ) appears is the same as the indentation of the line where ( originally appears (2 spaces).
  2. Every line between the ( and ) is indented by 2 spaces more than the lines where the ( and ) appear (i.e. 4 spaces).

This is consistent with other bracketed forms in Standard:

if (condition) {
  z = [
    true,
    false
  ]
} else {
  z = 7
}

See here how, likewise:

  1. The indentation of the line where } appears is the same as the line where { appears in all cases (0 spaces)
  2. The indentation of the line where ] appears is the same as the line where [ appears (2 spaces).
  3. The indentation of the line where ) appears is the same as the line where ( appears (0 spaces - it's the same line!)
  4. Every line between { and } is indented by 2 spaces more than the lines where the { and } appear (i.e. 2 spaces)
  5. Every line between [ and ] is indented by 2 spaces more than the lines where the [ and ] appear (i.e. 4 spaces)
  6. Every line between ( and ) is indented by 2 spaces more than the lines where the ( and ) appear (but there are no such lines!)

@feross
Copy link
Member

feross commented Nov 10, 2020

@s100 Right now the reason that it's formatted this way is because of the offsetTernaryExpressions rule which we enabled in standard 15. See: #927

I think that ideally, we'd make an exception to this rule when an expression wrapped in parens appears as the value in the ternary expression, as it does in your example. In that case, I think indenting as you have done makes the most sense.

Unfortunately, there's no eslint rule for this case. If you're keen on this, can you open an issue over at ESLint at explain the issue. If they add it, then I'm happy to fix up standard to format code as you've indicated.

@sheerun
Copy link

sheerun commented Nov 10, 2020

To be honest I think the best solution is to leave in standard config just linting rules, and use prettierx for formatting in standard, just like my prettier-standard package... otherwise it'll be never ending story of slight whitespace mismatches https://github.com/sheerun/prettier-standard

@sheerun
Copy link

sheerun commented Dec 28, 2020

btw. This probably should read:

export const y = new Date().getTime() % 2
  ? (
      <span>
        ghi
      </span>
    )
  : 'jkl'

but parens are not really necessary:

export const y = new Date().getTime() % 2
  ? <span>
      ghi
    </span>
  : 'jkl'

@s100
Copy link
Author

s100 commented Jan 4, 2021

@sheerun Hah, I entirely agree that the parentheses should be unnecessary but that's been impossible since at least standard@14:

import * as React from 'react'

export const y = new Date().getTime() % 2
  ? <span>
      ghi
    </span>
  : 'jkl'
test.js:6:1: Expected indentation of 2 spaces but found 4.
test.js:6:5: Expected indentation of 2 space characters but found 4.
import * as React from 'react'

export const y = new Date().getTime() % 2
  ? <span>
    ghi
    </span>
  : 'jkl'
test.js:6:1: Expected indentation of 2 spaces but found 4.
test.js:6:5: Expected indentation of 2 space characters but found 4.
import * as React from 'react'

export const y = new Date().getTime() % 2
  ? <span>
    ghi
  </span>
  : 'jkl'
test.js:6:3: Expected closing tag to match indentation of opening.

We added parentheses everywhere because there was no other way we could figure out to keep Standard happy. Starting from standard@15 even this workaround stopped working, as we see above, hence this issue. And in standard@16, we get:

Missing parentheses around multilines JSX

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants