Enable more linter checks in tsconfig.json by default

[prabh-62]

🚀 Feature request

Command (mark with an x)

• new
• build
• serve
• test
• e2e
• generate
• add
• update
• lint
• extract-i18n
• run
• config
• help
• version
• doc

Description

❯ npx @angular/cli@12 new web-apps --create-application false
❯ cd web-apps && yarn ng g app admin-console --routing --style scss

Currently, base tsconfig.json only contains strict, noImplicitReturns and noFallthroughCasesInSwitch flags

{
    "compileOnSave": false,
    "compilerOptions": {
      ...
      // all strict checks
      "strict": true,
      //  Linter checks
      "noImplicitReturns": true,
      "noFallthroughCasesInSwitch": true,
      ...
    }
}

Describe the solution you'd like

Enable new linter flags exposed by typescript in recent versions by defalt

{
  "compileOnSave": false,
  "compilerOptions": {
    ...
    // all strict checks
    "strict": true,
    //  Linter checks
    "noImplicitReturns": true,
    "noFallthroughCasesInSwitch": true,
	// TS 4.1
    "noUncheckedIndexedAccess": true,
	// TS 4.2
    "noPropertyAccessFromIndexSignature": true,
	// TS 4.3
    "noImplicitOverride": true,
	// TS 4.4
    "useUnknownInCatchVariables": true,
    "exactOptionalPropertyTypes": true,
    ...
  }
}

Relevant blog posts:

• Announcing TypeScript 4.1
  • --noUncheckedIndexedAccess
• Announcing TypeScript 4.2
  • --noPropertyAccessFromIndexSignature
• Announcing TypeScript 4.3
  • --noImplicitOverride
• Announcing TypeScript 4.4
  • --useUnknownInCatchVariables
  • --exactOptionalPropertyTypes

Describe alternatives you've considered

I just have to add these flags manually to tsconfig.json

[dgp1130]

It is a good idea to consider updating our strictness with recent TypeScript versions. Although it is worth keeping in mind that npx typescript --init does not enable any of these by default. Only strict is enabled by default. Angular can probably be a bit more opinionated here, but we should take care to not diverge too much where there isn't sufficient value to justify it.

