Eslint-plugin-react: [jsx-closing-tag-location]: Add line-aligned option similar to jsx-closing-bracket-location

[NiGhTTraX]

Current behavior

This warns:

return <div>
  foobar
</div>;

This doesn't:

return (
  <div>
    foobar
  </div>
);

The rule is checking that the column of the closing tag is equal to the column of the opening tag:

https://github.com/yannickcr/eslint-plugin-react/blob/a120758e25f04cdc37d2d3d80922f2b998ab63ed/lib/rules/jsx-closing-tag-location.js#L52-L59

Expected behavior

Ideally, this rule would accept an option line-aligned like jsx-closing-bracket-location that would enforce the closing tag to be aligned with the line containing the opening line.

I'm assuming the code would be similar to:

https://github.com/yannickcr/eslint-plugin-react/blob/a120758e25f04cdc37d2d3d80922f2b998ab63ed/lib/rules/jsx-closing-bracket-location.js#L115-L126

[ljharb]

I think I understand what you're asking for, but not "why". Certainly this non-JSX is valid:

return [
  foo,
];

but in that case, the array is part of javascript. with jsx, an opening tag enters you into an _entirely distinct parsing context_, and it's incredibly important to denote that by giving it a fresh indentation context.

Since we have "props-aligned", "line-aligned" might indeed make sense to add, but I really don't understand the benefit of this style.

[NiGhTTraX]

Sorry @ljharb, I'm not sure I understand your example. Maybe I wasn't clear enough in my issue. I want the following to be valid indentation:

render() {
  return <div>
    foobar
  </div>;
}

The rule wants this instead:

render() {
  return <div>
    foobar
         </div>;
}

[ljharb]

Right; your first example is what I'm suggesting isn't a good practice - but I do understand that's what you want.

Your second example, however, seems like a clear bug. If the setting is "tag-aligned", then "foobar" should be indented like so:

render() {
  return <div>
           foobar
         </div>;
}

or like this:

render() {
  return (
    <div>
      foobar
    </div>
  );
}

"line-aligned" would then be like this:

render() {
  return <div>
    foobar
  </div>;
}

I think it makes sense for closing-tag-location to have the same options as closing-bracket-location, despite my personal opinion on "line-aligned".

[eddiemonge]

Maybe you should just do JSX like this rule instead? https://github.com/yannickcr/eslint-plugin-react/blob/HEAD/docs/rules/jsx-wrap-multilines.md

[brammitch]

Another use case for "line-aligned":

We use the Operator-Linebreak with "Before" option to make ternary statements easier to read.

Without the "line-aligned" option, this generates a warning:

{this.props.isLoading
  ? <ProgressBar>
  : <div>
    foo
  </div>
}

[ljharb]

@brammitch

{this.props.isLoading
  ? <ProgressBar>
  : (
    <div>
      foo
    </div>
  )
}

or

{this.props.isLoading ? <ProgressBar> : (
  <div>
    foo
  </div>
)}

but if you don't wrap multiline jsx in parens for some reason, then i can see your use case applying.

[brammitch]

@ljharb
We don't wrap multiline JSX in parens.

However, after looking at Airbnb's style guide, it seems like we should change our rules to use jsx-wrap-multilines instead of making an exception for line-aligned closing tags.

[mrchief]

Here's a use case:

  test('renders default state correctly', () => {
    const tree = renderer.create(<Provider store={mockStore()}>
      <JobForm />
    </Provider>).toJSON()

    expect(tree).toMatchSnapshot()
  })

gives Expected closing tag to match indentation of opening react/jsx-closing-tag-location error. If you fix that, you get Expected indentation of 4 space characters but found 33 react/jsx-indent error.

[ljharb]

  test('renders default state correctly', () => {
    const tree = renderer.create(
      <Provider store={mockStore()}>
        <JobForm />
      </Provider>
    ).toJSON()

    expect(tree).toMatchSnapshot()
  })

[mrchief]

2 issues with that:

