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

fix(ivy): do not always accept `undefined` for directive inputs #33066

Closed
wants to merge 6 commits into from

Conversation

@JoostK
Copy link
Member

JoostK commented Oct 9, 2019

fix(ivy): do not always accept undefined for directive inputs

Prior to this change, the template type checker would always allow a
value of type undefined to be passed into a directive's inputs, even
if the input's type did not allow for it. This was due to how the type
constructor for a directive was generated, where a Partial mapped
type was used to allow for inputs to be unset. This essentially
introduces the undefined type as acceptable type for all inputs.

This commit removes the Partial type from the type constructor, which
means that we can no longer omit any properties that were unset.
Instead, any properties that are not set will still be included in the
type constructor call, having their value assigned to any.

Before:

class NgForOf<T> {
  static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']});

After:

class NgForOf<T> {
  static ngTypeCtor<T>(init: Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {
  ngForOf: ['foo', 'bar'],
  ngForTrackBy: null as any,
  ngForTemplate: null as any,
});

This change only affects generated type check code, the generated
runtime code is not affected.

Fixes #32690
Resolves FW-1606


feat(ivy): check regular attributes that correspond with directive inputs

Prior to this change, a static attribute that corresponds with a
directive's input would not be type-checked against the type of the
input. This is unfortunate, as a static value always has type string,
whereas the directive's input type might be something different. This
typically occurs when a developer forgets to enclose the attribute name
in brackets to make it a property binding.

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.


feat(ivy): disable strict null checks for input bindings

This commit introduces an internal config option of the template type
checker that allows to disable strict null checks of input bindings to
directives. This may be particularly useful when a directive is from a
library that is not compiled with strictNullChecks enabled.

Right now, strict null checks are enabled when fullTemplateTypeCheck
is turned on, and disabled when it's off. In the near future, several of
the internal configuration options will be added as public Angular
compiler options so that users can have fine-grained control over which
areas of the template type checker to enable, allowing for a more
incremental migration strategy.

@ngbot ngbot bot added this to the Backlog milestone Oct 9, 2019
@googlebot googlebot added the cla: yes label Oct 9, 2019
@ngbot ngbot bot modified the milestone: Backlog Oct 9, 2019
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-missing-inputs branch 2 times, most recently from cdeb782 to 62bbd02 Oct 10, 2019
@JoostK JoostK marked this pull request as ready for review Oct 10, 2019
@JoostK JoostK requested a review from angular/fw-compiler as a code owner Oct 10, 2019
@alxhub
alxhub approved these changes Oct 10, 2019
Copy link
Contributor

alxhub left a comment

LGTM, with one comment.

@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-missing-inputs branch from 62bbd02 to 7af170d Oct 11, 2019
JoostK added a commit to JoostK/angular that referenced this pull request Oct 11, 2019
In angular#33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-missing-inputs branch from 7af170d to f8198d9 Oct 11, 2019
@JoostK JoostK requested a review from angular/docs-infra as a code owner Oct 11, 2019
JoostK added a commit to JoostK/angular that referenced this pull request Oct 11, 2019
In angular#33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-missing-inputs branch from f8198d9 to 0209c48 Oct 11, 2019
@@ -78,7 +78,7 @@ export class CodeTabsComponent implements OnInit, AfterViewInit {

header: tabContent.getAttribute('header') || undefined,
language: tabContent.getAttribute('language') || undefined,
linenums: tabContent.getAttribute('linenums') || this.linenums,
linenums: tabContent.getAttribute('linenums') || this.linenums || false,

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 11, 2019

Member

Currently, undefined ends up behaving the same as false, but this wasn't always the case. undefined means "use the default value", which might change in the future. Can we pass undefined through to the CodeComponent, so that we have one place where undefined it converted to a default value?

Prior to this change, the template type checker would always allow a
value of type `undefined` to be passed into a directive's inputs, even
if the input's type did not allow for it. This was due to how the type
constructor for a directive was generated, where a `Partial` mapped
type was used to allow for inputs to be unset. This essentially
introduces the `undefined` type as acceptable type for all inputs.

This commit removes the `Partial` type from the type constructor, which
means that we can no longer omit any properties that were unset.
Instead, any properties that are not set will still be included in the
type constructor call, having their value assigned to `any`.

Before:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']});
```

After:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {
  ngForOf: ['foo', 'bar'],
  ngForTrackBy: null as any,
  ngForTemplate: null as any,
});
```

This change only affects generated type check code, the generated
runtime code is not affected.

Fixes #32690
Resolves FW-1606
JoostK added 5 commits Oct 10, 2019
…puts

Prior to this change, a static attribute that corresponds with a
directive's input would not be type-checked against the type of the
input. This is unfortunate, as a static value always has type `string`,
whereas the directive's input type might be something different. This
typically occurs when a developer forgets to enclose the attribute name
in brackets to make it a property binding.

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.
This commit introduces an internal config option of the template type
checker that allows to disable strict null checks of input bindings to
directives. This may be particularly useful when a directive is from a
library that is not compiled with `strictNullChecks` enabled.

Right now, strict null checks are enabled when  `fullTemplateTypeCheck`
is turned on, and disabled when it's off. In the near future, several of
the internal configuration options will be added as public Angular
compiler options so that users can have fine-grained control over which
areas of the template type checker to enable, allowing for a more
incremental migration strategy.
In #33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.
@JoostK JoostK force-pushed the JoostK:ngtsc-ttc-missing-inputs branch 2 times, most recently from 071ac56 to 169ef36 Oct 12, 2019
Copy link
Member

gkalpak left a comment

LGTM for docs-infra 👍

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Oct 14, 2019

@mhevery mhevery closed this in 50bf17a Oct 14, 2019
mhevery added a commit that referenced this pull request Oct 14, 2019
This commit introduces an internal config option of the template type
checker that allows to disable strict null checks of input bindings to
directives. This may be particularly useful when a directive is from a
library that is not compiled with `strictNullChecks` enabled.

Right now, strict null checks are enabled when  `fullTemplateTypeCheck`
is turned on, and disabled when it's off. In the near future, several of
the internal configuration options will be added as public Angular
compiler options so that users can have fine-grained control over which
areas of the template type checker to enable, allowing for a more
incremental migration strategy.

PR Close #33066
mhevery added a commit that referenced this pull request Oct 14, 2019
In #33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.

PR Close #33066
mhevery added a commit that referenced this pull request Oct 14, 2019
…puts (#33066)

Prior to this change, a static attribute that corresponds with a
directive's input would not be type-checked against the type of the
input. This is unfortunate, as a static value always has type `string`,
whereas the directive's input type might be something different. This
typically occurs when a developer forgets to enclose the attribute name
in brackets to make it a property binding.

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.

PR Close #33066
@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 16, 2019

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.

After updating to next.11 with this change, I'm getting hundreds of these template type checking errors:

error TS2322: Type 'string' is not assignable to type 'boolean'.

This can be for simple things like <button mat-raised-button disabled>disabled</button>. The specific error in this case is for the disabled attribute.

It also occurs for things like this

    <div
      cdkDropList
      [cdkDropListData]="items"
      class="example-list"
      cdkDropListSortingDisabled
      (cdkDropListDropped)="drop($event)">

Where cdkDropListSortingDisabled is giving the error and it maps to a setter that has a boolean type.

Is this related to this change? Or should I keep looking through other changes in next.11 that would have caused this?

Is this change correct and just putting in place what is described and linked to from FW-1475?

@JoostK

This comment has been minimized.

Copy link
Member Author

JoostK commented Oct 16, 2019

@Splaktar oh no, that was definitely not the intention of that commit (although it is the one that introduces your type check errors, indeed)

The idea behind checking static attributes is for situations like these:

<my-cmp mode="drak" config="config"></my-cmp>

with my-cmp binding to the following component class:

export class MyCmp {
  @Input() mode: 'light' | 'dark';
  @Input() config: SomeConfig;
}

In this case, the template has an invalid binding for mode (it's misspelled) and the config attribute should have been a binding instead of a static attribute:

<my-cmp mode="dark" [config]="config"></my-cmp>

This is why static attributes now get checked as well.

In a pedantic world, the errors you're seeing for disabled are technically correct, but highly undesirable. There's work planned to introduce public facing strictness flags to be able to influence the behavior of the type checker here, unfortunately that is not in next.11. This is my mistake, the change related to attributes should have landed together with having these options available as it would allow to easily disable them. My apologies!

The scale of this change was not caught as I realize now that the Material unit tests we run have full template type check disabled.

@Splaktar

This comment has been minimized.

Copy link
Member

Splaktar commented Oct 16, 2019

@JoostK OK, that's good to know, thank you for that information!

FW-1475 has a pretty good explanation about the kinds of ergonomics that Angular Material and other libraries depend upon, especially around @Inputs with boolean and number types and things like required and disabled.

ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…lar#33066)

Prior to this change, the template type checker would always allow a
value of type `undefined` to be passed into a directive's inputs, even
if the input's type did not allow for it. This was due to how the type
constructor for a directive was generated, where a `Partial` mapped
type was used to allow for inputs to be unset. This essentially
introduces the `undefined` type as acceptable type for all inputs.

This commit removes the `Partial` type from the type constructor, which
means that we can no longer omit any properties that were unset.
Instead, any properties that are not set will still be included in the
type constructor call, having their value assigned to `any`.

Before:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']});
```

After:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {
  ngForOf: ['foo', 'bar'],
  ngForTrackBy: null as any,
  ngForTemplate: null as any,
});
```

This change only affects generated type check code, the generated
runtime code is not affected.

Fixes angular#32690
Resolves FW-1606

PR Close angular#33066
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
This commit introduces an internal config option of the template type
checker that allows to disable strict null checks of input bindings to
directives. This may be particularly useful when a directive is from a
library that is not compiled with `strictNullChecks` enabled.

Right now, strict null checks are enabled when  `fullTemplateTypeCheck`
is turned on, and disabled when it's off. In the near future, several of
the internal configuration options will be added as public Angular
compiler options so that users can have fine-grained control over which
areas of the template type checker to enable, allowing for a more
incremental migration strategy.

PR Close angular#33066
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
In angular#33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.

PR Close angular#33066
ODAVING added a commit to ODAVING/angular that referenced this pull request Oct 18, 2019
…puts (angular#33066)

Prior to this change, a static attribute that corresponds with a
directive's input would not be type-checked against the type of the
input. This is unfortunate, as a static value always has type `string`,
whereas the directive's input type might be something different. This
typically occurs when a developer forgets to enclose the attribute name
in brackets to make it a property binding.

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.

PR Close angular#33066
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…lar#33066)

Prior to this change, the template type checker would always allow a
value of type `undefined` to be passed into a directive's inputs, even
if the input's type did not allow for it. This was due to how the type
constructor for a directive was generated, where a `Partial` mapped
type was used to allow for inputs to be unset. This essentially
introduces the `undefined` type as acceptable type for all inputs.

This commit removes the `Partial` type from the type constructor, which
means that we can no longer omit any properties that were unset.
Instead, any properties that are not set will still be included in the
type constructor call, having their value assigned to `any`.

Before:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']});
```

After:

```typescript
class NgForOf<T> {
  static ngTypeCtor<T>(init: Pick<NgForOf<T>,
    'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>;
}

NgForOf.ngTypeCtor(init: {
  ngForOf: ['foo', 'bar'],
  ngForTrackBy: null as any,
  ngForTemplate: null as any,
});
```

This change only affects generated type check code, the generated
runtime code is not affected.

Fixes angular#32690
Resolves FW-1606

PR Close angular#33066
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
This commit introduces an internal config option of the template type
checker that allows to disable strict null checks of input bindings to
directives. This may be particularly useful when a directive is from a
library that is not compiled with `strictNullChecks` enabled.

Right now, strict null checks are enabled when  `fullTemplateTypeCheck`
is turned on, and disabled when it's off. In the near future, several of
the internal configuration options will be added as public Angular
compiler options so that users can have fine-grained control over which
areas of the template type checker to enable, allowing for a more
incremental migration strategy.

PR Close angular#33066
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
In angular#33066 a limitation of Ivy's template type checker was fixed, where
all directive inputs would incorrectly allow `undefined` to be passed,
even when the input's type did not allow for it. Due to the fix, some
additional type errors were uncovered in AIO, where potential
`undefined` values would be passed to inputs that were not typed to
allow `undefined`.

PR Close angular#33066
AndrusGerman added a commit to AndrusGerman/angular that referenced this pull request Oct 22, 2019
…puts (angular#33066)

Prior to this change, a static attribute that corresponds with a
directive's input would not be type-checked against the type of the
input. This is unfortunate, as a static value always has type `string`,
whereas the directive's input type might be something different. This
typically occurs when a developer forgets to enclose the attribute name
in brackets to make it a property binding.

This commit lets static attributes be considered as bindings with string
values, so that they will be properly type-checked.

PR Close angular#33066
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.