max-len: comments: ignores JSX

[avalanche1]

Tell us about your environment

• ESLint Version:
  ^6.1.0
• Node Version:
  12.9.0
• npm Version:
  6.9.0
  What parser (default, Babel-ESLint, etc.) are you using?
  "@typescript-eslint/parser": ^2.0.0
  Please show your full configuration:

Configuration

env:
  #https://medium.com/@jzarzoso/setting-eslint-on-meteor-for-react-development-10af658b44e2
  browser: true
  jest/globals: true
extends:
  - "eslint:recommended"
  - "plugin:@typescript-eslint/recommended"
  - "plugin:@typescript-eslint/eslint-recommended"
  - "hardcore-fp"
  - "plugin:jest/recommended"
  - "plugin:react/recommended"
  - "prettier"
  - "prettier/@typescript-eslint"
parser: "@typescript-eslint/parser"
parserOptions:
#  project: "./tsconfig.json"
  ecmaVersion: "2019"
  sourceType: module
plugins:
  - "@typescript-eslint/eslint-plugin"
  - jest
  - react
  - react-hooks
  - promise
overrides:
  - files:
      - "*.test.js"
      - "*.spec.js"
    rules:
      fp/no-unused-expression: "off"
rules:
  "@typescript-eslint/camelcase": off
  "@typescript-eslint/explicit-function-return-type":
    - error
    - allowExpressions: true
  "@typescript-eslint/interface-name-prefix":
    - error
    - prefixWithI: always
  "@typescript-eslint/no-use-before-define": off
  arrow-body-style: "off"
  camelcase: off
  comma-dangle: "off"
  fp/no-mutating-methods:
    - error
    # Allow reassign Lodash methods
    - allowedObjects:
        - _
  fp/no-mutation:
    - error
    # Allow module.exports
    - commonjs: true
      allowThis: false
      # Allow assigning to window global
      exceptions:
        - object: window
#  fp/no-nil: "off"
  fp/no-unused-expression: "off"
  func-style: warn
  id-length:
    - warn
    - exceptions:
        - e
        - f
        - i
        - v
        - x
        - "y"
        - z
  id-match:
    - error
    - "^[a-zA-Z_]*$"
    - properties: true
  indent: off
  lines-around-comment:
    - warn
    - ignorePattern: /prettier-ignore/
  max-len:
    - warn
    - code: 90
      comments: 120
      tabWidth: 2
      ignorePattern: "import"
      ignoreUrls: true
  max-params:
    - error
    - 6
  multiline-ternary: "off"
  new-cap: off
  no-inline-comments: "off"
  no-magic-numbers:
    - error
    - ignore:
        - 0
        - 1
  no-multi-spaces: "off"
  no-return-await: off
  no-param-reassign:
    - error
    - props: true
  no-nested-ternary: "off"
  no-shadow:
    - error
    - builtinGlobals: true
  no-warning-comments: warn
  no-undef: error
  no-unneeded-ternary: "off"
  no-unused-vars: warn
  no-use-before-define: "off"
  object-curly-newline: "off"
  one-var: off
  operator-linebreak:
    - error
    - before
    - overrides:
        =: ignore
  padding-line-between-statements: "off"
  radix: "off"
  react/no-string-refs:
    - error
    - noTemplateLiterals: true
  react-hooks/rules-of-hooks: error
  react-hooks/exhaustive-deps: warn
  react/display-name: "off"
  react/prop-types: "off"
  semi: warn
settings: {}

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

export function ScoringResultsItem() {
  return (
    <List.Item>
      <Checkbox checked={state.checked}>
        <Avatar src={bankDatum.logoURL} />
        {/* Some really really really really really really really really really long comment */}
        <a href={bankDatum.PAURL} target="_blank">
          {bankDatum.name}
        </a>
      </Checkbox>
    </List.Item>
  );
}

eslint foo.js

What did you expect to happen?
no warnings logged

What actually happened? Please include the actual, raw output from ESLint.
6:1 warning This line has a length of 96. Maximum allowed is 90 max-len

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

[platinumazure]

Hi @avalanche1, thanks for the issue.

Is the issue that we ignore JSX, or that comments are not ignored if they are not the last/only token on the line?

If my hypothesis is correct, then this example should not warn:

export function ScoringResultsItem() {
  return (
    <List.Item>
      <Checkbox checked={state.checked}>
        <Avatar src={bankDatum.logoURL} />
        {
        /* Some really really really really really really really really really long comment */
        }
        <a href={bankDatum.PAURL} target="_blank">
          {bankDatum.name}
        </a>
      </Checkbox>
    </List.Item>
  );
}

In this case though, maybe the issue is that it's impossible to write a comment in JSX without going into code block mode first? And that the best way to do that with minimum ugliness is to have the braces on the same line? (Note: I don't know JSX very well, please bear with me.)

If I've understood the situation correctly, then I think we can consider an enhancement here (not a bug fix, as according to my understanding, the rule is currently working as designed).

[kaicataldo]

@platinumazure Taking a quick look at the code, that seems correct!

I do think it makes sense for us to account for the opening and closing brackets when checking comments using max-len. i.e. it would allow

{
  /* Some really really really really really really really really really long comment */
}

{/* Some really really really really really really really really really long comment */}

I think it should only ignore/account for the brackets when the brackets only contain a comment, though - otherwise, it should behave as it does now.

[avalanche1]

In my understanding, for the moment Eslint treats JSX comment construct (i.e. {/*comment*/} or {//comment} as code, not as comments.
That's why the false positives.

[kaicataldo]

@avalanche1 That's right - just for clarity though, {//comment} is invalid because the closing bracket is parsed as part of the line comment value.

[kaicataldo]

This issue looks similar to #11270. I wonder if we can standardize around what we consider a "line that only contains a comment" in JSX across our rules.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[kaicataldo]

I am championing this as a semver-minor bug fix to the default behavior (citing the precedence set here).

[btmills]

👍 I agree that this can be a semver-minor change, and that generally speaking we should consider the curlies part of the comment boundary in JSX. Should we also require that the curlies contain only a comment? For example, I would think the first case below should be valid but not the second:

<div>
    {/* this long comment should not violate max-len ignoring comments */}
    {42 /* this long comment that contains code inside the curlies should be invalid */}
</div>

[kaicataldo]

@btmills Totally agreed! Sorry I wasn't clearer on this point.

[btmills]

Ah I see where I missed that you had already written that. My fault!

[mdjermanovic]

This is accepted now.