Update: rewrite `indent` (fixes #1801, #3737, #3845, #6007, ...16 more)

[not-an-aardvark]

What is the purpose of this pull request? (put an "X" next to item)

[x] Bug fix (template)

Fixes https://github.com/eslint/eslint/issues/1801, fixes https://github.com/eslint/eslint/issues/3737, fixes https://github.com/eslint/eslint/issues/3845, fixes https://github.com/eslint/eslint/issues/6007, fixes https://github.com/eslint/eslint/issues/6571, fixes https://github.com/eslint/eslint/issues/6670, fixes https://github.com/eslint/eslint/issues/6813, fixes https://github.com/eslint/eslint/issues/7242, fixes https://github.com/eslint/eslint/issues/7274, fixes https://github.com/eslint/eslint/issues/7320, fixes https://github.com/eslint/eslint/issues/7420, fixes https://github.com/eslint/eslint/issues/7522, fixes https://github.com/eslint/eslint/issues/7616, fixes https://github.com/eslint/eslint/issues/7641, fixes https://github.com/eslint/eslint/issues/7662, fixes https://github.com/eslint/eslint/issues/7771, fixes https://github.com/eslint/eslint/issues/7892, fixes https://github.com/eslint/eslint/issues/8011, fixes https://github.com/eslint/eslint/issues/8038, fixes https://github.com/eslint/eslint/issues/8144

What changes did you make? (Give an overview)

The existing implementation of indent had a lot of bugs (see above list). It worked by detecting a node type (e.g. ObjectExpression), and then ensuring that the indentation around the object satisfies certain constraints (e.g. the properties of the ObjectExpression are offset by 4 spaces from the opening bracket). This approach had a number of disadvantages:

• Since it only checked indentation according to an explicit list of patterns, there were a lot of cases where it accidentally didn't check the indentation at all. For example, there was no check for the indentation of a closing ) in a CallExpression, so the rule just silently ignored incorrect indentation in these cases. (https://github.com/eslint/eslint/issues/7522)
• there were a lot of nodes where indentation wasn't checked at all. For example, it didn't check indentation for ternary expressions (https://github.com/eslint/eslint/issues/7420) or destructuring assignments (https://github.com/eslint/eslint/issues/6813).
• Since it could only check indent patterns on nodes, it couldn't check the indentation of comments (https://github.com/eslint/eslint/issues/3845, https://github.com/eslint/eslint/issues/6571) or optional tokens such as parentheses around an expression (https://github.com/eslint/eslint/issues/7522)

This commit rewrites the indent rule. The new strategy is based on tokens rather than nodes:

1. Create a hashmap (OffsetStorage#desiredOffsets). The keys are all the tokens and comments in the file, and the values are objects containing information for a specific offset, measured in indent levels, from a either a specific token or the first column. For example, an element in an array will have {offset: 1, from: openingCurly} to indicate that it is offset by one indentation level from the opening curly brace. All the offsets are initialized to 0 at the start.
2. As the AST is traversed, modify the offsets of tokens accordingly. For example, when entering a BlockStatement, offset all of the tokens in the BlockStatement by 1 from the opening curly brace of the BlockStatement.
3. After traversing the AST, calculate the expected indentation levels of every token in the file (according to the desiredOffsets map).
4. For each token, compare the expected indentation to the actual indentation in the file, and report the token if the two values are not equal.

This has the following advantages:

• It is guaranteed to check the indentation of every single token in the file, with the exception of some tokens that are explicitly ignored*. This ensures that no tokens end up unexpectedly being ignored.
• Since tokens/comments are used instead of nodes, there are no unchecked "stray tokens".
• All nodes are evaluated in a context-free manner. In other words, each node only has to set an offset for its own children, without worrying about what how much indentation the node itself has or what the node's parents are.
• The rule ends up with an expected indentation map for the entire file at once, and so it can fix the entire file in one pass. (The previous implementation often required multiple passes. For example, if a node was misaligned with its parent in the previous implementation, the node would get fixed, even if the node's position was actually correct and the parent was off.)

*There are a few cases where the new implementation explicitly ignores lines. I decided to do this because there is a huge amount of inconsistency in what people seem to prefer for these cases. In the future, we might want to stop ignoring these cases so that the indentation of all lines is checked. One such case is:

({
  foo:
  bar
});

// versus

({
  foo:
    bar
});

Comments are treated a bit differently from tokens in that they can have several different indentations. This is because it can be difficult to tell what the comment is referring to. For example:

if (foo) {
  doSomething();
  // comment about the doSomething() call
} else if (bar.baz()) {
  doSomethingElse();
}

// versus

if (foo) {
  doSomething();
// comment about the bar.baz() call
} else if (bar.baz()) {
  doSomethingElse();
}

Specifically, a comment is allowed to have one of three indentations:

1. The same indentation as the token right before it
2. The same indentation as the token right after it
3. The computed indentation for the comment itself

Is there anything you'd like reviewers to focus on?

As mentioned above, the new implementation checks a lot of cases that the old implementation did not check. Based on past experience with fixing bugs in the indent rule, I anticipate that this will break quite a lot of builds. We might want to consider holding this off until we release a major version.

I labeled this pull request as "evaluating". A lot of the bugs fixed by this issue are marked as "accepted", but some of them are not. We should also figure out how to manage ecosystem impact when we release this (see the paragraph above).

(This isn't an accurate measure of complexity, but the diff line count for this PR is a bit misleading; this change actually reduces the size of the indent rule by about 50 lines. The 2500 additional lines in the diff are mostly tests.)

[mention-bot]

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @gyandeeps and @eelyafi to be potential reviewers.

[mysticatea]

Amazing work, @not-an-aardvark !!

• I will review the code in this weekend.
• I think it should be a major version if indent rule introduces the errors of new kind of nodes by default.

[not-an-aardvark]

Added commits to address most of the comments.

Re. using array join in the multiline test cases, I'm thinking maybe we should try to improve the readability of these tests in general. How would you feel about formatting the tests like this?

ruleTester.run("indent", rule, {
    valid: [
        {
            code: `
if (foo) {
  bar();
} else {
  baz();
}
`,
            options: [2]
        }
    ]
});

That would allow the tests to clearly represent indentation without boilerplate from array joins or string concatenation, but it does make the file look less clean overall.

We could also use tagged template literals to avoid this effect by removing a constant number of indentation spaces from the resulting string. However, that might make it harder to see what's going on without looking at the template function.

function unIndent(strings) {
  // remove a constant amount of indentation, and return the resulting string.
}

ruleTester.run("indent", rule, {
    valid: [
        {
            code: unIndent`
                if (foo) {
                  bar();
                } else {
                  baz();
                }
            `, // this might be a bit misleading about how much indentation the string actually has
            options: [4]
        }
    ]
});

[platinumazure]

Re: readability of multiline tests: I'm certainly not strongly attached to array join. I'm open to suggestions (such as a normal template string or a tagged template string).

I just find the \n at the end of the test case lines distracting, as it makes it harder to spot a trailing punctuator token (e.g., opening curly brace). As far as I'm concerned, nearly anything else is an improvement over that.

[lukeapage]

Does this ignore jsx and continue to leave that to the react plugin jsx indent rule?

[not-an-aardvark]

@lukeapage I'm not sure I understand the question. Could you clarify what you're expecting the new implementation to do?

[lukeapage]

So, either jsx e.g. The lines with html tags are checked now and therefore the react plugin rule becomes obsolete, or those lines are also excluded and people should continue to use the rule in the react plugin to verify jsx indentation. I just wanted to make sure whatever approach was part of the plan.

[not-an-aardvark]

Thanks for bringing this up. I am not very familiar with JSX, and this implementation does not explicitly account for it. However, given that every token in a file is now checked, I think it's likely that the new implementation might report errors that the old implementation did not.

Maybe we should add special logic to account for unknown AST nodes.

[lukeapage]

heres the react plugin rule: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-indent.md because you can put standard js inside jsx you can end up with quite complex code to check e.g.

return a && (
    <a>    
        {
             b ?
                "string" :
               <more-jsx/>
        }
    </a>);

etc. you can see the number of different scenarios in the test suite for the react plugin https://github.com/yannickcr/eslint-plugin-react/blob/master/tests/lib/rules/jsx-indent.js

so maybe excluding everything inside an unknown node might work?

[not-an-aardvark]

Hmm, on second thought I think ignoring unknown AST nodes would not be a good idea, because whenever new syntax became supported the rule would end up unexpectedly ignoring it, which is the same problem we have now.

[vitorbal]

Maybe we can just ignore indentation inside JSXElement nodes?

[not-an-aardvark]

@platinumazure No problem! Please keep pointing out misleading comments if you find them; they'll make it difficult to understand the code in the future.

[mysticatea]

I'm sorry for late.

Again, I appreciate indent rule's rewrite. Thank you!

I found some issues.

Stack overflow

I got a stack overflow error if I linted acorn/src/expression.js. The current indent rule can finish linting the file.

$ cd acorn
$ npm install ../eslint
$ ./node_modules/.bin/eslint src/expression.js --rule "indent:[error,2]" --parser-options "ecmaVersion:6,sourceType:module"

Performance

I saw that the new implementation was x135 slower in some my private repository. I'm sorry, I cannot show the repositories... I guess this problem will be solved if the stack overflow problem is solved.

http://eslint.org/docs/developer-guide/working-with-rules-new#per-rule-performance

• Before: 0.8 seconds
• After: 108 seconds

Exports

I got unexpected errors on export statements of ES Modules.

/*eslint indent: error */
const foo = 1, bar = 2;

export {
    foo,  //※※※ error  Expected indentation of 0 spaces but found 4  indent
    bar   //※※※ error  Expected indentation of 0 spaces but found 4  indent
}

Semicolons at the first of lines

const doSomething = () => {}
;[1,2,3].forEach(doSomething)  //※※※ error  Expected indentation of 4 spaces but found 0  indent

I think this is an unexpected error for the semicolon-less style.

Template curlies

I felt that handling template curlies of this implementation was unexpected a bit. It seems:

• The first line in template curlies is expected 1 indent from the template's indent.
• The closing curly of template curlies is expected the same indent as the template's indent.

This behavior works perfectly for short templates, but not for long templates. An example is eaw/scripts/generate.js; I expected no error in this code, but got 2 errors (edit: I got 2 errors).

  161:13  error  Expected indentation of 8 spaces but found 12  indent
  168:9   error  Expected indentation of 4 spaces but found 8   indent

✖ 2 problems (2 errors, 0 warnings)

edit: But I expected the following errors:

  161:13  error  Expected indentation of 4 spaces but found 12  indent
  168:9   error  Expected indentation of 0 spaces but found 8   indent

✖ 2 problems (2 errors, 0 warnings)

People might use long template literals to generate HTML or something like. In that case, indentation which is based on the indent of the opening curly (edit: based on the line the opening curly exists) is expected because the start of the template literal is too far, I think.

This behavior is complex and there are vary preferences. Maybe it should not warn the first line in template curlies and the closing curly.

Nested ternary.

I like the following style:

let foo = (
    a === 1 ? "one" :
    a === 2 ? "two" :
    a === 3 ? "three" :
    a === 4 ? "four" :
    /* otherwise */ "more"
)

The current indent rule allows this code, but this implementation disallows. I want to allow the code above. I'm OK if it's behind of an option.

[mysticatea]

I rethought and edited the section about "template curlies".

[not-an-aardvark]

@mysticatea Thanks for the review! I'll try to fix the issues that you mentioned.

@platinumazure I've been working on making the test file easier to read. How does this look? (I pushed it onto a separate branch for now to avoid making the diff for this PR too large, but let me know if I should merge it into this PR.)

[not-an-aardvark]

@mysticatea How should the rule handle this?

var foo = () => {},
    bar = 1,
    baz = 2
    ; // <-- The rule currently enforces a semicolon here

var foo = () => {},
    bar = 1,
    baz = 2
    ; [1, 2, 3].map(foo) // <-- This looks strange

var foo = () => {},
    bar = 1,
    baz = 2
; [1, 2, 3].map(foo) // <-- This looks ok, but it contradicts the first option.

I think it would be best to make the first option incorrect, so the semicolon always has no offset.

var foo = () => {},
    bar = 1,
    baz = 2
;

[platinumazure]

@not-an-aardvark Is it still valid to have the semicolon after the last declarator init (at the end of the line)?

On Nov 30, 2016 2:36 PM, "Teddy Katz" notifications@github.com wrote:

@mysticatea https://github.com/mysticatea How should the rule handle this?

var foo = () => {}, bar = 1, baz = 2 ; // <-- The rule currently enforces a semicolon here var foo = () => {}, bar = 1, baz = 2 ; [1, 2, 3].map(foo) // <-- This looks strange var foo = () => {}, bar = 1, baz = 2 ; [1, 2, 3].map(foo) // <-- This looks ok, but it contradicts the first option.

I think it would be best to make the first option incorrect, so the semicolon always has no offset.

var foo = () => {}, bar = 1, baz = 2 ;

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eslint/eslint/pull/7618#issuecomment-263987650, or mute the thread https://github.com/notifications/unsubscribe-auth/AARWer98T-umiJRnQTw3tIR4JVOzvUI1ks5rDd42gaJpZM4K2Ir9 .

[not-an-aardvark]

@platinumazure Yes, that will work as it did before. My question only applies to the case where the user chooses to put the semicolon on the next line.

[mysticatea]

@not-an-aardvark I have a concern about the semicolon for people which are using comma-first style. (e.g. https://gist.github.com/isaacs/357981)

var foo = () => {}
  , bar = 1
  , baz = 2
  ;

indent rule is huge and monolithic, so probably it's difficult to keep configure-less. :worried:

[not-an-aardvark]

I suppose another option would be to ignore the trailing semicolon so that it's never reported.

var foo = 1,
    bar = 2
    ; // ok

var foo = 1,
    bar = 2
; // also ok

[not-an-aardvark]

I've also been thinking about whether it would be possible to parse arbitrary node configs, so that we wouldn't have to keep adding options to the rule, and plugin users wouldn't need to have their own version of the indent rule to enforce configs for non-standard nodes.

For example:

module.exports = {
  rules: {
    indent: ["error", 2, {
      FunctionDeclaration: {parameters: 1},
      WhileStatement: {body: 2},
      ExportNamedDeclaration: {specifiers: "first"}
    }]
  }
};

The rule would notice a WhileStatement option, and automatically add an indent level of 2 to the body of all WhileStatement nodes (even if we didn't have explicit code to handle WhileStatement nodes in the indent rule.

[mysticatea]

• ✔ Stack overflow
• ✔ Exports
• ✔ Template curlies

Performance

In my private repo, new indent rule still seems to be x135 slower than the current indent rule. It seems to get so slow if there are large object literals or array literals, such as test-data.js.

• Before: 803.928 ms
• After: 115286.021 ms

In eslint repo, new indent rule seems to be x6 slower than the current indent rule.

• Before: 697.510 ms
• After: 4314.090 ms

``` C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 4436.618 | 100.0% no-underscore-dangle | 0.407 | 0.0% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 4277.314 | 100.0% no-underscore-dangle | 0.399 | 0.0% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 4223.935 | 100.0% no-underscore-dangle | 0.379 | 0.0% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 4417.716 | 100.0% no-underscore-dangle | 0.396 | 0.0% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 4314.090 | 100.0% no-underscore-dangle | 0.407 | 0.0% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [indent-rewrite ≡]> git co master Switched to branch 'master' Your branch is up-to-date with 'origin/master'. C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 680.669 | 99.9% no-underscore-dangle | 0.395 | 0.1% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 697.510 | 99.9% no-underscore-dangle | 0.400 | 0.1% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 699.458 | 99.9% no-underscore-dangle | 0.413 | 0.1% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 706.844 | 99.9% no-underscore-dangle | 0.422 | 0.1% C:\Users\t-nagashima.AD\Documents\GitHub\eslint [master ≡]> eslint lib --env es6 --rule "indent:[error,4,{SwitchCase:1}]" --no-eslintrc Rule | Time (ms) | Relative :--------------------|----------:|--------: indent | 677.977 | 99.9% no-underscore-dangle | 0.405 | 0.1% ``` - I'm not sure why there are `no-underscore-dangle`. - I'm using global-installed `eslint-cli` here.

. I felt we need to improve this performance.

Semicolons at the first of lines

I think another option is:

• 1 indent if there are statements preceded by the semicolon.
• 0 indent if the semicolon is only token on the line.

// ✔ GOOD
var foo = () => {}
  , bar = 1
  , baz = 2
  ;
var foo = () => {}
  , bar = 1
  , baz = 2
;[bar, baz].forEach(foo)

// ✘ BAD
var foo = () => {}
  , bar = 1
  , baz = 2
;
var foo = () => {}
  , bar = 1
  , baz = 2
  ;[bar, baz].forEach(foo)

[not-an-aardvark]

I was able to improve performance by 60% by caching calls to getDesiredIndent, but it's still not as fast as the current rule. I'll keep investigating to see what's causing the performance issues.

[not-an-aardvark]

I improved the performance of getTokensAndComments(), which decreased the total runtime by about 75% on the large file. The total runtime for the rewritten rule on that file is now only about 60% more than the rule on master.

[not-an-aardvark]

I decided to just have the rule ignore trailing semicolons after VariableDeclaration nodes. I think changing the desired indentation based on whether a statement follows a semicolon would be a bit inelegant.

[eslintbot]

LGTM

[mysticatea]

Almost looks good to me. Performance issue has been reduced!

... Can I allow the following code by an option? This code is allowed by the current indent rule.

let foo = (
    a === 1 ? "one" :
    a === 2 ? "two" :
    a === 3 ? "three" :
    a === 4 ? "four" :
    /* otherwise */ "more"
)

[not-an-aardvark]

An option sounds good to me. Personally I prefer this:

let foo = (
    a === 1
        ? "one"
        : a === 2
            ? "two"
            : a === 3
                ? "three"
                : a === 4
                    ? "four"
                    : /* otherwise */ "more"
)

But I think adding an option for it is a good idea. How about something like flatTernaryExpression: true?

Ideally, I think the option name should follow the pattern from the other option names (e.g. ConditionalExpression: "flatten". But I'm not sure how to fit this option into the pattern.

If I'm understanding it correctly, the option would enforce 0 offset for the ConditionalExpression.alternate if the alternate is also a ConditionalExpression.

[mysticatea]

flatTernaryExpression: true sounds good to me.

If I'm understanding it correctly, the option would enforce 0 offset for the ConditionalExpression.alternate if the alternate is also a ConditionalExpression.

Exactly!

[not-an-aardvark]

While running the regression build I noticed several bugs in this implementation, so I'm in the process of fixing them now.

[not-an-aardvark]

Status of this PR:

• Implementation is done, and I've finished investigating the results from the regression build.
• Looking at other codebases, it seems like misindented ternary expressions and comments are quite common, so the rewrite will probably cause many new errors to be reported. (From what I can tell, a lot of people think the rule already does validate these things, so these errors slip into the codebase unnoticed.)
• We still need to decide what to do about JSX elements and other unknown syntax. Due to the way the previous rule was written, indentation in unknown syntax would just be ignored. However, the current rule reports errors, which could be annoying. (I noticed this when using this to lint a Flow project -- for example, it doesn't expect any indentation for exported types). I think the best way to address this would be to have an option to ignore specific node types (e.g. "ignore": ["JSXElement"], which would cause the rule to simply ignore the indentation of all tokens in JSXElement nodes.

[jameswomack]

My team and I are very stoked about this PR. We 💘 🏩 💘 LOVE 💘 🏩 💘 ESLint and I see ESLint used every day in myriad teams at Netflix.

Although ESLint is a major help to us, its handling of fixing and reporting warning/errors of indentation is an ongoing chronic pain point 🤕 . In an effort to get that fixed, I searched through repos and issues until I found this PR.

How I've tested this PR

I'm now using the latest commit as my ESLint package, e.g.:

{
  "devDependencies": {
    "eslint": "git://github.com/eslint/eslint.git#4716d2baf235fe0bdae8d199666f1b727c42cb95"
  }
}

Major pain point this PR seems to solve

• --fix re-indenting everything but line comments

Lesser pain points this PR seems to solve

Indentation and indentation warnings/errors regarding the following:

• Property accessors broken up by newlines, e.g.

  Foo.getData()
  .then(processData)
  .then(presentData)
  .catch(presentDataFailDialog)

• Arrow functions
• Block comments

I'm not in a position to say this PR should be merged as-is. I can say that it's working for my team as-is though. We plan to use 4716d2baf235fe0bdae8d199666f1b727c42cb95 until the aforementioned indentation-related issues are fixed in eslint@latest.

[platinumazure]

We are hoping to merge this pretty soon with the ESLint 4.0 release (not yet scheduled but should happen in a month or two). Thanks for letting us know that this is going to help!

On Mar 16, 2017 17:20, "James J. Womack" notifications@github.com wrote:

My team and I are very stoked about this PR. We 💘 🏩 💘 LOVE 💘 🏩 💘 ESLint and we use it every day at Netflix.

Although ESLint is a major help to us, its handling of fixing and reporting warning/errors of indentation is an ongoing chronic pain point 🤕 . In an effort to get that fixed, I searched through repos and issues until I found this PR. How I've tested this PR

I'm now using the latest commit as my ESLint package, e.g.:

{ "devDependencies": { "eslint": "git://github.com/eslint/eslint.git#4716d2baf235fe0bdae8d199666f1b727c42cb95" } }

Major pain point this PR seems to solve

• --fix re-indenting everything but line comments

Lesser pain points this PR seems to solve

Indentation and indentation warnings/errors regarding the following:

• Property accessors broken up by newlines, e.g.

  Foo.getData() .then(processData) .then(presentData) .catch(presentDataFailDialog)

• Arrow functions

• Block comments

I'm not in a position to say this PR should be merged as-is. I can say that it's working for my team as-is though. We plan to use 4716d2baf235fe0bdae8d199666f1b727c42cb95 until the aforementioned indentation-related issues are fixed in eslint@latest.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eslint/eslint/pull/7618#issuecomment-287208650, or mute the thread https://github.com/notifications/unsubscribe-auth/AARWekcMoJR92l6SqCnG9yJMykr8GBiDks5rmbWsgaJpZM4K2Ir9 .

[not-an-aardvark]

In today's TSC meeting, the TSC accepted this proposal for 4.0.0. We also decided to add a deprecated indent-legacy rule (a copy of the current indent rule) to make migration to v4.0.0 easier. (Although the rewritten rule has the same schema as the old rule, it reports a lot of errors that the previous rule missed.)

To make this easier to review, I'll leave this PR as-is and add indent-legacy in a separate PR.

[ilyavolodin]

@jameswomack It's awesome to hear you guys are already testing this out. Please let us know if you run into any issues that we can fix before this gets merge in. As @platinumazure and @not-an-aardvark noted, we are planning on merging this in as part of 4.0 release, which should be coming soon-ish.

P.S. It's great to hear that Netflix is using ESLint. That's one of the few large companies that I haven't heard anything from in regards to ESLint usage. If you get a chance, could you adding your company logo to our website?

[eslintbot]

LGTM

[eslintbot]

LGTM

[jameswomack]

@ilyavolodin Thanks for the update RE: v4! We've continued to use commit 4716d2baf235fe0bdae8d199666f1b727c42cb95 as our ESLint package over the last 5 days without issues. I've opened a PR to add the Netflix logo to the site as well.

[eslintbot]

LGTM

[eslintbot]

LGTM

[kaicataldo]

@jameswomack Really appreciate the feedback - this is hugely helpful for us!

[lukeapage]

Now this is on master, I tried running it in our large (react/jsx) codebase.

I am getting alot of errors because jsx is not being excluded from identation errors e.g.

        wrapper = mount(
            <X
                {...props}
            />
        );

deciding that {...props} should be on the same indentation as <X (and alot worse, e.g. comments in code inside jsx being indented flat)

I didn't come across anything else wrong, generally the rule works alot better, but i can do a proper review once the screwing up of jsx indentation is fixed.

Let me know if you want a bug or more info.

[ilyavolodin]

Ohh.. That's a really good point. Old indent rule was skipping all of the JSX nodes, and leaving it to plugins to enforce indentation in JSX. New rule is checking all of the nodes (including JSX). I wonder if we should make exception for JSX? @not-an-aardvark thoughts?

[not-an-aardvark]

@lukeapage Thanks for trying this out.

I had suspected that this might be a problem. I'm not very familiar with JSX, so I have a few questions:

• Did the old indent rule check this case, or did it just ignore it?
• Are there multiple common indentation styles for JSX, or would everyone be happy if the rule just enforced an indentation level of 1 for {...props}?
• You mentioned "comments in code inside jsx being indented flat". Can you give an example of this?

I think the options here are to either (a) add reasonable checking for JSX nodes, or (b) ignore them (like we'll probably have to do with other unknown nodes). More details on this decision can be found here.

[soda0289]

I'm pretty sure the old rule ignored JSX entirely, can't remember seeing indentation warnings on any JSX code. There are several rules in eslint-plugin-react, jsx-indent jsx-indent-props jsx-closing-bracket-location, that I think will not be compatible if we try and enforce indenting.

[not-an-aardvark]

@soda0289 As someone who hasn't used eslint-plugin-react, I'm trying to distinguish between these two possibilities. Which of them is closer to being true?

1. Everyone agrees on how JSX code should be indented, but the old indent rule didn't check it. The jsx-indent rule and its variants implement these semantics. Everyone can turn these rules on for their jsx without much configuration, and be satisfied with the indentation that they enforce.
2. There are multiple competing styles for JSX indentation, and jsx-indenthas many configuration options. If we checked JSX by default in the indent rule without these options, a lot of people wouldn't like the resulting indentation.

If (1) is true, then I think we should check JSX in indent by default. If (2) is true, I think we should ignore it and leave it to jsx-indent.

[jez9999]

Does this rewrite allow the SwitchCase indent rule to be disabled, as I suggested in #7270 ?

[kaicataldo]

My preference would be to improve the JSX indentation and have it on by default but to add an option that allows the user to disable it if they want to use a custom rule (ignoreJSX?).

For instance, in the case reported above the rule should allow both of these options:

wrapper = mount(
    <X
        {...props}
    />
);

wrapper = mount(
    <X {...props} />
);

Edit: Interestingly enough, this is covered by two rules in eslint-plugin-react:

• https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-indent.md
• https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-indent-props.md

[soda0289]

@not-an-aardvark I agree with 1) that there is a standard way that everyone agrees on how JSX should be indented. It is HTML/XML based so inner tags should have one level of indentation. That is standard.

When it comes to props and breaking a JSX single tag over multiple lines I have seen different ways. I think @kaicataldo made a good example of just ignoring them and ignoring the closing tag.

[not-an-aardvark]

@soda0289 I don't think @kaicataldo's example would be a problem here because the rule could allow both of the given examples. The case I'm concerned about is if there are two common styles that are exactly the same except for their indentation, because then the rule wouldn't be able to allow both styles at once without ignoring the node entirely. Are there any cases like that?