Sitelet https://web.archive.org/web/20210809230719/https://github.com/angular/angular-cli/issues/21279
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

Enable more linter checks in tsconfig.json by default #21279

Closed
2 of 15 tasks
prabh-62 opened this issue Jul 3, 2021 · 7 comments
Closed
2 of 15 tasks

Enable more linter checks in tsconfig.json by default #21279

prabh-62 opened this issue Jul 3, 2021 · 7 comments

Comments

@prabh-62
Copy link
Task lists! Give feedback

@prabh-62 prabh-62 commented Jul 3, 2021

🚀 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,
    ...
  }
}

angular_strict_tsconfig_ts_4_4

Relevant blog posts:

Describe alternatives you've considered

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

@ngbot ngbot bot modified the milestone: needsTriage Jul 5, 2021
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 5, 2021
@alan-agius4 alan-agius4 changed the title Enable more linter checks in tsconfig.json by default in v13 Enable more linter checks in tsconfig.json by default Jul 7, 2021
@dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Jul 8, 2021

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
Copy link
Member

@mgechev mgechev commented Jul 8, 2021

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
Copy link
Collaborator

@dgp1130 dgp1130 commented Jul 9, 2021

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
Copy link
Member

@mgechev mgechev commented Jul 9, 2021

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
Copy link
Collaborator

@dgp1130 dgp1130 commented Jul 9, 2021

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.

alan-agius4 added a commit to alan-agius4/angular-cli that referenced this issue Aug 9, 2021
…cessFromIndexSignature` to workspace tsconfig

With this change, when the workspace is created in strict mode (the default) we add the following  additional tsconfig options;
- [noImplicitOverride](https://www.typescriptlang.org/tsconfig#noImplicitOverride)
- [noPropertyAccessFromIndexSignature](https://www.typescriptlang.org/tsconfig#noPropertyAccessFromIndexSignature)

Closes angular#21279
alan-agius4 added a commit that referenced this issue Aug 9, 2021
…cessFromIndexSignature` to workspace tsconfig

With this change, when the workspace is created in strict mode (the default) we add the following  additional tsconfig options;
- [noImplicitOverride](https://www.typescriptlang.org/tsconfig#noImplicitOverride)
- [noPropertyAccessFromIndexSignature](https://www.typescriptlang.org/tsconfig#noPropertyAccessFromIndexSignature)

Closes #21279
@cyrilletuzi
Copy link
Contributor

@cyrilletuzi cyrilletuzi commented Aug 9, 2021

To anyone interested by this topic: as it's indeed difficult to keep on track of TypeScript best practices, that's why I created typescript-strictly-typed, which will enable everything for you.

Also, while other options may be debatable and a matter of opinion, I also want to backup @dgp1130 about noUncheckedIndexedAccess: I ended up to remove it from my tool, despite its goal is full strictness. It's not just this option may be awkward for beginners, it's that in the current state of TypeScript (v4.3), one could say it's "buggy" because it reports errors which are not errors, because TypeScript inference is currently not good enough:

For example:

const list: number[] = [1, 2, 3];

if (list.length > 0) {
  list[0].toString(); // will report an error saying it could be `undefined`
}

It may change in a future release if TypeScript inference gets better, but for now it's indeed an option that should be manually enabled only by people aware of these limitations.

@dgp1130
Copy link
Collaborator

@dgp1130 dgp1130 commented Aug 9, 2021

It's not just this option may be awkward for beginners, it's that in the current state of TypeScript (v4.3), one could say it's "buggy" because it reports errors which are not errors, because TypeScript inference is currently not good enough:

For example:

const list: number[] = [1, 2, 3];

if (list.length > 0) {
  list[0].toString(); // will report an error saying it could be `undefined`
}

I think TypeScript actually is powerful enough to infer the type, it just chooses not to because literals are generally inferred as the primitive type (T[], string, number) rather than the true literal type ([1, 2], "test", 5). You could rewrite this as:

const list = [1, 2, 3] as const;
list[0].toString(); // works

The as const makes TypeScript infer list as a tuple of [1, 2, 3], so list[0] is known at compile-time to be exactly 1 (with no undefined possibility), despite noUncheckedIndexedAccess. So not really a "bug" in the compiler, just the consequence of a design choice around literal typings.

That said, users making this mistake still have to recognize the issue and figure out where to put an as const, which is not obvious or intuitive, particularly to those who may not fully grasp the distinction between an array type and a tuple type, which is especially confusing since this distinction only exists in TypeScript and not JavaScript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants