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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type check directive template context #31556

Closed
pauldraper opened this issue Jul 14, 2019 · 8 comments
Closed

Type check directive template context #31556

pauldraper opened this issue Jul 14, 2019 · 8 comments

Comments

@pauldraper
Copy link

@pauldraper pauldraper commented Jul 14, 2019

馃殌 feature request

Relevant Package

@angular/compiler

Description

Template contexts appear not to be checked.

    <ng-container *ngIf="data(); let d">
      {{ d.a }}
      {{ d.c }}
    </ng-container>

There is zero checking for the type of d.

Example: https://github.com/pauldraper/angular-template-context I would like ng build to fail.

Describe the solution you'd like

Template context types are type checked.

Describe alternatives you've considered

Don't use template contexts.

I've also tried using Ivy but it seems broken.

ERROR in __ng_typecheck__.ts:5:22 - error TS2304: Cannot find name 'Partial'.

5 const _ctor1: (init: Partial<Pick<i1.NgIf, "ngIf" | "ngIfThen" | "ngIfElse">>) => i1.NgIf = (null!);
@pauldraper pauldraper changed the title Type check template context Type check directive template context Jul 14, 2019
@alexzuza

This comment has been minimized.

Copy link
Contributor

@alexzuza alexzuza commented Jul 15, 2019

Angular Ivy type-checking system uses Partial and Pick built-in TypeScript types. They are declared in lib.es5.d.ts which means that you have to provide at least es5 in lib option of your tsconfig.json file:

{
  "angularCompilerOptions": {
    "fullTemplateTypeCheck": true,
    "enableIvy": true
  },
  "compilerOptions": {
    "moduleResolution": "node",
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "target": "es5",
    "lib": ["dom", "es5"]
                     \/
                  add this               
  }
}

Once you've done, the compiler will be executed without any errors.

Why?
Angular Ivy type-checking system for ngIf only supports ngIf guard but not template context guard.
The generated code for your case looks like:

import * as i0 from './app.component';
import * as i1 from '@angular/common';

const _ctor1: (init: Partial<Pick<i1.NgIf, "ngIf" | "ngIfThen" | "ngIfElse">>) => i1.NgIf = (null!);

function _tcb1(ctx: i0.AppComponent) { if (true) {
    var _t1 = _ctor1({ ngIf: ctx.data() });
    var _t2: any = (null!);
    if (ctx.data()) {
        var _t3 = _t2.$implicit;
        var _t4 = document.createElement("ng-container");
        "" + _t3.a + _t3.c;
    }
} }

where the type of _t3 variable is any.

The NgIf directive doesn't implement ngTemplateContextGuard feature like NgForOf does.
What could work here is:

interface NgIfContext<T = any> {
  $implicit: T;
}

class GuardedNgIf<T> {
  ngIf: T;
  static ngTemplateContextGuard<T>(dir: GuardedNgIf<T>, ctx: any): ctx is NgIfContext<T> {
    return true;
  }
}
const _ctor1: <T = any>(init: Partial<Pick<GuardedNgIf<T>, "ngIf">>) => GuardedNgIf<T> = (null!);

function _tcb1(ctx: AppComponent) { 
  var _t1 = _ctor1({ ngIf: ctx.data() });
  var _t2: any = (null!);
  if (GuardedNgIf.ngTemplateContextGuard(_t1, _t2)) {
    var _t3 = _t2.$implicit;

    "" + _t3.a + _t3.c; // Error:(47, 22) TS2339: Property 'c' does not exist on type '{ a: string; b: string; }'.
  }
}

// cc @JoostK

@JoostK

This comment has been minimized.

Copy link
Member

@JoostK JoostK commented Jul 15, 2019

@alexzuza thanks for the explanation! :-) Would you be able to open a PR with your proposed changes?

@pauldraper I would be interested to know how you ended up without lib.es5.d.ts, if that is indeed the cause of the Ivy error. The reason why I'm asking is that perhaps we should not assume that these typings are available and have the type checker output its own definitions.

@alexzuza

This comment has been minimized.

Copy link
Contributor

@alexzuza alexzuza commented Jul 15, 2019

@alexzuza thanks for the explanation! :-) Would you be able to open a PR with your proposed changes?

@JoostK Will do

