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

update eslint:recommended and eslint-config-eslint. #14673

Closed
aladdin-add opened this issue Jun 5, 2021 · 8 comments · Fixed by #14691
Closed

update eslint:recommended and eslint-config-eslint. #14673

aladdin-add opened this issue Jun 5, 2021 · 8 comments · Fixed by #14691
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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint

Comments

@aladdin-add
Copy link
Member

aladdin-add commented Jun 5, 2021

The version of ESLint you are using.
latest

The problem you want to solve.
update eslint:recommended and eslint-config-eslint.

Your take on the correct solution to problem.

  • y: enable
  • n: not enable
  • ?: not sure
Name Added in eslint version eslint:recommended eslint-config-eslint Description
default-case-last 7.0.0-alpha.0 ? y enforce default clauses in switch statements to be last
default-param-last 6.4.0 ? y enforce default parameters to be last
function-call-argument-newline 6.2.0 n y('consistent') enforce line breaks between arguments of a function call
grouped-accessor-pairs 6.7.0 n y require grouped accessor pairs in object literals and classes
id-denylist 7.4.0 n n disallow specified identifiers
no-constructor-return 6.7.0 y y disallow returning value from constructor
*no-dupe-else-if 6.7.0 y y disallow duplicate conditions in if-else-if chains
*no-import-assign 6.4.0 y y disallow assigning to imported bindings
no-loss-of-precision 7.1.0 y y disallow literal numbers that lose precision
no-nonoctal-decimal-escape 7.14.0 y y disallow \8 and \9 escape sequences in string literals
no-promise-executor-return 7.3.0 y y disallow returning values from Promise executor functions
no-restricted-exports 7.0.0-alpha.0 n n disallow specified names in exports
*no-setter-return 6.7.0 y y disallow returning values from setters
no-unreachable-loop 7.3.0 y y disallow loops with a body that allows only one iteration
no-unsafe-optional-chaining 7.15.0 y y disallow use of optional chaining in contexts where the undefined value is not allowed
no-useless-backreference 7.0.0-alpha.0 y y disallow useless backreferences in regular expressions
prefer-exponentiation-operator 6.7.0 n y disallow the use of Math.pow in favor of the ** operator
prefer-regex-literals 6.4.0 n y disallow use of the RegExp constructor in favor of regular expression literals

is it too late to consider adding this to eslint v8.0.0? �:-( 

@aladdin-add aladdin-add added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 5, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jun 5, 2021
@aladdin-add aladdin-add removed this from Needs Triage in Triage Jun 5, 2021
@aladdin-add aladdin-add added this to Needs Triage in Triage via automation Jun 5, 2021
@aladdin-add aladdin-add moved this from Needs Triage to Feedback Needed in Triage Jun 5, 2021
@nzakas
Copy link
Member

nzakas commented Jun 8, 2021

It’s not clear to me what exactly you are suggesting should change. Are you saying that rules with “y” in the eslint:recommended table should be added? I’m just not sure how to read this table.

Keep in mind that recommended rules must flag potential errors or suggest better ways of doing things. We can’t recommend any rules that are stylistic.

@aladdin-add
Copy link
Member Author

Are you saying that rules with “y” in the eslint:recommended table should be added?

yes! updated the issue to make it clearer.

@nzakas
Copy link
Member

nzakas commented Jun 9, 2021

Thanks, that helps. I think a lot of these rules fall under the category of stylistic, so I’m not in favor of adding them to eslint:recommended.

The ones I think make sense to add are:

  • no-import-assign
  • no-loss-of-precision
  • no-nonoctal-decimal-escape
  • no-promise-executor-return
  • no-setter-return
  • no-unreachable-loop
  • no-unsafe-optional-chaining
  • no-useless-backreference

I don’t care too much about eslint-config-eslint. Historically, we have tried to enable as many rules as possible so we are constantly testing them. As such, I’m fine with adding whatever people want.

@mdjermanovic
Copy link
Member

I vote for these four:

  • no-loss-of-precision
  • no-unsafe-optional-chaining
  • no-useless-backreference
  • no-nonoctal-decimal-escape

The first three flag errors in the code, and I can't think of any reason to not enable each.

no-nonoctal-decimal-escape flags the use of a legacy syntax that can be easily avoided.


no-unreachable-loop

This can be problematic because by default it flags a pattern that checks if an object has enumerable properties:

for (const key in obj) {
  has = true;
  break;
}

And a pattern to check if there's anything in a sequence:

for (const value of iterable) {
  has = true;
  break;
}

no-promise-executor-return

There are some patterns that would be reported by this rule:


Rest of the rules marked as "?" or "y":

  • default-case-last: while very uncommon, the code it flags is valid.
  • default-param-last: this is only a best practice, the code it flags is valid.
  • no-constructor-return: I believe most codebases should enable this rule to catch errors in the code, but there are valid patterns where constructor returns a value.
  • no-dupe-else-if: this rule is already in eslint:recommended
  • no-import-assign: this rule is already in eslint:recommended
  • no-setter-return: this rule is already in eslint:recommended

@nzakas
Copy link
Member

nzakas commented Jun 10, 2021

Happy to go along with @mdjermanovic’s suggestions.

@aladdin-add
Copy link
Member Author

aladdin-add commented Jun 10, 2021

sounds good!

I can make 2 PRs: one for the eslint:recommended; one for eslint-config-eslint (if no objections).
cc @btmills @snitin315

@snitin315
Copy link
Contributor

Sounds good to me 👍🏻

@btmills
Copy link
Member

btmills commented Jun 10, 2021

I also agree with @mdjermanovic for eslint:recommended. For eslint-config-eslint, similar to @nzakas I’m fine trying to enable the ones you suggested, and we can disable one if it’s too difficult to adopt.

Marking as accepted and adding to the v8.0.0 roadmap since we’re all in favor.

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 10, 2021
@btmills btmills moved this from Feedback Needed to Ready to Implement in Triage Jun 10, 2021
@btmills btmills added this to Planned in v8.0.0 Jun 10, 2021
aladdin-add added a commit to aladdin-add/eslint that referenced this issue Jun 10, 2021
* no-loss-of-precision
* no-unsafe-optional-chaining
* no-useless-backreference
* no-nonoctal-decimal-escape
@aladdin-add aladdin-add moved this from Planned to Pull Request Opened in v8.0.0 Jun 10, 2021
@nzakas nzakas linked a pull request Jun 17, 2021 that will close this issue
1 task
@nzakas nzakas moved this from Pull Request Opened to Ready for Merge in v8.0.0 Jun 17, 2021
Triage automation moved this from Ready to Implement to Complete Aug 5, 2021
v8.0.0 automation moved this from Ready for Merge to Done Aug 5, 2021
nzakas pushed a commit that referenced this issue Aug 5, 2021
* no-loss-of-precision
* no-unsafe-optional-chaining
* no-useless-backreference
* no-nonoctal-decimal-escape
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 2, 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 Feb 2, 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 breaking This change is backwards-incompatible core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Archived in project
v8.0.0
  
Done
Triage
Complete
Development

Successfully merging a pull request may close this issue.

5 participants