Configs: Switch all recommended values to errors?

[JoshuaKGoldberg]

Before You File a Proposal Please Confirm You Have Done The Following...

• I have searched for related issues and found none that match my proposal.
• I have read the FAQ and my problem is not listed.

Description

Per eslint/eslint#16512, a preference of at least three ESLint or plugin maintainers (myself included) is to use warnings only as a temporary measure:

Warnings were intended to be used temporarily. My belief is that using warnings long term results in people tuning them out as just console noise, so I don’t believe you are getting much benefit with long-term warnings.

Personally, I would advocate switching from warn to error in your strict config. As mentioned, warnings are easily ignored and thus not that useful other than for temporary/transitional usage. If someone opts-in to using your strict config, then presumably they're interested in actually enforcing stricter/more-opinionated rules with actual violations and not just warnings.

Proposal: let's switch meta.docs.recommended to just specify a recommended config family?

- recommended: 'error' | 'strict' | 'warn' | false;
+ recommended: 'recommended' | 'strict' | false;

Impacted Configurations

• recommended
• recommended-requiring-type-checking
• strict

Additional Info

No response

[bradzacher]

We've spoken about this before - errors should never ever be in the codebase, warnings should really not be, but it's not terrible if they are.

The recommended rules that are warning:

• no-unused-vars
• no-explicit-any
• no-non-null-assertion

I think no-explicit-any should probably be an error at this point of TS's maturity.
But the other two are more difficult.

Non-null assertions are a useful tool when used sparingly. We use them copiously in this codebase because TS API types aren't great, and neither are some of our types (eg .parent).

Unused variables are just a part of the development cycle, but they shouldn't exist in production code. The problem is that historically there were a number of tools that would hard-block the rebuild pipeline if they encountered ESLint errors. So by setting it to error you can end up getting in people's way and frustrating them!

I come from a perspective of lint at scale where it's impossible to enforce "lint 0" across the entire codebase, so leveraging the different severities is an important tool.
But I definitely understand that at most codebase's scale - warnings don't really have a good use beyond migration usecases.

So I'm in favour of this change.

[Josh-Cena]

I'm not in favor of no-non-null-assertion being an error—if anything I'd like to see this rule being removed from "recommended"... I've constantly seen people getting confused about the warning in legitimate usage places. This rule also interacts badly with poor CFA (a.k.a. microsoft/TypeScript#9998) and noUncheckedIndexAccess. What's the alternative? Change strings[k]! to strings[k] as string, which doesn't look any safer. To me, warnings mean "there's a fair chance it doesn't matter, feel free to suppress it", which is a much more friendly signal than an error.

[JoshuaKGoldberg]

Yeah I'd be in favor of this too. It's a pity because no-non-null-assertion is quite good in spirit... but in practice I often find myself having to work around it.

@bradzacher IIRC you'd had an insightful comment somewhere about why no-non-null-assertion is in recommended but banning type assertions altogether isn't. Am I remembering right?

[bradzacher]

Previously you couldn't truly ban as assertions cos there were just so many usecases that you needed them. as const is one example, but there is (or used to be, at least) cases where you need an as to get the type right.
For example - exporting an object directly. You had two options - export default ({ ... } as Type) or const obj: Type = { ... }; export default obj;. Both suck for their own reasons.

TBH though with 4.9's satisfies operator - I would say we could actually ban as assertions outright now - the only cases for as are really just unsafe "trust me TS I know what I'm doing" cases (examples.

The problem with non-null assertions is that they're so easy to add. IMO they are actually a bad language feature because they're too easy to use and abuse. At least with assertions you need to manually write out the type and add parentheses, so it's cumbersome and really easy to see that you're doing something out-of-the-ordinary. But non-null assertions are just one character. One little exclamation mark. IMO you should avoid them at all costs and instead use optional chaining paired with "useless" runtime checks (as I mentioned below).

I know we don't do it a lot in our codebase - we're very lazy because of the old nullish parent typing stuff and a lot of the TS APIs too, but I'd love to get us there.
Asserting branch coverage is a big problem though - cos it also means you gotta add "istanbul ignore" comments everywhere 😢.

[bradzacher]

One alternative to the assertion is just adding "unnecessary" checks to help TS refine the types.
People are really averse to adding unnecessary checks but ultimately defensively programming isn't the negative people make it out to be!

An anecdotal "case study" of sorts. Flow does not have a "non null assertion" operator. In fact there's no way to type-assert your way out of a nullish type like you can in TS because flow disallows unsound assertions.
The only way to handle it is via runtime checks.

So the entire Facebook codebase (the largest JS codebase in the world) is built with many "unnecessary" checks to help flow refine types.

To make it even worse - the graphql schemas have every field listed as nullable by default because technically any field could throw (which is silently logged and the value nulled out). In practice most of the fields won't throw though. So there's even more checks in the codebase than the average codebase.

Lots of "unnecessary" code that really doesn't harm anything or cause any measurable perf impact.