Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
Closed
Labels
Milestone
Description
Similar to what I did for 2.0.0 (in#651), 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.
First up, let me clarify that this set will be somewhat opinionated. It shall contain a small set of stylistic rules that I believe are a best practice based off of what I've seen in my career, and what the community has converged towards in the past.
eslint-recommended
See#1273
recommended (rules without type information)
Comments about the current config
"@typescript-eslint/adjacent-overload-signatures": "error",- I think this is a best practice.
- Organising overload signatures next to one another makes code a lot easier to understand.
- Verdict: in.
"@typescript-eslint/ban-ts-ignore": "error",- Helps prevent type safety issues.
- This shall be replaced by the
ban-ts-commentcomment rule, which also bansts-nocheck.
"@typescript-eslint/ban-types": "error",- The default config helps standardise the codebase on the consistently named base types, and warns against unsafe types.
- We will be updating the default config in 3.0.0 to make it better, and closer to what people use (see discussion infeat(eslint-plugin): [ban-types] rework default options #848)
- Verdict: in.
"@typescript-eslint/camelcase": "error","@typescript-eslint/class-name-casing": "error","@typescript-eslint/interface-name-prefix": "error",- These rules are hotly contested by some users that are migrating existing codebases to eslint, or just to ts-eslint after only using
eslint:recommended:- Some users believe that there's no point to enforcing naming conventions.
- Some users have legacy code that breaks the rest of the codebase's conventions.
- Some users don't like to enforce camelCase for properties, as their APIs use snake_case.
- My response has always been the same; In my opinion, it's better to enforce some standard than no standard, and camelCase is the standard the vast, vast majority of the JS/TS community uses.
- In particular with new codebases, enforcing a standard from the get-go means that you have a consistent codebase with zero-effort, which reduces diff churn and cognitive load for engineers.
- I do understand that turning this rule on for existing codebases can be a pain, however; with no rule fixer, it is an entirely manual process to either disable the rule for each non-compliant file, or to fix up violations.
- That being said, if you find you don't like it for any reason, it is just a one-line change in your eslintrc to turn it off.
- All of these naming rules are being merged into a single rule,
naming-convention(feat(eslint-plugin): add rule naming-conventions #1318)- As such, they will all be removed from the recommended config, and replaced with
naming-convention. naming-conventionissuper flexible and powerful, so if it doesn't quite fit, it should be easy to reconfigure it to your liking.
- As such, they will all be removed from the recommended config, and replaced with
- These rules are hotly contested by some users that are migrating existing codebases to eslint, or just to ts-eslint after only using
"@typescript-eslint/consistent-type-assertions": "error",- TODO - want to remove this from recommended, and re-add the (deleted)
no-angle-bracket-assertions.
- TODO - want to remove this from recommended, and re-add the (deleted)
"@typescript-eslint/explicit-function-return-type": "warn",- There has been some indirect debate about this rule being recommended, as people see it as a stylistic preference.
- I've always been of the opinion that every function should have a strict contract:
- It helps you catch places where inferred types don't exactly match up with expected.
- It makes code reviews in "dumb" web UIs (i.e. github UI) easier.
- If your project doesn't have
noImplicitReturnsturned on, it can also help you catch code paths that don't return a value.
- A lot of people don't like being forced to typeevery single function return type, which I can understand; for non-exported functions, typing the return may be seen as useless.
- My current thinking for this one is that we replace it with
explicit-module-boundary-types.- This rule isn't as strict, but still enforces you create clear contracts at the module boundary, where it really matters.
- Looking for feedback here.
"@typescript-eslint/member-delimiter-style": "error",- This is a pure stylistic rule; though nobody has directly complained about this one directly.
- In the same vein as naming; I think it's good to enforce some standard rather than no standard.
- Unlike the naming rules, this rule has a fixer which covers all cases, so there isn't a big burden to turning this rule on an existing codebase.
- Verdict: remove.
"@typescript-eslint/no-array-constructor": "error",- I don't really have a strong opinion here. I don't think anyone does - we haven't had any real feedback in either direction.
- Without feedback directly asking for its removal, I'm happy for it to stay in.
- Verdict: in.
"@typescript-eslint/no-empty-function": "error",- Stylistic rule that helps ensure you don't have useless code.
- The argument could be made that this is unnecessary due to bundlers trimming dead code.
- I don't think we've had an argument for its removal.
- Without feedback directly asking for its removal, I'm happy for it to stay in.
- Verdict: in.
"@typescript-eslint/no-empty-interface": "error",- In the same vein as
no-empty-function - The argument could be made that this is unnecessary due to interfaces not being runtime code.
- I don't think we've had an argument for its removal.
- Without feedback directly asking for its removal, I'm happy for it to stay in.
- Verdict: in.
- In the same vein as
"@typescript-eslint/no-explicit-any": "warn",- Best practice. There is almost no case for using
anywith the existance ofunknownandnever. - Verdict: in.
- Best practice. There is almost no case for using
"@typescript-eslint/no-inferrable-types": "error",- Stylistic rule that helps ensure you don't have useless code.
- The argument could be made that this is unnecessary due to interfaces not being runtime code.
- I don't think we've had an argument for its removal.
- Without feedback directly asking for its removal, I'm happy for it to stay in.
- Verdict: in.
"@typescript-eslint/no-misused-new": "error",- Helps catch incorrect usage of
new/constructorwithin classes and interfaces respectively. - Verdict: in.
- Helps catch incorrect usage of
"@typescript-eslint/no-namespace": "error",- Namespaces are, for the most part, considered deprecated syntax. In all new projects, the preferred way is to use import/export syntax to namespace your code and hide internals.
- Especially when used with global declaration merging, namespaces can make it very hard to reason about where code is defined, and how it is used.
- Verdict: in.
"@typescript-eslint/no-non-null-assertion": "warn",- The problem is that this operator is a complete safety hole.
- e.g. It's very easy to add a non-null assertion, and then have someone refactor code higher up, making the assertion incorrect.
- Because the operator is tells typescript "shut up I know what I'm doing", there's no way to catch "incorrect" non-null assertions.
- With the addition of optional chaining and nullish coalescing in TS 3.7, there is almost no need for this operator.
- There are still some dubiously valid use cases, hence it is recommended as a warning.
- Verdict: in.
- The problem is that this operator is a complete safety hole.
"@typescript-eslint/no-this-alias": "error",- Aliasing
thisis an old, old, old, and bad practice. You should just use arrow functions. - Verdict: in.
- Aliasing
"@typescript-eslint/no-unused-vars": "warn",- Base rule is recommended, but our implementation is frought with issues because it relies upon scope analysis, which is currently in a bad state ([meta] Problems with the rules that use scope analysis [no-unused-vars][no-undef][no-shadow][no-redeclare][no-use-before-define] #1856).
- Unsure if we should leave this in or not.Looking for feedback here.
"@typescript-eslint/no-use-before-define": "error",- The errors caught by this rule are generally caught by typescript. There's a few edge cases, but by-and-large, it's covered.
- It also relies upon scope analysis, which is currently in a bad state.
- Verdict: remove.
"@typescript-eslint/no-var-requires": "error",requirestatements are always typed asany, which is a type safety hole.- You should instead use import/export syntax, which will use defined types for the module.
- Verdict: in.
"@typescript-eslint/prefer-namespace-keyword": "error",- Already shouldn't be using namespaces due to
no-namespace, but this rule just makes sure that if you do have to use it, you don't use themodule Foo {}syntax, so it's clear what you're doing.
- Already shouldn't be using namespaces due to
"@typescript-eslint/triple-slash-reference": "error",- This rule ensures you use the import syntax when required instead of reference comments
- Verdict: in.
"@typescript-eslint/type-annotation-spacing": "error",- This is a pure stylistic rule; though nobody has directly complained about this one directly.
- In the same vein as naming; I think it's good to enforce some standard rather than no standard.
- Unlike the naming rules, this rule has a fixer which covers all cases, so there isn't a big burden to turning this rule on an existing codebase.
- Verdict: remove.
"no-var": "error",vardeclarations are error-prone due and often hard to undersand due to scope hoisting. Should just useletandconst.- TypeScript will transpile
let/consttovarfor you if you are targetting an old runtime, so there's no reason to usevar. - Verdict: in.
"prefer-const": "error",- TypeScript will generally do a better job of inferring types if you use
const. - The existence of
consthas recently been debated, but I think that it's better to use it when possible. Though I don't feel strongly in either direction. - Unsure if we should leave this in or not.Looking for feedback here.
- TypeScript will generally do a better job of inferring types if you use
"prefer-rest-params": "error",- using
argumentsis a non-typesafe way of accessing arguments. Using a rest param means you can strictly and clearly declare function inputs and requirements. - TypeScript will transpile a rest param to
argumentsif you are targetting an old runtime, so there's no reason to usearguments. - Verdict: in.
- using
"prefer-spread": "error"- In the same vein, spread arguments is much clearer and safer than using
.apply, becuase you don't have to worry about manually specifying thethiscontext. - TypeScript will transpile a spread argument to
applyif you are targetting an old runtime, so there's no reason to use.apply. - Verdict: in.
- In the same vein, spread arguments is much clearer and safer than using
Comments about the new rules
@typescript-eslint/ban-ts-comment- A better version of
ban-ts-ignore - Verdict: add as an error
- A better version of
@typescript-eslint/brace-style- Stylistic
- Verdict: do not add
@typescript-eslint/class-literal-property-style- Stylistic
- Verdict: do not add
@typescript-eslint/default-param-last- Stylistic
- Verdict: do not add
@typescript-eslint/explicit-module-boundary-types- As mentioned in the previous section, this is a less-strict version of
@typescript-eslint/explicit-function-return-type - Current thinking is that this should be added.
- Looking for feedback here.
- As mentioned in the previous section, this is a less-strict version of
@typescript-eslint/naming-convention- As mentioned in the previous section, this unifies several naming rules.
- However, I don't think it's worthwhile dealing with the fallout from having stylistic rules in the recommended, so this won't be added.
- Verdict: do not add
@typescript-eslint/method-signature-style- Stylistic, but it has some value in making types safer.
- Looking for feedback here.
@typescript-eslint/no-extra-non-null-assertion- avoids useless code, same as
no-extra-semi, which is recommended in the base ruleset. - Verdict: add as error
- avoids useless code, same as
@typescript-eslint/no-extra-semi- We added this extension to support TS features.
- It is recommended in the base ruleset.
- Verdict: add as error
@typescript-eslint/no-non-null-asserted-optional-chain- Obviously this is an error
- Verdict: add as error
@typescript-eslint/prefer-as-const- Stylistic, but just plain better practice
- Verdict: add as warning
@typescript-eslint/quotes- Stylistic
- Verdict: do not add
TL;DR
{ "extends": "./configs/base.json", "rules": { "@typescript-eslint/adjacent-overload-signatures": "error",+ "@typescript-eslint/ban-ts-comment": "error",- "@typescript-eslint/ban-ts-ignore": "error", "@typescript-eslint/ban-types": "error", "camelcase": "off",- "@typescript-eslint/camelcase": "error",- "@typescript-eslint/class-name-casing": "error",- "@typescript-eslint/consistent-type-assertions": "error",- "@typescript-eslint/explicit-function-return-type": "warn",+ "@typescript-eslint/explicit-module-boundary-types": "warn",- "@typescript-eslint/interface-name-prefix": "error",- "@typescript-eslint/member-delimiter-style": "error", "no-array-constructor": "off", "@typescript-eslint/no-array-constructor": "error", "no-empty-function": "off", "@typescript-eslint/no-empty-function": "error", "@typescript-eslint/no-empty-interface": "error", "@typescript-eslint/no-explicit-any": "warn",+ "@typescript-eslint/no-extra-non-null-assertion": "error",+ "@typescript-eslint/no-extra-semi": "error", "@typescript-eslint/no-inferrable-types": "error", "@typescript-eslint/no-misused-new": "error", "@typescript-eslint/no-namespace": "error",+ "@typescript-eslint/no-non-null-asserted-optional-chain": "error", "@typescript-eslint/no-non-null-assertion": "warn", "@typescript-eslint/no-this-alias": "error", "no-unused-vars": "off", "@typescript-eslint/no-unused-vars": "warn",- "no-use-before-define": "off",- "@typescript-eslint/no-use-before-define": "error", "@typescript-eslint/no-var-requires": "error",+ "@typescript-eslint/prefer-as-const": "warn", "@typescript-eslint/prefer-namespace-keyword": "error", "@typescript-eslint/triple-slash-reference": "error",- "@typescript-eslint/type-annotation-spacing": "error", "no-var": "error", "prefer-const": "error", "prefer-rest-params": "error", "prefer-spread": "error" } }recommended-requiring-type-checking (ruleswith type information)
Comments about the current config
"@typescript-eslint/await-thenable": "error",- Best practice to avoid unnecessary
awaits. - Verdict: in.
- Best practice to avoid unnecessary
"@typescript-eslint/no-for-in-array": "error",- Best practice to avoid accidentally iterating over array properties.
- Verdict: in.
"@typescript-eslint/no-misused-promises": "error",- Helps catch bugs due to missing
awaits. - Verdict: in.
- Helps catch bugs due to missing
"@typescript-eslint/no-unnecessary-type-assertion": "error",- Stylistic rule, I don't think this helps catch any bugs.
- It's certainly a good practice to avoid unnecessary code.
- Looking for feedback here.
- Current thinking is to keep it in
"@typescript-eslint/prefer-includes": "error","@typescript-eslint/prefer-string-starts-ends-with": "error",- Stylistic rules; I don't think they help catch any bugs.
- These rules are about consistency, and enforcing the use of new APIs, which are all good things.
- Looking for feedback here.
- Current thinking is to remove it
"@typescript-eslint/prefer-regexp-exec": "error",- RegExp#exec is faster than String#match. Why would you ever use the slower version?
- Verdict: in.
"@typescript-eslint/require-await": "error",- Async function without an await will cause the runtime to wrap the return value in a promise.
- This is unnecessary and a bug.
- Our extension also adds support for explicitly returning promises, which makes the rule easier to use.
- Verdict: in.
"@typescript-eslint/unbound-method": "error",- Usage of unbound methods are a consistent source of bugs.
- Verdict: in.
Comments about the new rules
@typescript-eslint/no-base-to-string- Stylistic, may catch some errors though.
- Looking for feedback here.
@typescript-eslint/no-dynamic-delete- Stylistic, may catch some errors though.
- Verdict: do not add
@typescript-eslint/no-floating-promises- Helps catch bugs due to non-awaited function calls
- Verdict: add as error
@typescript-eslint/no-implied-eval- using
eval-like methods is a security problem, and a type safety hole - Verdict: add as error
- using
@typescript-eslint/no-throw-literal- stylistic, it's a great practice, but it's hard to recommend for all codebases
- eg react suspense requires you to throw Promises.
- Verdict: do not add
- stylistic, it's a great practice, but it's hard to recommend for all codebases
@typescript-eslint/no-unnecessary-boolean-literal-compare- Stylistic, I don't think it has enough value to recommend for everyone
- Verdict: do not add
@typescript-eslint/no-unnecessary-condition- Unsure if this should be recommended. It's great as it removes unnecessary code, but does it help catch bugs?
- Two arguments I know of against this rule:
- It relies upon having 100% correct types, which isn't always the case.
Case-in-point: the typescript API types themselves, which have a lot of things defined as non-nullable, but in practice are actually nullable in some cases. The types are non-nullable because it's the 0.1% case that it's nullable, so nullable types would pollute the TS codebase with unnecessary checks. - There's a lot of people that practice defensive programming as well, purposely guarding against things the types suggest are impossible, just in case something goes wrong at runtime.
- It relies upon having 100% correct types, which isn't always the case.
- Looking for feedback here.
@typescript-eslint/no-unsafe-assignment(feat(eslint-plugin): add rule no-unsafe-assignment #1694)- This should improve the safety of every codebase
- Verdict: add as error
@typescript-eslint/no-unsafe-call- This should improve the safety of every codebase
- Verdict: add as error
@typescript-eslint/no-unsafe-member-access- This should improve the safety of every codebase
- Verdict: add as error
@typescript-eslint/no-unsafe-return- This should improve the safety of every codebase
- Verdict: add as error
@typescript-eslint/prefer-nullish-coalescing- This is a good rule on the surface, but it is not easy to recommend due to the changes often being not exactly 1:1 replacements, esp in complex code.
- Verdict: do not add
@typescript-eslint/prefer-optional-chain- This is a good rule on the surface, but it is not easy to recommend due to the changes often being not exactly 1:1 replacements, esp in complex code.
- Verdict: do not add
@typescript-eslint/prefer-readonly-parameters- Stylistic, many people would hate this due to the deep readonly requirement
- Verdict: do not add
@typescript-eslint/restrict-plus-operands- Prevents you from accidentally adding
string + undefined, which is definitely a bug. - Makes sure you are correctly handling string + number
- Verdict: add as error
- Prevents you from accidentally adding
@typescript-eslint/restrict-template-expressions- Catches accidentally passing objects/arrays/nulls/undefined into template expressions, which may not produce the string you expect.
- A lot of time these strings are used in user-output positions, so leaking
nulletc is an obvious bug. - Verdict: add as error
@typescript-eslint/return-await- Stylistic, may catch some errors though.
- Looking for feedback here.
@typescript-eslint/switch-exhaustiveness-check- Stylistic, may catch some errors though.
- Looking for feedback here.
TL;DR
{ "extends": "./configs/base.json", "rules": { "@typescript-eslint/await-thenable": "error",+ "@typescript-eslint/no-floating-promises": "error", "@typescript-eslint/no-for-in-array": "error",+ "@typescript-eslint/no-implied-eval": "error", "@typescript-eslint/no-misused-promises": "error", "@typescript-eslint/no-unnecessary-type-assertion": "error",+ "@typescript-eslint/no-unsafe-assignment": "error",+ "@typescript-eslint/no-unsafe-call": "error",+ "@typescript-eslint/no-unsafe-member-access": "error",+ "@typescript-eslint/no-unsafe-return": "error",- "@typescript-eslint/prefer-includes": "error", "@typescript-eslint/prefer-regexp-exec": "error",- "@typescript-eslint/prefer-string-starts-ends-with": "error", "require-await": "off", "@typescript-eslint/require-await": "error",+ "@typescript-eslint/restrict-plus-operands": "error",+ "@typescript-eslint/restrict-template-expressions": "error", "@typescript-eslint/unbound-method": "error", "no-var": "error", "prefer-const": "error", "prefer-rest-params": "error", "prefer-spread": "error" } }