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

Incorrect indentation reported from 7.29.0 #3218

Closed
quiram opened this issue Feb 25, 2022 · 9 comments
Closed

Incorrect indentation reported from 7.29.0 #3218

quiram opened this issue Feb 25, 2022 · 9 comments

Comments

@quiram
Copy link

quiram commented Feb 25, 2022

We have a particular issue after upgrading eslint-plugin-react to 7.29.0 from 7.28.0: indentation doesn't seem to be counted correctly in some cases, resulting in false positives. Take this code snippet:

    case 'multiChoice':
      for (let i = 0; i < obj.options.length; i += 1) {
        options[i] = new Option(obj.options[i].description, parse(obj.options[i].option));
      }
      return new MultiChoiceNode(obj.question, // This is line 34 in the original file
        obj.questionKey,
        options,
        parse(obj.singleOpening),
        parse(obj.multipleOpening),
        parse(obj.singleClosing),
        parse(obj.multipleClosing));

Running the linter on this file with 7.28.0 causes no problem, but from 7.29.0 it results in the following error:

TemplateParser.js:34:7: Expected indentation of 6 space characters but found 8. [Error/react/jsx-indent]

Note that the indentation in line 34 is indeed 6 spaces, not 8 as the error message reports.

@quiram quiram changed the title Incorrect number of indentation reported from 7.29.0 Incorrect indentation reported from 7.29.0 Feb 25, 2022
@job13er
Copy link

job13er commented Feb 25, 2022

I've noticed the same issue. I was just trying to create a simple repo to reproduce the issue, but haven't had much luck yet. I did a little digging to see what changed, and found it was #620 which fixed an issue with aligning the closing parens in jsx return statements, but seems to also be flagging correct non-jsx returns as incorrect. I suspect the issue is related to this change, and the fact that non-jsx functions are being evaluated there as well.

Screen Shot 2022-02-25 at 5 34 27 AM

@job13er
Copy link

job13er commented Feb 25, 2022

@quiram just to clarify, despite the line number in the error saying 34, the error is actually complaining because line 40 is not also indented at 6. I believe what you're seeing is what I was seeing, the rule is treating your return as if it were returning JSX where the closing paren should line up with the return statement.

A better description for this issue might be "jsx-indent reporting error on non-jsx return"

Update: actually, you can probably resolve your issue just by formatting that return statement differently:

      return new MultiChoiceNode(
        obj.question,
        obj.questionKey,
        options,
        parse(obj.singleOpening),
        parse(obj.multipleOpening),
        parse(obj.singleClosing),
        parse(obj.multipleClosing)
      );

My problem is that I'm returning a promise chain and so the closing indent is really supposed to be different than the opening indent.

@job13er
Copy link

job13er commented Feb 25, 2022

Sorry it took me a minute, but here's a simple repo that reproduces the issue, if you change the package.json to pin to 7.28.0 the error no longer appears.

https://github.com/job13er/jsx-indent-bug

@quiram
Copy link
Author

quiram commented Feb 25, 2022

Thanks @job13er for looking at this so quickly and for providing the sample repo, I tried for a bit but JavaScript is not my forte (minor thing: the repo says 7.49.0, but I think it's 7.29.0).

Update: actually, you can probably resolve your issue just by formatting that return statement differently:

Odd, I tried that and the linter just put the closing parenthesis back to the previous line. However, I think your theory makes sense: another thing that I tried was to increase the indentation of the whole block so the first line does indeed have 8 spaces of indentation, and when running eslint with autofix it would only fix the last line, so there is something definitely there.

For now what I'm doing is skipping 7.29.0 by modifying my dependency definition like this:

"eslint-plugin-react": ">7.20.0 <7.29.0 || ^7.30.0",

@job13er
Copy link

job13er commented Feb 25, 2022

@quiram You're quite right, I fixed the reference in the repo :)

I'm just pinning to 7.28.0 explicitly as a workaround in the meantime. Just to point it out, your versioning there assumes that a future 7.30.0 will resolve the issue. I'm not sure that's a safe assumption.

@quiram
Copy link
Author

quiram commented Feb 25, 2022

@job13er you're right that 7.30.0 may not fix the problem, but if it doesn't I'll find out soon enough 😄
We have a nightly build to update to the latest minor version of all our npm dependencies; if 7.30.0 fixes it then we're sorted, if not then I'll bump the version.

@ljharb
Copy link
Member

ljharb commented Feb 25, 2022

It wouldn't be 7.30.0, since that's a minor version; i'll fix it in v7.29.1.

@ljharb
Copy link
Member

ljharb commented Feb 25, 2022

v7.29.1 is released.

@job13er
Copy link

job13er commented Feb 25, 2022

Thanks for the quick turnaround! :)

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

3 participants