feat(compiler): narrow types of expressions used in *ngIf by chuckjaz · Pull Request #20702 · angular/angular

chuckjaz · 2017-11-30T01:10:44Z

Structural directives can now specify a type guard that describes
what types can be inferred for an input expression inside the
directive's template.

NgIf was modified to declare an input guard on ngIf.

After this change, fullTemplateTypeCheck will infer that
usage of ngIf expression inside it's template is truthy.

For example, if a component has a property person?: Person
and a template of <div *ngIf="person"> {{person.name}} </div>
the compiler will no longer report that person might be null or
undefined.

The template compiler will generate code similar to,

  if (NgIf.ngIfTypeGuard(instance.person)) {
    instance.person.name
  }

to validate the template's use of the interpolation expression.
Calling the type guard in this fashion allows TypeScript to infer
that person is non-null.

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

Given a component like:

export interface Person {
  name: string;
}

@Component({
  template: `<div *ngIf="person">Hello {{person.name}}</div>`
})
export Greeter {
  @Input() person?: Person;
}

The compiler would produce an error stating that person might be undefined.

What is the new behavior?

The compiler infers that person is Person inside the *ngIfbut is still Person | undefined outside the *ngIf.

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

trotyl · 2017-11-30T01:19:09Z

Likely will close #19756?

mary-poppins · 2017-11-30T01:19:35Z

You can preview 4d39227 at https://pr20702-4d39227.ngbuilds.io/.

mary-poppins · 2017-11-30T19:12:16Z

You can preview b4bdf80 at https://pr20702-b4bdf80.ngbuilds.io/.

vicb · 2017-11-30T19:27:35Z

vicb · 2017-11-30T19:33:07Z

Consider adding this to the @Input() instead of doing name matching ?

I discussed this with Misko and, though this is technically right, it give an esoteric feature way to much visibility and using naming convention is much better for such a corner case.

vicb · 2017-11-30T19:35:04Z

q to discuss reduce vs foreach

vicb · 2017-11-30T19:39:31Z

remove = undefined ?

let does not default to undefined but, rather, raises an error if it is accessed before initialization.

vicb · 2017-11-30T19:51:15Z

Comments:

would be nice to configure this via @Input(),
commit comment refers to ngIfTypeGuard instead of ngIfGuard,
needs doc updated - at least API docs,
pls add fixes #19756 as mentioned by @trotyl,
add a test with renamed inputs: @Input(a-dash) aDash;,
split the PR in 2 commits: general mechanism & NgIf update ?

mary-poppins · 2017-12-01T00:39:53Z

You can preview 2360d6e at https://pr20702-2360d6e.ngbuilds.io/.

mary-poppins · 2017-12-04T18:35:30Z

You can preview f7ef4a3 at https://pr20702-f7ef4a3.ngbuilds.io/.

mary-poppins · 2017-12-04T23:14:53Z

You can preview 4bc39c0 at https://pr20702-4bc39c0.ngbuilds.io/.

mary-poppins · 2017-12-05T01:34:07Z

You can preview 080e072 at https://pr20702-080e072.ngbuilds.io/.

mary-poppins · 2017-12-05T04:52:05Z

You can preview d9a3fa0 at https://pr20702-d9a3fa0.ngbuilds.io/.

mary-poppins · 2017-12-05T22:06:15Z

You can preview ce114a3 at https://pr20702-ce114a3.ngbuilds.io/.

chuckjaz · 2017-12-06T00:25:22Z

would be nice to configure this via @input(),

As discussed off-line, this would raise the visibility of a very infrequently used feature.

commit comment refers to ngIfTypeGuard instead of ngIfGuard,

Fixed.

needs doc updated - at least API docs,

Will do in a follow-on PR.

pls add fixes #19756 as mentioned by @trotyl,

Done;

add a test with renamed inputs: @input(a-dash) aDash;,

Missed this one. Will add a test.

split the PR in 2 commits: general mechanism & NgIf update ?

I prefer to keep them together as they need to be taken together for the feature to work. For example, we would never revert one of the commits without reverting them all as the NgIf change makes no sense without the rest and vis versa.

Structural directives can now specify a type guard that describes what types can be inferred for an input expression inside the directive's template. NgIf was modified to declare an input guard on ngIf. After this change, `fullTemplateTypeCheck` will infer that usage of `ngIf` expression inside it's template is truthy. For example, if a component has a property `person?: Person` and a template of `<div *ngIf="person"> {{person.name}} </div>` the compiler will no longer report that `person` might be null or undefined. The template compiler will generate code similar to, ``` if (NgIf.ngIfTypeGuard(instance.person)) { instance.person.name } ``` to validate the template's use of the interpolation expression. Calling the type guard in this fashion allows TypeScript to infer that `person` is non-null. Fixes: angular#19756?

