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-indent and indent require the same indentation amount #915

Closed
Wilfred opened this issue Oct 17, 2016 · 14 comments
Closed

jsx-indent and indent require the same indentation amount #915

Wilfred opened this issue Oct 17, 2016 · 14 comments

Comments

@Wilfred
Copy link
Contributor

Wilfred commented Oct 17, 2016

Related to #898, but I think it's sufficiently different to warrant a separate issue.

Given the JS:

import React from 'react';

function foo() {
  if (true) {
    if (true) {
      if (true) {
        someText = [
          <span>{text}</span>,
          otherText,
        ];
      }
    }
  }
}

and following eslint configuration:

{
    "parserOptions": {
        "ecmaVersion": 6,
        "ecmaFeatures": {
            "jsx": true,
            "impliedStrict": true
        },
        "sourceType": "module"
    },
    "rules": {
        "indent": ["warn", 2],
        "react/jsx-indent": "warn"
    },
    "plugins": [
        "react"
    ]
}

Then I get a warning Expected indentation of 12 space characters but found 10. for the <span>.

I suspect this is because the default indent for jsx-indent is 4 spaces, whereas I've configured indent to use 2 spaces.

I think that jsx-indent should either be able to handle different indent values to indent, or perhaps it should just reuse the existing indent value?

@ljharb
Copy link
Member

ljharb commented Oct 17, 2016

Rules can't access other rules' config. If you want 2 spaces, you have to configure both rules.

@tswaters
Copy link

This raises the question as to why there are two rules.

Could it not work similar how the eslint indent rule works (allowing override on SwitchCase, among others, if desired) -- allow it to accept extra parameters to overwrite specific things? e.g.,

"react/jsx-indent": ["error", 2, {props: 4}]

By default, it could use the same value.

@ljharb
Copy link
Member

ljharb commented Nov 12, 2016

@tswaters there are two rules because JSX isn't part of JS, so eslint core doesn't tend to have rules that can work with it.

@tswaters
Copy link

My mistake, I misread the original post.

I was talking about react/jsx-indent and react/jsx-indent-props being separate.... it seems to me these should be the same with -props controlled via options similar to how the core indent rule works in eslint.

@ljharb
Copy link
Member

ljharb commented Nov 14, 2016

That does seem reasonable - however, people indent their props in far crazier ways then they indent their object literal keys, so it might get unwieldy.

The first step would be enhancing jsx-indent to handle all the props cases, and ship that as a semver-minor - and then later, deprecate/remove jsx-indent-props.

@yannickcr, thoughts?

@blissdev
Copy link

I've encountered the same issue where after the ESLint 4 upgrade, the JSX and ESLint indent levels have to match up otherwise there are conflicts.

@HHogg
Copy link

HHogg commented Aug 22, 2017

After also doing the upgrade from

eslint@^3.7.1
eslint-plugin-react@^6.0.0

... to

eslint@^4.4.0
eslint-plugin-react@^7.1.0

The indent and jsx-indent conflict with one another... I've just disabled indent for the moment.

@ljharb
Copy link
Member

ljharb commented Aug 22, 2017

@HHogg you can instead configure indent to ignore JSX and delegate to eslint-plugin-react, which solves the problem.

@fruitraccoon
Copy link

@ljharb Any chance you have an example of how to setup the indent rule to ignore JSX?

I have the same situation as @HHogg. Looking at the docs for indent, I'm guessing that the ignoredNodes option is the way to do this, but eslint selectors are a bit outside my skill set at the moment and a working example to start from would be great! :)

@ljharb
Copy link
Member

ljharb commented Aug 24, 2017

https://eslint.org/docs/rules/indent#ignorednodes, with a selector of "JSX*" should do it.

@fruitraccoon
Copy link

Thank very much for the quick reply @ljharb - using your suggestion I was able to get something working. Using the wildcard character on the syntax tree node name didn't work for me, but by using the rule below, I seem to have about the equivalent of how our linting used to work:

"indent" : ["error", 4, { "ignoredNodes": ["JSXElement *"] }],

I believe that this will just turn off the indent rule for anything to do with JSX, including within any JSX expressions.

A side note for anyone else exploring this - I found this site very useful for finding out what the AST node names actually are (https://astexplorer.net/).

@ljharb
Copy link
Member

ljharb commented Aug 24, 2017

@Wilfred does this solve your issue?

@Wilfred
Copy link
Contributor Author

Wilfred commented Aug 24, 2017

I ended up configuring both plugins to just use the same indent, so I'm happy.

In a perfect world the plugins would detect the conflicting settings, but I don't see how that's possible. It's just a small papercut.

I'm happy to close this if you don't think there's anything else we can do here?

@ljharb
Copy link
Member

ljharb commented Aug 24, 2017

Unless eslint core agrees to make a global "indent" setting that the core indent rule reads (and then that jsx-indent could read), there's nothing we can do.

@ljharb ljharb closed this as completed Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants