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

Bug: no-underscore-dangle not working as expected in ES2022 public/private class fields #15810

Closed
1 task done
robertotcestari opened this issue Apr 26, 2022 · 4 comments · Fixed by #15818
Closed
1 task done
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly new syntax This issue is related to new syntax that has reached stage 4 repro:yes rule Relates to ESLint's core rules
Projects

Comments

@robertotcestari
Copy link
Contributor

robertotcestari commented Apr 26, 2022

Environment

Node version: 16.13
npm version: 8.1.2
Local ESLint version: 8.14.0
Global ESLint version: -
Operating System: macOS m1 pro

What parser are you using?

Default (Espree)

What did you do?

Configuration
{
  "extends": "eslint:recommended",
  "env": {
    "browser": true,
    "node": true,
    "es6": true
  },
  "parserOptions": {
    "ecmaVersion": 2022
  },
  "rules": {
    "no-underscore-dangle": [
      2,
      {
        "enforceInMethodNames": true
      }
    ]
  }
}

What did you expect to happen?

The rule no-underscore-dangle should also apply to the new ES2022 public/private class fields.
The implementation was marked done #14857 , however, it still does not work, as no errors are shown in public/private class fields.

What actually happened?

Errors were raised only for methods in the class (and properties in the constructor), but not for methods or properties defined via public/private class fields syntax.

image

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

No response

@robertotcestari robertotcestari added bug ESLint is working incorrectly repro:needed labels Apr 26, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Apr 26, 2022
@robertotcestari robertotcestari changed the title Bug: (fill in) Bug: no-underscore-dangle not working as expected in ES2022 public/private class fields Apr 26, 2022
@robertotcestari robertotcestari changed the title Bug: no-underscore-dangle not working as expected in ES2022 public/private class fields Bug: no-underscore-dangle not working as expected in ES2022 public/private class fields Apr 26, 2022
@btmills
Copy link
Member

btmills commented Apr 27, 2022

I was able to repro this in the demo (or the new demo!).

/* eslint no-underscore-dangle: ["error", { "enforceInMethodNames": true }] */

class Class {
  _publicField = "false negative";
  #_privateField = "false negative";
  
  _publicMethod() { return "true positive"; }
  #_privateMethod() { return "true positive"; }

  constructor() {
    this._publicField = "true positive";
    this.#_privateField = "true positive";
  }
}

In my opinion, class fields are new enough that we could fix this in a semver-minor release. If the team disagrees, the alternative is putting this check behind an option just like enforceInMethodNames, so potentially enforceInClassFields, which defaults to false to maintain compatibility with the current behavior and then default it to true in the next major release.

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion repro:yes rule Relates to ESLint's core rules new syntax This issue is related to new syntax that has reached stage 4 and removed repro:needed labels Apr 27, 2022
@robertotcestari
Copy link
Contributor Author

The option enforceInClassFields is a great suggestion, @btmills . There is this #15811, but i am happy to submit another code with the enforceInClassFields option property.

@mdjermanovic
Copy link
Member

Since this is a stylistic rule, and it already doesn't check all identifiers (for example, it allows property names in object literals), I think we should add this new check behind an option to maintain compatibility with the current behavior.

enforceInClassFields (default false) sounds good to me.

@robertotcestari
Copy link
Contributor Author

Hi @btmills @mdjermanovic, just submitted a new PR (#15818) with

  • New option enforceInClassFields (default false)
  • Test cases
  • Changes in documentation where appropriate.

This should fix and address your suggestions.

@mdjermanovic mdjermanovic linked a pull request Apr 28, 2022 that will close this issue
1 task
@mdjermanovic mdjermanovic moved this from Needs Triage to Pull Request Opened in Triage Apr 28, 2022
Triage automation moved this from Pull Request Opened to Complete May 4, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 1, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly new syntax This issue is related to new syntax that has reached stage 4 repro:yes rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
3 participants