strictNullChecks support in v4.0 is broken #15432

Closed
IgorMinar opened this Issue Mar 23, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@IgorMinar
Member

IgorMinar commented Mar 23, 2017

I'm submitting a ... (check one with "x")

[x] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
Originally we implemented strictNullChecks support by hand-modifying the public api surface and testing apps against that with strictNullChecks tsc option on. We though we got a good coverage and fixed all the issues, but during the RC we started noticing that we missed quite a few places.

Eventually @mhevery went off to implement this properly by actually making the Angular codebase compile with strictNullChecks on without hand-tweaking the API. This work (#15405) surfaced a lot more issues with our public api surface that affects apps that do turn on strictNullChecks.

To make matters worse, we realized that if we were to allow apps to turn on strictNullChecks with these bugs in our public api surface, we wouldn't be able to fix the issues without breaking the apps that do turn on strictNullChecks.

Expected behavior
Correct strictNullChecks support.

Proposed plan

  • Prevent applications from turning on strictNullChecks for now via a bogus symbol in our api that fails the strict null check
  • Release 4.0 without the strictNullChecks support
  • Merge @mhevery #15405 and any other follow up PRs related to this
  • Release the strictNullChecks support in 4.1 (due in April, approximately one month from now)

IgorMinar added a commit to IgorMinar/angular that referenced this issue Mar 23, 2017

IgorMinar added a commit to IgorMinar/angular that referenced this issue Mar 23, 2017

IgorMinar added a commit to IgorMinar/angular that referenced this issue Mar 23, 2017

IgorMinar added a commit to IgorMinar/angular that referenced this issue Mar 23, 2017

vicb added a commit that referenced this issue Mar 23, 2017

mhevery added a commit to mhevery/angular that referenced this issue Mar 24, 2017

mhevery added a commit to mhevery/angular that referenced this issue Mar 24, 2017

mhevery added a commit to mhevery/angular that referenced this issue Mar 24, 2017

@IgorMinar IgorMinar added this to the 4.x.x-candidates milestone Mar 24, 2017

mhevery added a commit to mhevery/angular that referenced this issue Mar 24, 2017

@peterkelly

This comment has been minimized.

Show comment
Hide comment
@peterkelly

peterkelly Mar 28, 2017

Why are you actively introducing code to prevent strictNullChecks, when it would otherwise work properly for at least a subset of cases?

In a project I am working on, we have successfully been using strictNullChecks since 1.6. During our migration to Angular 2, we had to have our build process apply some patches we created to a small number of definition files in the API so that our code would continue to build, but other than that it has been working fine (including in combination with Ionic 2.0).

I can understand the desire to ensure that the public API definition files properly reflect whether or not certain functions can accept or return values that are null or undefined, and that introducing null or undefined to a type constitutes a breaking API change. However, given that there are already existing projects that are using Angular with strictNullChecks (e.g. because they had a large existing codebase which already worked with it, and added Angular 2+ as a dependency), these changes are already breaking anyway.

Perhaps it might be worthwhile distributing strictNullCheck-compatible definition files that can be used as an option instead? I'm tempted to do so as part of a separate npm package, but if this is likely to be resolved shortly then it's probably better for me to wait it out.

peterkelly commented Mar 28, 2017

Why are you actively introducing code to prevent strictNullChecks, when it would otherwise work properly for at least a subset of cases?

In a project I am working on, we have successfully been using strictNullChecks since 1.6. During our migration to Angular 2, we had to have our build process apply some patches we created to a small number of definition files in the API so that our code would continue to build, but other than that it has been working fine (including in combination with Ionic 2.0).

I can understand the desire to ensure that the public API definition files properly reflect whether or not certain functions can accept or return values that are null or undefined, and that introducing null or undefined to a type constitutes a breaking API change. However, given that there are already existing projects that are using Angular with strictNullChecks (e.g. because they had a large existing codebase which already worked with it, and added Angular 2+ as a dependency), these changes are already breaking anyway.

Perhaps it might be worthwhile distributing strictNullCheck-compatible definition files that can be used as an option instead? I'm tempted to do so as part of a separate npm package, but if this is likely to be resolved shortly then it's probably better for me to wait it out.

unlight pushed a commit to unlight/angular-webpack-seed that referenced this issue Apr 9, 2017

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Apr 10, 2017

Member

Because the type information is broken, and once we fix it we would break a lot of people, hence we could not roll it out in v4.1, but have to wait until v5. So by preventing people, we are saying this never worked, and it becomes a feature of v4.1.

If you really insist on doing strictNullChecks in your code knowing that you will be broken in v4.1 than you can put "skipLibCheck": true, in your tsconfig.json which would allow you to turn on "strictNullChecks": true, without braking your build.

Member

mhevery commented Apr 10, 2017

Because the type information is broken, and once we fix it we would break a lot of people, hence we could not roll it out in v4.1, but have to wait until v5. So by preventing people, we are saying this never worked, and it becomes a feature of v4.1.

If you really insist on doing strictNullChecks in your code knowing that you will be broken in v4.1 than you can put "skipLibCheck": true, in your tsconfig.json which would allow you to turn on "strictNullChecks": true, without braking your build.

@aluanhaddad

This comment has been minimized.

Show comment
Hide comment
@aluanhaddad

aluanhaddad Apr 10, 2017

@mhevery As you know, this disables checking of all .d.ts files. This includes hand authored application level declarations. Although these files can generally be simply changed to .ts, this is not always the case.

Regardless, disabling checking of all libraries is not an acceptable workaround.

It is toxic.

aluanhaddad commented Apr 10, 2017

@mhevery As you know, this disables checking of all .d.ts files. This includes hand authored application level declarations. Although these files can generally be simply changed to .ts, this is not always the case.

Regardless, disabling checking of all libraries is not an acceptable workaround.

It is toxic.

@peterkelly

This comment has been minimized.

Show comment
Hide comment
@peterkelly

peterkelly Apr 10, 2017

I think I'm just going to stick to Angular 2

I think I'm just going to stick to Angular 2

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Apr 11, 2017

Member

BTW, enabling string null checks is on top of my priority list, so this should be fixed soon.

Member

mhevery commented Apr 11, 2017

BTW, enabling string null checks is on top of my priority list, so this should be fixed soon.

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Apr 27, 2017

Contributor

As for me compiler error only on forms and http components and 1 for core

Contributor

Delagen commented Apr 27, 2017

As for me compiler error only on forms and http components and 1 for core

@mhevery

This comment has been minimized.

Show comment
Hide comment
@mhevery

mhevery Apr 27, 2017

Member

Support landed in v4.1

Member

mhevery commented Apr 27, 2017

Support landed in v4.1

@mhevery mhevery closed this Apr 27, 2017

@CSchulz

This comment has been minimized.

Show comment
Hide comment
@CSchulz

CSchulz Apr 27, 2017

There is still an open bug with forms #16357

CSchulz commented Apr 27, 2017

There is still an open bug with forms #16357

@Delagen

This comment has been minimized.

Show comment
Hide comment
@Delagen

Delagen Apr 27, 2017

Contributor

@CSchulz see my comment, not only

Contributor

Delagen commented Apr 27, 2017

@CSchulz see my comment, not only

@dominique-mueller dominique-mueller referenced this issue in dominique-mueller/angular-notifier May 3, 2017

Merged

chore(config): Enable TSC strict mode, add editorconfig file #24

asnowwolf added a commit to asnowwolf/angular that referenced this issue Aug 11, 2017

juleskremer added a commit to juleskremer/angular that referenced this issue Aug 28, 2017

@peter

This comment has been minimized.

Show comment
Hide comment
@peter

peter Oct 10, 2017

As of 10:th of October 2017 I cannot compile the latest released version of Angular (4.4.4) with latest TypeScript (2.5.3) and scrictNullChecks. Failures:

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:2:22: Class 'DefaultIterableDifferFactory' incorrectly implements interface 'IterableDifferFactory'.
Types of property 'create' are incompatible.
Type '(trackByFn?: TrackByFunction | undefined) => DefaultIterableDiffer' is not assignable to type '{ (trackByFn?: TrackByFunction | undefined): IterableDiffer; (_cdr?: ChangeDetectorRe...'.

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:10:22: Class 'DefaultIterableDiffer' incorrectly implements interface 'IterableChanges'.
Types of property 'forEachOperation' are incompatible.
Type '(fn: (item: IterableChangeRecord, previousIndex: number | null, currentIndex: number | null) =...' is not assignable to type '(fn: (record: Iterabl

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:10:22: Class 'DefaultIterableDiffer' incorrectly implements interface 'IterableDiffer'.
Types of property 'diff' are incompatible.

peter commented Oct 10, 2017

As of 10:th of October 2017 I cannot compile the latest released version of Angular (4.4.4) with latest TypeScript (2.5.3) and scrictNullChecks. Failures:

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:2:22: Class 'DefaultIterableDifferFactory' incorrectly implements interface 'IterableDifferFactory'.
Types of property 'create' are incompatible.
Type '(trackByFn?: TrackByFunction | undefined) => DefaultIterableDiffer' is not assignable to type '{ (trackByFn?: TrackByFunction | undefined): IterableDiffer; (_cdr?: ChangeDetectorRe...'.

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:10:22: Class 'DefaultIterableDiffer' incorrectly implements interface 'IterableChanges'.
Types of property 'forEachOperation' are incompatible.
Type '(fn: (item: IterableChangeRecord, previousIndex: number | null, currentIndex: number | null) =...' is not assignable to type '(fn: (record: Iterabl

Error at node_modules/@angular/core/src/change_detection/differs/default_iterable_differ.d.ts:10:22: Class 'DefaultIterableDiffer' incorrectly implements interface 'IterableDiffer'.
Types of property 'diff' are incompatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment