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

indent rule's ignoredNodes not respected when multiline #9882

Comments

@bawjensen
Copy link

@bawjensen bawjensen commented Jan 24, 2018

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.
@eslint eslint bot added the triage label Jan 24, 2018
@bawjensen

This comment has been minimized.

Copy link
Author

@bawjensen bawjensen commented Jan 24, 2018

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:

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

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Jan 24, 2018

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

This comment has been minimized.

Copy link
Author

@bawjensen bawjensen commented Jan 24, 2018

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

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Feb 1, 2018

@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

This comment has been minimized.

Copy link
Member

@platinumazure platinumazure commented Feb 1, 2018

@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

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Feb 1, 2018

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

This comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 2, 2018

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 comment has been minimized.

Copy link
Member

@not-an-aardvark not-an-aardvark commented Feb 2, 2018

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

This comment has been minimized.

Copy link
Contributor

@ljharb ljharb commented Feb 2, 2018

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.

This was referenced Mar 22, 2018
@eslint eslint bot locked and limited conversation to collaborators Aug 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.