chuckjaz · 2017-12-06T00:32:05Z

add a test with renamed inputs: @input(a-dash) aDash;,

Done.

mary-poppins · 2017-12-06T00:42:38Z

You can preview 4c47bc4 at https://pr20702-4c47bc4.ngbuilds.io/.

cyrilletuzi · 2017-12-14T12:52:25Z

@chuckjaz Does it close #18248 ?

cyrilletuzi · 2018-01-18T13:38:58Z

@chuckjaz Can't get this to work. Angular 5.2.1, TS 2.5.3, fullTemplateTypeCheck enabled. Is there something else to do ?

angular-automatic-lock-bot · 2019-09-13T04:51:43Z

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

googlebot added the cla: yes label Nov 30, 2017

mhevery added the area: core Issues related to the framework runtime label Nov 30, 2017

chuckjaz force-pushed the type-narrow branch from 4d39227 to b4bdf80 Compare November 30, 2017 19:03

vicb reviewed Nov 30, 2017

View reviewed changes

Comment thread packages/compiler/src/aot/static_reflector.ts Outdated

vicb Nov 30, 2017 •

edited

Loading

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

guard*s*

vicb reviewed Nov 30, 2017

View reviewed changes

Comment thread packages/compiler/src/aot/static_reflector.ts Outdated

vicb Nov 30, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q to discuss reduce vs foreach

vicb reviewed Nov 30, 2017

View reviewed changes

chuckjaz force-pushed the type-narrow branch from b4bdf80 to 2360d6e Compare December 1, 2017 00:30

chuckjaz mentioned this pull request Dec 4, 2017

Language Service: More forgiving support of union types in templates #17953

Closed

chuckjaz force-pushed the type-narrow branch from 2360d6e to f7ef4a3 Compare December 4, 2017 18:26

chuckjaz force-pushed the type-narrow branch from f7ef4a3 to 4bc39c0 Compare December 4, 2017 23:04

chuckjaz force-pushed the type-narrow branch from 4bc39c0 to 080e072 Compare December 5, 2017 01:26

chuckjaz force-pushed the type-narrow branch from 080e072 to d9a3fa0 Compare December 5, 2017 04:42

chuckjaz force-pushed the type-narrow branch from d9a3fa0 to ce114a3 Compare December 5, 2017 21:50

chuckjaz force-pushed the type-narrow branch from ce114a3 to 4c47bc4 Compare December 6, 2017 00:31

vicb approved these changes Dec 7, 2017

View reviewed changes

mhevery approved these changes Dec 7, 2017

View reviewed changes

chuckjaz added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release labels Dec 7, 2017

jasonaden closed this in e7d9cb3 Dec 8, 2017

IgorMinar mentioned this pull request Dec 12, 2017

ngIf type narrowing feature introduced by #20702 is incomplete #20967

Closed

cyrilletuzi mentioned this pull request Jan 18, 2018

feat(@schematics/angular): strict TypeScript angular/devkit#397

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

chuckjaz commented Nov 30, 2017

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

trotyl commented Nov 30, 2017

Uh oh!

mary-poppins commented Nov 30, 2017

Uh oh!

mary-poppins commented Nov 30, 2017

Uh oh!

vicb Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

chuckjaz Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

vicb Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

vicb Nov 30, 2017

Choose a reason for hiding this comment

Uh oh!

chuckjaz Dec 1, 2017

Choose a reason for hiding this comment

Uh oh!

vicb commented Nov 30, 2017

Uh oh!

mary-poppins commented Dec 1, 2017

Uh oh!

mary-poppins commented Dec 4, 2017

Uh oh!

mary-poppins commented Dec 4, 2017

Uh oh!

mary-poppins commented Dec 5, 2017

Uh oh!

mary-poppins commented Dec 5, 2017

Uh oh!

mary-poppins commented Dec 5, 2017

Uh oh!

chuckjaz commented Dec 6, 2017

Uh oh!

chuckjaz commented Dec 6, 2017

Uh oh!

mary-poppins commented Dec 6, 2017

Uh oh!

cyrilletuzi commented Dec 14, 2017

Uh oh!

cyrilletuzi commented Jan 18, 2018

Uh oh!

angular-automatic-lock-bot Bot commented Sep 13, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

vicb Nov 30, 2017 •

edited

Loading