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

Don't enable skipLibCheck in --strict mode #16696

Closed
1 task done
cyrilletuzi opened this issue Jan 17, 2020 · 9 comments · Fixed by #16786
Closed
1 task done

Don't enable skipLibCheck in --strict mode #16696

cyrilletuzi opened this issue Jan 17, 2020 · 9 comments · Fixed by #16786

Comments

@cyrilletuzi
Copy link
Contributor

🚀 Feature request

Command (mark with an x)

  • config

Description

I'm quite surprised by this move: #16682, which enables TypeScript skipLibCheck by default. From what I know, it's usually considered as a temporary workaround for issues that should be correctly resolved. While it may have performances and compatibility advantages, it degrades types strictness (ie. some real errors may not be reported anymore).

It goes in an opposite way than the new --strict option in v9 that finally enables strict mode.

Describe the solution you'd like

I won't argue on your choice to make skipLibCheck the default (I suppose it comes from some issues I'm not aware of), but is it possible to not enable it when the --strict option has been provided, for people who wants to work with strict types?

@ngbot ngbot bot added this to the needsTriage milestone Jan 18, 2020
@alan-agius4
Copy link
Collaborator

Hi @cyrilletuzi, the skipLibCheck change is mainly due that TypeScript in some cases will emit incompatible DTS with older version of the compiler.

So if a library was build with TypeScript 3.7, it cannot be consumed without skipLibCheck in TypeScript 3.6 projects.

Let me loop in @IgorMinar to see if we should disable skipLibCheck when using the --strict option when generating a new Angular workspace.

More info about DTS compatibility issues can be found below:
microsoft/TypeScript#36207
microsoft/TypeScript#36216

@alan-agius4 alan-agius4 added the needs: more info Reporter must clarify the issue label Jan 18, 2020
@cyrilletuzi
Copy link
Contributor Author

cyrilletuzi commented Jan 18, 2020

Hi @alan-agius4, thanks for your answer.

As an Angular library author myself, I'm very aware of this problem. So I'll give you some feedback and some alternatives.

Angular has always considered support of newer TypeScript versions as not priority, delaying this to the next minor release (eg., Angular 9.0 will only support TS 3.6, and TS 3.7 support should come in 9.1). While it's true it's not a priority for Angular itself, it's not true for Angular librairies.

Indeed, a library should work with all v9 versions, including 9.0. It means it's not safe for an Angular library to use new features of TS 3.7 (like ?. and ??) until Angular 10.

So as a library author, I've always restricted myself to:

  • only use the lower TS version (so for Angular 9 it means TS 3.6),
  • if I have a very good reason to use a newer one (TS 3.7), to have a CI check to verify it works with the lower version (TS 3.6) too.

I see 3 better alternatives than using skipLibCheck to solve this problem:

  1. force libraries authors to use the lower TypeScript version in Angular Package Format specification and ng-packagr,
  2. add a downlevel-dts step in ng-packagr build,
  3. change the Angular priority strategy about TypeScript support.

@alan-agius4
Copy link
Collaborator

Couple of points to clear:

  1. the latest RC of Angular 9 supports TypeScript 3.7. Meaning that Angular 9.0 supports both TS 3.6 and 3.7.
  2. Using downlevel-dts alone is not enough. Because NGCC and NGC will need to be made aware of this, and thus logic needs to be added to resolve the new typings. As also, currently the metadata file can only reference a single DTS for the object shapes.
  3. Using nullish coalescing, optional chaining and other new TS features is perfectly fine as long as they don't alter the DTS shape.

@cyrilletuzi
Copy link
Contributor Author

Thanks for the clarifications. I wasn't aware of TS 3.7 support. But note that it was just an example (same scenario happened and will continue to happen for each new major release), and that if Angular 9.0 final release still support TS 3.6 too, the issue will still be here for librairies authors.

Given your feedback, I see a new alternative: would it be possible that ng-packagr does a downlevel-dts not for using the result for real but just to do a diff (lower DTS vs. newer DTS), and warn the library author if there are potential issues?

@alan-agius4 alan-agius4 added needs: discussion On the agenda for team meeting to determine next steps and removed needs: more info Reporter must clarify the issue labels Jan 21, 2020
@alan-agius4
Copy link
Collaborator

@cyrilletuzi regarding your comment

it degrades types strictness (ie. some real errors may not be reported anymore).

Can you provide some examples?

What I am aware of is that If a library ships a broken model, for instance:

export interface Foo {
    bar: InvalidType
}

Where InvalidType doesn't exist, instead of a compilation error, bar will fallback and become any during consumption.

import { Foo} from 'foo';
export const x: Foo = {
  bar: false
};

Another use-case where skipLibCheck will have a negative impact in a local project, is where the declaration of models or custom typings file in a .d.ts. instead of a .ts file, as validation of .d.ts will be skipped.

@cyrilletuzi
Copy link
Contributor Author

@alan-agius4 Indeed after some tests I couldn't find relevant cases. So I'll close this issue.

@alan-agius4 alan-agius4 removed the needs: discussion On the agenda for team meeting to determine next steps label Jan 23, 2020
@IgorMinar
Copy link
Contributor

I just talked to @DanielRosenwasser and he pointed out that skipLibCheck will cause local d.ts files to not be checked which is not good. We are going to revert this change and look into --incremental instead in the future releases (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#faster-subsequent-builds-with-the---incremental-flag)

@filipesilva
Copy link
Contributor

@cyrilletuzi I also want to mention that TS tries to keep versions futureproof wrt the next minor. 3.6 was future proofed against some features that 3.7 was meant to include. That is to say, anytime you find incompatible typings, we'd like to know. It should be safe for library authors to use the upper bound of supported TS versions for Angular.

kyliau pushed a commit that referenced this issue Jan 29, 2020
kyliau pushed a commit that referenced this issue Jan 29, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants