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

Changes to the recommended sets for 5.0.0 #3746

Closed
Tracked by #23
bradzacher opened this issue Aug 16, 2021 · 9 comments
Closed
Tracked by #23

Changes to the recommended sets for 5.0.0 #3746

bradzacher opened this issue Aug 16, 2021 · 9 comments
Labels
breaking change This change will require a new major version to be released package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin recommended-rules Discussion about recommended rule sets
Milestone

Comments

@bradzacher
Copy link
Member

bradzacher commented Aug 16, 2021

Similar to what I did for 3.0.0 (in #1423), I'm putting forward the new recommended set ahead of time.
I'm looking for feedback from the community before we go ahead and make the changes.

Hint: search for the text (new) to find the changes

Header Key
  • New = new since version 3 - the last time we updated the recommended sets
  • Ext = extension rule
  • Dep = deprecated
    • ☑️ = deprecated in the next major
    • 🗑️ = to be removed from the plugin in the next version
  • R = recommended
    • ⚠️ = recommended as warning
    • 🛑 = recommended as an error
    • 🗑️ = remove from recommended this version
  • RWT = recommended with typechecking
    • ⚠️ = recommended as warning
    • 🛑 = recommended as an error
    • 🗑️ = remove from recommended this version
Rule New Ext Dep R RWT Comment
adjacent‑overload‑signatures 🛑
array‑type
await‑thenable 🛑
ban‑ts‑comment 🛑
ban‑tslint‑comment ☑️ There's still 3m weekly downloads for TSLint - too many users to make this recommended for now.
ban‑types 🛑 We should remove object from the default config, because enough people want to use it and disagree with the reasoning.
brace‑style ☑️
class‑literal‑property‑style
comma‑dangle ☑️ ☑️ Not in ESLint's recommended.
comma‑spacing ☑️
consistent‑indexed‑object‑style ☑️ Stylistic.
consistent‑type‑assertions
consistent‑type‑definitions
consistent‑type‑imports ☑️ Stylistic.
default‑param‑last ☑️
dot‑notation ☑️
explicit‑function‑return‑type
explicit‑member‑accessibility
explicit‑module‑boundary‑types 🗑️ (new) A lot of people are against this style and turn the rule off. It's entirely stylistic so we shouldn't recommend it.
func‑call‑spacing ☑️
indent ☑️
init‑declarations ☑️
keyword‑spacing ☑️
lines‑between‑class‑members ☑️
member‑delimiter‑style
member‑ordering
method‑signature‑style
naming‑convention
no‑array‑constructor ☑️ 🛑
no‑base‑to‑string
no‑confusing‑non‑null‑assertion ☑️ Stylistic.
no‑confusing‑void‑expression ☑️ It's a good rule, but I think in general the errors it prevents will just be caught by the typechecker.
no‑dupe‑class‑members ☑️
no‑duplicate‑imports ☑️ ☑️ Stylistic.
no‑dynamic‑delete
no‑empty‑function ☑️ 🛑
no‑empty‑interface 🛑
no‑explicit‑any ⚠️
no‑extra‑non‑null‑assertion 🛑
no‑extra‑parens ☑️
no‑extra‑semi ☑️ 🛑
no‑extraneous‑class
no‑floating‑promises 🛑
no‑for‑in‑array 🛑
no‑implicit‑any‑catch ☑️ ☑️ (new) TS 4.4 now has a compiler flag for this. So it's now a rule we don't need! Because 4.4 is still in RC, we'll just deprecate it, and remove it in the next major.
no‑implied‑eval ☑️ 🛑
no‑inferrable‑types 🛑
no‑invalid‑this ☑️
no‑invalid‑void‑type
no‑loop‑func ☑️ ☑️ Not in ESLint's recommended.
no‑loss‑of‑precision ☑️ ☑️ 🛑 (new) ESLint added this to their recommended set.
no‑magic‑numbers ☑️
no‑misused‑new 🛑
no‑misused‑promises 🛑
no‑namespace 🛑
no‑non‑null‑asserted‑optional‑chain 🛑
no‑non‑null‑assertion ⚠️
no‑parameter‑properties
no‑redeclare ☑️ ☑️ Recommended by ESLint, but we disable it in our eslint-recommended set.
no‑require‑imports
no‑shadow ☑️ ☑️ Not in ESLint's recommended.
no‑this‑alias 🛑
no‑throw‑literal ☑️
no‑type‑alias
no‑unnecessary‑boolean‑literal‑compare
no‑unnecessary‑condition
no‑unnecessary‑qualifier
no‑unnecessary‑type‑arguments
no‑unnecessary‑type‑assertion 🛑
no‑unnecessary‑type‑constraint ☑️ 🛑 (new) Helps remove redundant code.
no‑unsafe‑argument ☑️ 🛑 (new) Same reasoning as the other no‑unsafe-* rules.
no‑unsafe‑assignment 🛑
no‑unsafe‑call 🛑
no‑unsafe‑member‑access 🛑
no‑unsafe‑return 🛑
no‑unused‑expressions ☑️
no‑unused‑vars‑experimental 🗑️ (new) no‑unused‑vars now completely supports TS - so this rule is no longer required.
no‑unused‑vars ☑️ ⚠️
no‑use‑before‑define ☑️
no‑useless‑constructor ☑️
no‑var‑requires 🛑
non‑nullable‑type‑assertion‑style
object‑curly‑spacing ☑️ ☑️ Not in ESLint's recommended.
prefer‑as‑const 🛑
prefer‑enum‑initializers ☑️ Stylistic.
prefer‑for‑of
prefer‑function‑type
prefer‑includes
prefer‑literal‑enum‑member ☑️ Stylistic.
prefer‑namespace‑keyword 🛑
prefer‑nullish‑coalescing
prefer‑optional‑chain
prefer‑readonly‑parameter‑types
prefer‑readonly
prefer‑reduce‑type‑parameter
prefer‑regexp‑exec 🗑️ (new) This is a stylistic rule and enough people have raised issues about its premise being wrong.
prefer‑return‑this‑type ☑️ Stylistic.
prefer‑string‑starts‑ends‑with
prefer‑ts‑expect‑error
promise‑function‑async
quotes ☑️
require‑array‑sort‑compare
require‑await ☑️
restrict‑plus‑operands 🛑
restrict‑template‑expressions 🛑
return‑await ☑️ 🛑
semi ☑️
sort‑type‑union‑intersection‑members ☑️ Stylistic.
space‑before‑function‑paren ☑️
space‑infix‑ops ☑️ ☑️ Not in ESLint's recommended.
strict‑boolean‑expressions
switch‑exhaustiveness‑check
triple‑slash‑reference 🛑
type‑annotation‑spacing
typedef
unbound‑method 🛑
unified‑signatures
@bradzacher bradzacher added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin breaking change This change will require a new major version to be released recommended-rules Discussion about recommended rule sets and removed package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Aug 16, 2021
@pgsandstrom
Copy link

I would prefer no‑inferrable‑types to be removed from the recommended list. I think it is debatable if it can be considered a best practice. Especially when I run into code such as this, where the rule just wants to make the code less uniform:

function test(
  a: number,
  b: number,
  c: number = 1,
  d: number = 1
) {
  //
}

Also consider issues such as #3145 and the rule can be quite annoying. The recommended list should be reserved for rules that are overwhelmingly more useful than annoying, and I dont think this rule makes the cut.

@joealden
Copy link

@bradzacher to clarify, this is for 5.0.0, not 4.0.0 (as we are already on version 4.x.x)? If so, then the title of this issue should probably be updated, and this issue should probably be added to the 5.0.0 milestone.

@bradzacher bradzacher changed the title Changes to the recommended sets for 4.0.0 Changes to the recommended sets for 5.0.0 Aug 20, 2021
@bradzacher bradzacher added this to the 5.0.0 milestone Aug 20, 2021
@bradzacher
Copy link
Member Author

@joealden - ooops, thanks for that. We didn't change the sets in 4.0 so when I copy pasted my old issue from 3.0 I just incremented the version instead of thinking 🤦

@pgsandstrom perhaps we should instead add a new option to ignore function parameters and set it as the default - would that address your concerns with it being recommended?

@pgsandstrom
Copy link

I think that would improve the rule, but my personal opinion is still that the value it provides is a bit too opinionated and limited to be part of the recommended set.

@joealden
Copy link

To put in my 2 cents regarding no‑inferrable‑types, I've personally never found this rule annoying, and I personally think it should remain in the recommended rule set, as in my experience and opinion, it leads to more readable code 99% of the time.


Regarding a rule that is semi-related to no‑inferrable‑types, I almost always turn off explicit-module-boundary-types when using the recommended rule set, especially when I'm writing code that will never be published on NPM. My thinking regarding this rule is that it is useful for values that are accessible via a packages public API (as it prevents one from as easily making a breaking change without realizing it), but for modules that aren't exposed via a public API (either modules completely internal to a public package, or modules that exist within a codebase that isn't a library), I think this is overkill, as any module boundary type changes will be picked up the TypeScript compiler itself within the project.

I'm not saying that I think we should 100% remove this rule for the recommended preset, but in my experience, I turn it off most of the time, so I'm wondering what others think regarding this rule.

@JoshuaKGoldberg
Copy link
Member

I almost always turn off explicit-module-boundary-types

+1 to this, I don't think I've ever enabled that rule.

@pgsandstrom
Copy link

I also always turn off explicit-module-boundary-types and it has been turned off at all major projects I've been working on for the last few years. I wouldn't mind it being removed from the recommended preset.

@bradzacher
Copy link
Member Author

For me the point of the explicit-module-boundary-types rule is two fold:

First - readability. It means you don't have to rely on intellisense to figure out what a function is or does. This makes it easier to do reviews without an IDE. Code is read more times than it's written so IMO optimising for readability is paramount.

Second - it allows you to catch problems sooner.
By default TypeScript in the IDE does not check files you don't have open. So that means if you change the exports of file A, you don't find out that file B is broken until you manually run your build.

In some projects this is fine because you always have a webpack build in watch mode or something, but in others you have no build and must manually run tsc.
So the annotations force you to be explicit at the boundary and thus help catch some problems sooner - speeding the development process.

But I understand that some people find it cumbersome. I disagree with it personally, but I understand that some people like to heavily lean on the checker's inference.

This is one of the ways in which flow is better than TS (IMO). Flow has a mode called types-first which enables significant performance improvements by only analysing the types of a dependency's exports. For types-first you have to annotate the boundary, and it's a hard compiler error if you don't.
But alas - TS doesn't do any such optimisations.


But at this point it's pretty clear that this is a divisive topic and nobody agrees on it. Without some perf improvement from TS - it's not worth pushing the style - especially as there are so many people that are against it / turn it off.

@MichaelDeBoey
Copy link
Contributor

@bradzacher This one can be closes as #3809 & #3818 are merged

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This change will require a new major version to be released package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin recommended-rules Discussion about recommended rule sets
Projects
None yet
Development

No branches or pull requests

5 participants