eslint --fix formats that to

    const tree = renderer.create(<Provider store={mockStore()}>
      <JobForm />
                                 </Provider>).toJSON()

which then gives the react/jsx-indent error.

If you run eslint without the autofix. you get error Unexpected newline after '(' function-paren-newline error.

Been struggling with this for past few hours. :(

[ljharb]

@mrchief then try:

 test('renders default state correctly', () => {
    const tree = renderer.create((
      <Provider store={mockStore()}>
        <JobForm />
      </Provider>
    )).toJSON()

    expect(tree).toMatchSnapshot()
  })

[mrchief]

@ljharb That works. I'm not sure if I like the extraneous parens.

And the fact that I've to go rewrite all my existing code - which I think I'll have to rollback in some future update that'll complain about the extra parens.

[rbalicki2]

I, too, would like line-aligned. I don't think it's a niche requirement, TBH.

Adding extra line breaks so that the opening tag and the closing tag can be in the same column adds a lot visual bloat that doesn't need to be there, especially in components with multiple nested conditions. (Yes, I know, these can be extracted out, but there's a lot of legacy code).

[ezzatron]

@ljharb Can you expand on the reasons why you recommend wrapping multiline JSX in parens? The react/jsx-wrap-multilines rule docs are pretty vague/subjective:

Wrapping multiline JSX in parentheses can improve readability and/or convenience.

I did a bit of my own research, and found some potential reasons that have to do with automatic semicolon insertion for return statements:

• Why use parenthesis on JavaScript return statements?
• This comment on a prettier/prettier issue

But if that's the reason for the recommendation, wouldn't it be better / more comprehensive to simply lint for unreachable code? Happy to have my opinion changed btw, I just want to understand better.

[ljharb]

@ezzatron a single JSX element (containing all its children) is a single method. All multiline constructs should be wrapped by parens, brackets, or braces so that it's explicitly clear what the single value is. Certainly this helps with ASI hazards, but it also visually helps group a JSX element without needing to look solely for the closing jsx tag.

[Chudesnov]

This issue mostly concernes people who don't use airbnb's style guide (because jsx-wrap-multilines allows to avoid this issue), so I'll try not to mention it but focus on this individual rule. (Also I'd like to work on getting this issue resolved, so let's open a PR for it at #1745)

I doubt that

an opening tag enters you into an entirely distinct parsing context, it's incredibly important to denote that <...>

While this statement is strictly true in terms of specifications (ECMA-262 / Facebook JSX), the distinction between them is irrelevant from the application developer's standpoint, instead, the whole point of using JSX is to hide the underlying nested-function-calls API, not to switch contexts (especially in code style).

a single method <...> should be wrapped <...> so that it's explicitly clear what the single value is.

If that's the rationale to warn about code style OP suggested, then it would also be fair to warn about

  return createElement(
    LongComponentClassNameWithLotsOfUnneccessaryPrefixes,
    {},
    null
  )

and prefer

  return createElement(
                        LongComponentClassNameWithLotsOfUnneccessaryPrefixes,
                        {},
                        null
                      )

or

  return createElement
  (
    LongComponentClassNameWithLotsOfUnneccessaryPrefixes,
    {},
    null
  ) // except this does not work

But for JS it's unlikely a matter of objective readability (example 2 is actually less readable for people who don't have a lot of experience with languages other than JavaScript), but solely a matter of taste.

[ljharb]

@Chudesnov it's still relevant because things inside { }, in a jsx context, follow JS rules, whereas everything else in a jsx context follows jsx rules. jsx hides the function calls; it can not hide the cognitive overhead of needing to know about the contexts and when to switch between them.

[golopot]

My opinion is that this rule should stop checking the column location of the closing tag, and leave the indent dealt by indent or jsx-indent. And so that this rule only check that closing tag is in a new line.

[rojobuffalo]

I'm supportive of the OP request. I really like not wrapping jsx in parens and satisfying line-aligned.

return <div>
</div>;

[qfox]

Heya. Any activity here?