Eslint-plugin-react: jsx-indent behavior with { bool && jsx } syntax

[MoOx]

I just upgraded to v4.3 and got issue on this code

        {
          head.title &&
          <h1>
            { head.title }
          </h1>
        }

  63:11  error  Expected indentation of 12 space characters but found 10  react/jsx-indent

But the change is weird:

        {
          head.title &&
            <h1>
              { head.title }
            </h1>
        }

Is this on purpose? I don't see any change in the changelog about that. I also added some other rules it should not interfere right?

[yannickcr]

jsx-indent was not updated since v4.0.0 so I don't know what happened here.

Can you give me your eslint, babel-eslint and eslint-plugin-react versions before and after the upgrade?

[MoOx]

Ok nevermind, I wasn't using jsx-indent before... (Sorry about the false alarm).

So my issue would be "Is there a way to disable jsx-indent for this use case?" (Renaming accordingly).

[ljharb]

@MoOx whether you surround the <h1 /> in parens or not (as other rules/guides require), semantically it's a nested expression, and as such, it should be indented.

[MoOx]

Sorry but I don't really get what you are explaining.

Why _jsx_-indent is warning me about non jsx indentation? I would expect that jsx-indent rule only warn for indentation _inside_ jsx nodes, like it's explained in the doc.

Btw, this seems consistent

{
  condition1 &&
  condition2
}

But this doesn't

{
  condition1 &&
    condition2
}

I don't understand why the first JSX level need to be indented, seems like a bug to me.

[ljharb]

OK so, jsx-indent handles indentation for everything inside JSX, which includes this JS - namely because the normal indent rule won't handle it.

I agree with your "condition1" and "condition2" analogy - but that's not the same thing. Imagine that instead of "condition2", you had a multiline array:

// bad
{
  condition1 && [
  1,
  2,
  ]
}

// good
{
  condition1 && [
    1,
    2,
  ]
}

The reason this is the right analogy is because every JSX expression has implicit parens around them - the fact that they're optional to type, like semicolons, doesn't mean they aren't always semantically present (like semicolons). In other words, your original example, with parens added:

// bad
        {
          head.title && (
          <h1>
            { head.title }
          </h1>
          )
        }

// good
        {
          head.title && (
            <h1>
              { head.title }
            </h1>
          )
        }

[MoOx]

I totally understand that this code is not "good"

{
  condition1 && [
    1,
    2,
  ]
}

But I found this is opinionated to assume the same behavior for the JSX I provided, as I consider to be transformed as this

{
  head.title &&
  React.createElement("h1", ...)
}

And not

{
  head.title &&
    React.createElement("h1",)
}

I think if you got parenthesis, it's fine to yell for this code

        {
          head.title && (
          <h1>
            { head.title }
          </h1>
          )
        }

But not on this one

        {
          head.title &&
          (<h1>
            { head.title }
          </h1>)
        }

And eslint will not yell for code like this

if (
  condition &&
  condition2
) {
  // stuff
}

On all the different react projects with different teams (note: I am freelance) I worked for the past 3 years, we indented this way, and not using parenthesis, so it's feel like I am stuck and cannot use this rule at all.

Can we at least provide the user the choice via an option?

I get your point for the "it lints everything in jsx", which make totally sense now you are highlighting this point, but we should provide flexibility (or maybe just not assume too much :p).

[yannickcr]

Well. My first intent when writing the jsx-indent rule was to be as close as possible to the original eslint indent rule (actually, most of it is a simple copy-past from the original rule).

If I remember correctly the rule assume that a JSXOpeningElement (<h1>) should always start one level deeper than his parent if they are not both on the same line (even if the parent is not a JSXOpeningElement itself, like a LogicalExpression in our case).

So... I'm divided here. Both @ljharb explanation and @MoOx expected behavior makes sense to me.

[ljharb]

@MoOx I agree that with parens, either of the two styles would be appropriate:

{
  foo &&
  (<h1>
    bar
  </h1>)
}

{
  foo && (
    <h1>
      bar
    </h1>
  )
}

// your preferred style without parens
{
  foo &&
  <h1>
    bar
  </h1>
}

Without parens, I think your preferred style (the latter) is uncommon and unidiomatic, but does make subjective sense - only while omitting the parens.