alexzuza added a commit to alexzuza/angular that referenced this issue Jul 15, 2019
directive

This commits adds the `ngTemplateContextGuard` static method to the NgIf
directive.
The function of this static method is to narrow the template rendering
context variable `ctx`
within generated type checking code for consumers of the directive.

Previously, the ngIf only supported `ngTemplateGuard_ngIf` guard which
narrows the expression passed to an @input of the directive.

The new behaviour is to constrain a NgIf template reference variable so that for the following template:

```html
<div *ngIf="{ foo: 'bar' }; let x">
  {{ x.unknown }}
</div>
```
the compiler will generate the type-check block (TCB):

```typescript
if (NgIf.ngTemplateContextGuard(_t1, _t2) && { "foo": "bar" }) {
  var _t3 = _t2.$implicit;
  var _t4 = document.createElement("div");
  "" + _t3.unknown;
}
```
and it will fail to compile with the error `Property 'unknown' does not
exist on type '{ "foo": string; }'`

As part of the changes, the `NgIf` directive and `NgIfContext` class
become generic with default type `any`.

PR Close angular#31556
@ngbot ngbot bot added this to the needsTriage milestone Jul 17, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 18, 2019
@pauldraper

This comment has been minimized.

Copy link
Author

@pauldraper pauldraper commented Jul 19, 2019

@JoostK I'm not sure. You can take a look at my rather minimal example.

https://github.com/pauldraper/angular-template-context

Add "enableIvy": true to src/tsconfig.json and then run ng build.

@pauldraper

This comment has been minimized.

Copy link
Author

@pauldraper pauldraper commented Jul 26, 2019

On the Ivy point, oh, I see. My example has lib: "dom" but not "es5".

Alright, after fixing that, even with Angular Ivy this code still validates.

And based on the documentation for NgIf.ngTemplateGuard_ngIf, it appears this is supposed to be type checked.

https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_if.ts#L219-L227

EDIT: Oh, I see your comments.

@simeyla

This comment has been minimized.

Copy link

@simeyla simeyla commented Aug 10, 2019

Even without Ivy you can sort of achieve this today (8.1.2), if you use *ngFor instead of *ngIf....

This is quite a clumsy hack - but works to achieve what the OP was trying to do:

export type Product = { sku: string, price: number };

// helper function to take a single product and convert it to the correct type 
// this works even for a `*ngTemplateOutletContext` parameter which is why I needed this
coerceProductArray(product: any): Product[]
{
    return [ product ];   // creates a single item array
}

Then iterate through this list - (of one item) - and the correct type for product will be available inside. The attempted multiplication of a string below will not be allowed!

<ng-container *ngFor="let product of coerceProductArray(product)">

    <!-- Compiles -->
    {{ product.sku }}
    {{ product.price }}
    {{ product.price * 2 }}

    <!-- These have squiggles! Yay -->
    {{ product.desc }}
    {{ product.price + 'cat' }}
    {{ product.sku * 2 }}

</ng-container>

What I'm really having a hard time understanding is why ng_for_of.ts is generic, but ng_if.ts isn't. Is it just not possible for *ngIf to use the same mechanism that *ngFor does to achieve this.

What's the reason ng_if isn't a generic type today? Where does Ivy help with this in future - will this problem just go away some day. (And yes - I too was very confused by the comments highlighted in the previous post).

I was about to embark on creating my own *ngIf but I figured it would be a futile exercise until i can understand better what's going on - this seemed to be the closest issue.

Thanks

@pauldraper

This comment has been minimized.

Copy link
Author

@pauldraper pauldraper commented Aug 23, 2019

Interesting. I didn't not know it was possible with ngForOf.

ngIf simply makes no attempt for generic types, so it very unsurprising it is untyped.

crisbeto added a commit to crisbeto/angular that referenced this issue Nov 29, 2019
Fixes the content of `NgIf` being typed to any.

Fixes angular#31556.
mhevery added a commit that referenced this issue Dec 2, 2019
Fixes the content of `NgIf` being typed to any.

Fixes #31556.

PR Close #33997
@mhevery mhevery closed this in 02958c0 Dec 2, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

@angular-automatic-lock-bot angular-automatic-lock-bot bot commented Jan 2, 2020

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 Jan 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.