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

feat(compiler): narrow types of expressions used in *ngIf #20702

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@chuckjaz
Member

chuckjaz commented Nov 30, 2017

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

@googlebot googlebot added the cla: yes label Nov 30, 2017

@trotyl

This comment has been minimized.

Show comment
Hide comment
@trotyl

trotyl Nov 30, 2017

Contributor

Likely will close #19756?

Contributor

trotyl commented Nov 30, 2017

Likely will close #19756?

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 30, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Nov 30, 2017

@@ -142,6 +166,27 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver {
(stmt: o.Statement) => o.applySourceSpanToStatementIfNeeded(stmt, sourceSpan)));
});
if (this.guards.length) {
let guardExpression: o.Expression|undefined = undefined;

This comment has been minimized.

@vicb

vicb Nov 30, 2017

Contributor

remove = undefined ?

@vicb

vicb Nov 30, 2017

Contributor

remove = undefined ?

This comment has been minimized.

@chuckjaz

chuckjaz Dec 1, 2017

Member

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

@chuckjaz

chuckjaz Dec 1, 2017

Member

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

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Nov 30, 2017

Contributor

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 ?
Contributor

vicb commented Nov 30, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 1, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 4, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 4, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 5, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 5, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 5, 2017

@chuckjaz

This comment has been minimized.

Show comment
Hide comment
@chuckjaz

chuckjaz Dec 6, 2017

Member

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.

Member

chuckjaz commented Dec 6, 2017

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.

feat(compiler): narrow types of expressions used in *ngIf
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: #19756?
@chuckjaz

This comment has been minimized.

Show comment
Hide comment
@chuckjaz

chuckjaz Dec 6, 2017

Member

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

Done.

Member

chuckjaz commented Dec 6, 2017

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

Done.

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Dec 6, 2017

@vicb

vicb approved these changes Dec 7, 2017

@mhevery

mhevery approved these changes Dec 7, 2017

@cyrilletuzi

This comment has been minimized.

Show comment
Hide comment
@cyrilletuzi

cyrilletuzi Dec 14, 2017

Contributor

@chuckjaz Does it close #18248 ?

Contributor

cyrilletuzi commented Dec 14, 2017

@chuckjaz Does it close #18248 ?

@cyrilletuzi

This comment has been minimized.

Show comment
Hide comment
@cyrilletuzi

cyrilletuzi Jan 18, 2018

Contributor

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

Contributor

cyrilletuzi commented Jan 18, 2018

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

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