After discussion within the tooling team, our takeaway with these flags is (interested to get @mgechev's opinion as well):

noUncheckedIndexedAccess

This is really useful for objects and really annoying for arrays. having a[0] return an undefined type is awkward and particularly confusing to those who might be learning arrays for the first time. It also applies to array destructuring unless you type the object as a tuple [string, /* ... */] instead of an array string[]. Advanced users might understand the difference and be able to work around it, but this can easily trip up new devs.

The main benefit of catching possibly undefined properties in objects only applies when using index types which is a bit of anti-pattern IMHO, Map objects should be preferred where possible (or even a Record<string, T | undefined>).

As a result, we feel that the benefit for catching undefined properties in objects is outweighed by the awkwardness of using arrays and shouldn't be included by default.

noPropertyAccessFromIndexSignature

We generally like this option as it is a pretty simple change which clarifies the user intent and catches possible typos without much friction. The error message is very straightforward and easily fixed. This should be included by default.

noImplicitOverride

The override keyword is really nice for catching accidental overrides that may not have been intentional. We think this is a good feature for the type system and very useful to have.

There were some concerns about this breaking a lot of existing documentation and education content already out there, however most Angular APIs do not use inheritance, so not much is actually broken. override is not required when implementing interfaces or abstract classes, so things like ngOnInit() are not affected by this option. As a result, users can define an Angular component, directive, service, etc. without running into a breakage. We couldn't think of any Angular APIs that expect users extend an override a method. I'm sure some exist, but they are obscure enough to not be an issue for new devs getting started with Angular.

Our takeaway is that this doesn't introduce much friction for new devs or existing educational content, so it is good to include by default.

useUnknownInCatchVariables

Using unknown in catch variables is more reflective of what the type system actually knows about a thrown object, but is often more annoying to use in practice. Throwing non-Error objects is generally considered an anti-pattern and should be avoided. Enabling this would also likely break almost all existing educational content which catches an error.

Our preference is for users to consider lint checks like no-throw-literal and treating any caught objects as Error. We think that would provide a better experience than useUnknownInCatchVariables.

exactOptionalPropertyTypes

This option draws a clear distinction between foo?: string and foo?: string | undefined which can be very confusing to new users. In particular, One particularly awkward edge case is that you can't put a string | undefined value into an optional string type and the error message is very unhelpful in this regard.

interface Bar {
  bar?: string;
}
const baz = 'test' as string | undefined;

const bar: Bar = {
  bar: baz,
  // Type 'string | undefined' is not assignable to type 'string'.
  //     Type 'undefined' is not assignable to type 'string'.
};

This is a very nuanced option and will easily trip up even advanced developers who aren't familiar with it. I think I saw somewhere that even the TypeScript team doesn't recommend this unless you have a good reason for it in your project, but I'm not able to find that statement now. Our take is that this is too nuanced and specific, making it likely to cause more pain than it actually solves and shouldn't be included by default.

Of course, all of these options can be manually added with minimal effort, this is only talking about what should be included by default for ng new projects. In total, this means that noPropertyAccessFromIndexSignature and noImplicitOverride should be added to ng new. We should also gate these so they are not included when --nostrict is provided, since the user wouldn't want them in that case anyways (and I think they both require strictNullChecks regardless).

These should be added in the next major to reduce breakages of existing educative content (please please please, pin the versions of any tutorials or workshops you own specifically to avoid breakages from stuff like this). TS 4.3 should be the minimum by Angular v13, so it should be safe to include those two without worrying about older TypeScript versions that don't support these flags.

I would love to migrate existing projects with the same options, however there don't appear to be any existing codemods which do this. VSCode can fix all noImplicitOverride and noPropertyAccessFromIndexSignature errors in a file, and both are pretty precise fixes, so migrating a project should be relatively easily, even if not done automatically. We'll probably skip existing projects on this for now and let them migrate themselves.

[mgechev]

Given our null safety target noUncheckedIndexedAccess makes sense. I agree that noImplicitOverride may invalidate some courses and tutorials, but I'm not expecting a significant impact.

In my opinion, the value noPropertyAccessFromIndexSignature brings is a bit questionable. Editor's autocompletion should help with avoiding typos and I don't see a significant benefit of distinguishing how we access properties declared with an index signature. I'd rather prefer to use a consistent syntax across my entire project.

I'd be worried to migrate existing projects. The flags for which we can't apply codemods with precise fixes could break many applications without bringing significant value. An optional migration could be a viable option.

[dgp1130]

In my opinion, the value noPropertyAccessFromIndexSignature brings is a bit questionable. Editor's autocompletion should help with avoiding typos and I don't see a significant benefit of distinguishing how we access properties declared with an index signature. I'd rather prefer to use a consistent syntax across my entire project.

I agree this has less value than something like noImplicitOverride, though not everyone uses autocomplete for every property they write. Plenty of devs will type out full property names (especially for small identifiers like id or name) and they won't have anything to flag that they introduced a typo. Having used Closure compiler, the idea of using a string lookup for properties like this is reasonable to me (where that controls property renaming), but I do understand that coming from a pure-JS perspective, it is a bit awkward to draw a distinction between these two syntaxes at the type level.

I could go either way on this one, but I do think catching real typos and mistakes outweighs the desire to use a consistent syntax. If users care that much about syntax, they can always remove the option.

I'd be worried to migrate existing projects. The flags for which we can't apply codemods with precise fixes could break many applications without bringing significant value. An optional migration could be a viable option.

To clarify a bit, I'm not suggesting we migrate existing projects here. I do think it is possible to do a clean migration since noImplicitOverride and noPropertyAccessFromIndexSignature can be fixed inline at the location of the error with a trivial transformation. If such codemods already existed, I might be more inclined to try something here, but I don't think it's worth us writing such codemods. I'm happy to keep this issue scoped to just new projects.

[mgechev]

SGTM!

I could go either way on this one, but I do think catching real typos and mistakes outweighs the desire to use a consistent syntax. If users care that much about syntax, they can always remove the option.

Likewise. We can enable and check people's feedback. Also we can consider if this is a flag enabled in g3 to ensure consistency across the 1P and 3P ecosystems.

[dgp1130]

Looking internally, it seems that none of these options are currently used.

However, there already is a tsetse check which is basically equivalent to noPropertyAccessFromIndexSignature due to the aforementioned Closure compiler property renaming syntax used internally. There is also a FR to enable the real flag, though I'm not sure how this fits in with the existing tsetse check.

I couldn't find an explicit FR for noImplicitOverride, however it is listed in the TS4.3 upgrade doc, so I think the intent is to eventually enable this.

For the other checks, I couldn't find any documented interest in enabling noUncheckedIndexedAccess, useUnknownInCatchVariables, or exactOptionalPropertyTypes. I imagine the first two would be too breaking to easy enable and the last one is likely too nuanced to expect devs to understand to work with. We could reach out to the TypeScript team at Google to confirm this, but my expectation is that there's not much desire to make any of these happen.

Based on this, my take is that enabling noImplicitOverride and noPropertyAccessFromIndexSignature should align us more closely with google3.