indent rule's ignoredNodes not respected when multiline

[bawjensen]

Tell us about your environment

• ESLint Version: 4.16.0
• Node Version: 8.9.4
• npm Version: 5.6.0

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint

Please show your full configuration:

Configuration

module.exports = {
    parser: 'babel-eslint',
    rules: {
        indent: ['error', 4, {
            ignoredNodes: [
                'JSXOpeningElement',
            ],
        }],
    },
};

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

function Test() {
    return (
        <div>
            {_.map(array, arrayValue => {
                return (condition &&
                    <Inner
                        one={arrayValue.one}
                    >
                        {arrayValue.two}
                    </Inner>
                );
            })}
        </div>
    );
}

./node_modules/.bin/eslint test.jsx

What did you expect to happen?

No error.

What actually happened? Please include the actual, raw output from ESLint.

path/to/sandbox/2018-01-23/test.jsx
  8:1  error  Expected indentation of 24 spaces but found 20  indent

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

[bawjensen]

I dug into the source code a little on this one already. It seems, to my untrained eye, that the crux of the matter is this line:

eslint/lib/rules/indent.js

Line 954 in 9cbb487

if (!unknownNodeTokens.has(offsets.getFirstDependency(token))) {

This line checks the token corresponding to the node about to be ignored, finds the token upon which its indentation is based, and ensures it's outside the current node before ignoring it.

In this scenario, I want to ignore indentation around JSXOpeningElement, so I have it in my ignoredNodes. What I observed with a little debugging is that the opening token < on line 6 gets properly ignored because the token upon which its indentation is based belongs to another node. However, the closing token > on line 8 has its indentation based upon the opening token (line 6), which belongs to the same node, and therefore doesn't make it into the _ignoredTokens WeakSet.

[not-an-aardvark]

I think this is happening because JSX indentation currently works in two phases:

• First the JSXElement handler gets run, which indents all tokens in the JSXElement by one level relative to the first <, even the tokens that are part of the opening element. After that happens the desired indentation looks like this:

  <Inner
      one={arrayValue.one}
      >
      {arrayValue.two}
  </Inner>

• Normally, the JSXOpeningElement handler would run afterwards and correct the indentation of the first >, but that's not happening in this case because JSXOpeningElement is ignored.

• The opening < on line 6 is actually getting ignored for unrelated reasons -- the indent rule currently doesn't check the operands of logical operators (e.g. &&).

[bawjensen]

Hm, interesting. That implied to my mind that I could add JSXElement to the ignoredNodes but that resulted in:

path/to/sandbox/2018-01-23/test.jsx
   8:1  error  Expected indentation of 24 spaces but found 20  indent
  10:1  error  Expected indentation of 24 spaces but found 20  indent

✖ 2 problems (2 errors, 0 warnings)
  2 errors, 0 warnings potentially fixable with the `--fix` option.

Which is the wrong direction, but better matches the original scenario of using Airbnb's recommended set of ignoredNodes: https://github.com/airbnb/javascript/blob/53b2d7d245ba4abefc0429bfda4a46f099b9ace5/packages/eslint-config-airbnb-base/rules/style.js#L141

If I copy paste theirs (with changing 2 spaces to 4) in a new config:

Configuration

module.exports = {
    parser: 'babel-eslint',
    rules: {
        // this option sets a specific tab width for your code
        // https://eslint.org/docs/rules/indent
        indent: ['error', 4, {
            SwitchCase: 1,
            VariableDeclarator: 1,
            outerIIFEBody: 1,
            // MemberExpression: null,
            FunctionDeclaration: {
                parameters: 1,
                body: 1
            },
            FunctionExpression: {
                parameters: 1,
                body: 1
            },
            CallExpression: {
                arguments: 1
            },
            ArrayExpression: 1,
            ObjectExpression: 1,
            ImportDeclaration: 1,
            flatTernaryExpressions: false,
            // list derived from https://github.com/benjamn/ast-types/blob/HEAD/def/jsx.js
            ignoredNodes: ['JSXElement', 'JSXElement > *', 'JSXAttribute', 'JSXIdentifier', 'JSXNamespacedName', 'JSXMemberExpression', 'JSXSpreadAttribute', 'JSXExpressionContainer', 'JSXOpeningElement', 'JSXClosingElement', 'JSXText', 'JSXEmptyExpression', 'JSXSpreadChild'],
            ignoreComments: false
        }],
    },
};

It results in:

path/to/sandbox/2018-01-23/test.jsx
  8:1  error  Expected indentation of 24 spaces but found 20  indent

✖ 1 problem (1 error, 0 warnings)
  1 error, 0 warnings potentially fixable with the `--fix` option.

(exactly the same error as originally)

So it's a less simple scenario than my original bug report, but this is actually what I care about fixing given the project I'm working in is based on Airbnb's config.

I'm not sure how this impacts your analysis, given that we can assume JSXElement's handler doesn't get run and the same symptom manifests.

[ljharb]

@airbnb is having this same problem; when JSXOpeningElement is in ignored nodes, the > on a multiline opening element - which has no node type - is not ignored.

[platinumazure]

@not-an-aardvark

The opening < on line 6 is actually getting ignored for unrelated reasons -- the indent rule currently doesn't check the operands of logical operators (e.g. &&).

I know this is ancillary to the issue, but is this something that should be changed? Should we only ignore the first token of the node after &&? Or maybe only ignore all tokens on the first line of the operand following &&, but allow the rule to lint later lines against the first line? I'll open a new issue if so. Thanks!

[not-an-aardvark]

I think the current behavior is that it ignores the first token after &&, but the rule lints later lines against the indentation of that first token.

Removing the "ignores logical operators" behavior is tracked in #8978.

[ljharb]

Reading this issue; I'm still not sure what code change would be needed to fix it :-/ certainly the >, since it's part of the JSXOpeningElement node, should be ignored.

Perhaps the JSXElement handler should be checking to see if any of those nodes are already ignored by another selector?

[not-an-aardvark]

If my explanation in #9882 (comment) was correct, then I think the solution would be to update the JSXElement handler to only offset the children of the element without offsetting the tokens that are part of the openingElement or closingElement. However, #9882 (comment) makes me think my explanation might not have been correct, so I need to investigate it further.

[not-an-aardvark]

This seems to resolve the originally-posted issue, provided that JSXElement and JSXOpeningElement are ignored:

diff --git a/lib/rules/indent.js b/lib/rules/indent.js
index 79a0f25c..acc52463 100644
--- a/lib/rules/indent.js
+++ b/lib/rules/indent.js
@@ -1043,7 +1043,6 @@ module.exports = {
                 offsets.ignoreToken(operator);
                 offsets.ignoreToken(tokenAfterOperator);
                 offsets.setDesiredOffset(tokenAfterOperator, operator, 0);
-                offsets.setDesiredOffsets([tokenAfterOperator.range[1], node.range[1]], tokenAfterOperator, 1);
             },
 
             "BlockStatement, ClassBody"(node) {

@ljharb You mentioned that you're also having this same problem. Have you encountered it in any situations which don't involve a LogicalExpression right before the opening element (e.g. foo && <Bar></Bar>?

(If you have a codebase with a lot of errors from this issue, one fast way to check could be to temporarily apply that diff and see if any errors remain.)

[ljharb]

That’s the only scenario I’ve seen it in, but i can’t really survey 10k+ files :-)

I’ll try it locally as well.