I don't mind adding an option for this indentation, but I'd want it to only allow it when parens around the jsx element are omitted.

Thoughts?

[MoOx]

I really thing all 3 examples above are valid by default, but an option would be fine.

Note that this code is valid with eslint

/* eslint indent: ["error", 2] */

var foo, bar, baz;

if (
  foo &&
  (bar)
) baz()

if (
  foo && (
    bar
  )
) baz()

if (
  foo &&
  bar
) baz()

Note: It seems eslint indent rules does not even read indentation in if() condition. This code is valid too

/* eslint indent: ["error", 2] */

var foo, bar, baz;

if (
    foo &&
        (bar)
) baz()

Tried on the demo http://eslint.org/demo/

[ljharb]

Conditionals are different than jsx { } - so while I think eslint's rule _should_ cover it with indent, I don't think that's the right analogy.

[MoOx]

About about this

/* eslint indent: ["error", 2] */

var foo, bar;

foo &&
(bar());

foo && (
  bar()
);

foo &&
bar();

foo &&
  bar();

[ljharb]

I think you've established they're not parallel :-) However I think that's a gap in indent, and it's an appropriate thing for jsx-indent to be enforcing.

[MoOx]

Linting is also about coding style and preferences, that why I was asking to allow this syntax to not be considered as an error. Enforcing coding style is weird imo.

[smashercosmo]

Also, for example, in our project we indent only markup in jsx and not expressions. For example

<div>
  {condition &&
  <div>Hello world!</div>
  }
</div>

Would be awesome to be able to provide options for that kind of formatting.

[billyjanitsch]

Ternaries are also an issue. We'd like to do:

<div>
  {isCondition
    ? <div>
        some content
      </div>
    : <span />
  }
</div>

But this currently errors, instead wanting:

<div>
  {isCondition
    ? <div>
      some content
    </div>
    : <span />
  }
</div>

Which IMO is much less readable, since the opening and closing <div> tags are not aligned.

[milesj]

Can this really be it's own rule instead of relying on secondary indent rules?

// bad 
{(var && <Comp />)}

// good
{var && <Comp />}

// good
{var && (
  <Comp />
)}

// bad
{var && <Comp>
  <div />
</Comp>}

// good
{var && (
  <Comp>
    <div />
  </Comp>
)}

// bad
{var 
  ? <Comp />
  : <Comp />
}

// good (assuming no props)
{var ? <Comp /> : <Comp />}

// good
{var ? <Comp /> : (
  <Comp>
    <div />
  </Comp>
)}

// good
{var ? (
  <Comp>
    <div />
  </Comp>
) : <Comp />}

// good
{var ? (
  <Comp>
    <div />
  </Comp>
) : (
  <Comp>
    <div />
  </Comp>
)}

[jrajav]

Any update on this? As @billyjanitsch pointed out, the way this enforces indentation for ternaries makes no sense.

[milesj]

The recent commits listed in this PR has now broken syntax like this:

expect(wrapper.prop('children')).to.deep.equal([
  '\n  ',
  <Element key="0" tagName="main" attributes={{ role: 'main' }}>
    {[
      '\n    Main content\n    ',
      <Element key="1" tagName="div" attributes={{}}>
        {[
          '\n      ',
          <Element key="2" tagName="a" attributes={{ href: '#' }}>
            {['Link']}
          </Element>,
          '\n      ',
          <Element key="3" tagName="span" attributes={{ className: 'foo' }}>
            {['String']}
          </Element>,
          '\n    ',
        ]}
      </Element>,
      '\n  ',
    ]}
  </Element>,
  '\n  ',
  <Element key="4" tagName="aside" attributes={{ id: 'sidebar' }}>
    {['\n    Sidebar content\n  ']}
  </Element>,
  '\n\n',
]);

It expects all instances of <Element> and </Element> to be indented 2 more spaces, even though the items in the array would be off. This seems wrong.

[billyjanitsch]

The referenced commit 42037c4 only fixes one case, but doesn't address many other cases such as the one mentioned in https://github.com/yannickcr/eslint-plugin-react/issues/540#issuecomment-213571362.

[ljharb]

@billyjanitsch do you mind filing a new issue so that it can be more easily addressed?