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

Fixing formatting on higher-order function with template string loses indentation #241

Open
timdp opened this issue Jul 20, 2022 · 5 comments

Comments

@timdp
Copy link

timdp commented Jul 20, 2022

What version of this package are you using?

v17.0.0

What operating system, Node.js, and npm version?

Node v18.6.0, npm v8.13.2, ESLint v8.20.0, prettier-eslint-cli v6.0.1

What happened?

I apologize in advance if I'm in the wrong place. My (current) best guess is that there's a bug with how this package interacts with ESLint.

I'm using prettier-eslint-cli to format my code. I'm using the config provided in this package's readme and calling prettier-eslint --write index.js. At first, I thought this was a prettier-eslint issue, but I found out that it happens when prettier-eslint calls ESLint with the Standard config (and { fix: true }).

So here goes! When I try to format this code:

module.exports =
  (arg1, arg2, arg3, arg4, arg5, arg6, arg7) =>
    (arg1, arg2, arg3, arg4, arg5, arg6, arg7) => {
      console.info(
        `I want this to go on a new line so I am making it long, it's ${true}`
      )
    }

(which is already properly formatted), the argument to console.info loses its indentation:

      console.info(
      `I want this to go on a new line so I am making it long, it's ${true}`
      )

What did you expect to happen?

The argument should be one nesting level deeper.

Interestingly, it appears to be related to template strings. If I use a regular string, the indentation is as intended. Additionally, ESLint itself complains about indentation if I use a regular string but not if I use a template string.

I assume the issue is either with one of the underlying rules or with ESLint itself, but I haven't dug that deep. What I do know is that ESLint with defaults formats the code more sanely:

module.exports =
  (arg1, arg2, arg3, arg4, arg5, arg6, arg7) =>
  (arg1, arg2, arg3, arg4, arg5, arg6, arg7) => {
    console.info(
      `I want this to go on a new line so I am making it long, it's ${true}`
    );
  };

This is why I'm filing it as an issue with the Standard configuration.

My best guess is that it's related to a difference in how higher-order (arrow) functions are handled, since by default, ESLint doesn't indent those as much.

I went through the names of all the rules that Standard applies but couldn't identify the one that governs this. I did try to apply the config for the indent rule separately but it didn't seem relevant.

Are you willing to submit a pull request to fix this bug?

If you tell me where to look. 🙂

@voxpelli
Copy link
Member

Have you tried reproducing this with a straight up standard setup? Without any prettier parts?

Which version of standard?

@timdp
Copy link
Author

timdp commented Jul 20, 2022

standard --fix doesn't change indentation. So is this purely governed by Prettier? I've been using them in tandem for so long that I'm hazy on which one does what, to be honest.

Standard itself is at v17.0.0 as well.

@timdp
Copy link
Author

timdp commented Jul 20, 2022

From instrumenting the code a bit, I can see that eslint --fix explicitly decides to indent a regular string:

{
  token: Token {
    type: 'String',
    value: "'I want this to go on a new line so I am making it long, it is true'",
    start: 360,
    end: 428,
    loc: SourceLocation { start: [Position], end: [Position] },
    range: [ 360, 428 ]
  },
  numSpaces: 6,
  actualIndent: [ ' ', ' ', ' ', ' ', ' ', ' ' ]
}

But with a template string, there's no suggested fix. I assume that's why it inherits the indentation level of the console.log call around it, which is wrong.

... but interestingly, it does seem connected to the fact that there's a closure, and that its argument list is so long that it hits the next line. As soon as one of those conditions is no longer true, the template string gets indented.

@timdp
Copy link
Author

timdp commented Jul 20, 2022

So I went further down the rabbit hole and found out that it is an option specific to Standard that's triggering the issue:

"ignoredNodes": ["TemplateLiteral *", "JSXElement", "JSXElement > *", "JSXAttribute", "JSXIdentifier", "JSXNamespacedName", "JSXMemberExpression", "JSXSpreadAttribute", "JSXExpressionContainer", "JSXOpeningElement", "JSXClosingElement", "JSXFragment", "JSXOpeningFragment", "JSXClosingFragment", "JSXText", "JSXEmptyExpression", "JSXSpreadChild"],

Ignoring TemplateLiteral * excludes the template string node and breaks indentation.

This ignore was originally released in Standard v14.0.0 for standard/standard#1176, in an attempt to fix a bug in ESLint. It was subsequently relaxed in v14.0.2 with standard/standard#1385. However, it apparently has undesired side effects.

@feross You originally added the exclusion. Do you have an opinion on this?

@timdp
Copy link
Author

timdp commented Apr 5, 2023

Just documenting my ugly workaround for posterity:

// .eslintrc.cjs

const standard = require('eslint-config-standard')

const indent = [...standard.rules.indent]
indent[2] = {
  ...indent[2],
  ignoredNodes: indent[2].ignoredNodes.filter(
    (expr) => !expr.startsWith('Template')
  )
}

module.exports = {
  extends: 'standard',
  rules: {
    indent
  }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

2 participants