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(language-service): determine correct type for ngFor exported values #34089

Closed
wants to merge 2 commits into from

Conversation

@ayazhafiz
Copy link
Member

ayazhafiz commented Nov 27, 2019

Currently, variables of an unknown type in an *ngFor expression are
refined to have the type of the iterable binding of the *ngFor
expression. Unfortunately, this is a bug for variables aliasing
values exported by *ngFor,
including index and first, because they are also given the type of
the binding expression, but they are not of the binding type. For
example, in

@Component({
  selector: 'test',
  template: `
    <div *ngFor="let hero of heroes; let i = index; let isFirst = first">
      {{ hero }}
    </div>
  `
})
export class TestComponent {
  heroes: Hero[];
}

The local variables i and isFirst are determined to have a type of
Hero, when actually their types are number and boolean,
respectively.

This commit fixes this bug by checking if the value of a variable in an
*ngFor expression is known to be an export and assigning the variable
the type of that export value. Only if the variable does not alias an
export is it typed with the binding value of the *ngFor expression.

Closes angular/vscode-ng-language-service#460

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No
Currently, variables of an unknown type in an `*ngFor` expression are
refined to have the type of the iterable binding of the `*ngFor`
expression. Unfortunately, this is a bug for variables aliasing
[values exported by
`*ngFor`](https://angular.io/api/common/NgForOf#local-variables),
including `index` and `first`, because they are also given the type of
the binding expression, but they are not of the binding type. For
example, in

```typescript
@component({
  selector: 'test',
  template: `
    <div *ngFor="let hero of heroes; let i = index; let isFirst = first">
      {{ hero }}
    </div>
  `
})
export class TestComponent {
  heroes: Hero[];
}
```

The local variables `i` and `isFirst` are determined to have a type of
`Hero`, when actually their types are `number` and `boolean`,
respectively.

This commit fixes this bug by checking if the value of a variable in an
`*ngFor` expression is known to be an export and assigning the variable
the type of that export value. Only if the variable does not alias an
export is it typed with the binding value of the `*ngFor` expression.

Closes angular/vscode-ng-language-service#460
@ayazhafiz ayazhafiz requested a review from kyliau Nov 27, 2019
@ayazhafiz ayazhafiz requested a review from angular/tools-language-service as a code owner Nov 27, 2019
@ayazhafiz ayazhafiz self-assigned this Nov 27, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 27, 2019
@googlebot googlebot added the cla: yes label Nov 27, 2019
@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 27, 2019

Why does the language service have special knowledge of ngFor at all? Shouldn't it infer proper types for all structural directives?

…ed values
@ayazhafiz ayazhafiz requested a review from kyliau Nov 27, 2019
@kyliau

This comment has been minimized.

Copy link
Member

kyliau commented Nov 27, 2019

Why does the language service have special knowledge of ngFor at all? Shouldn't it infer proper types for all structural directives?

For historical reasons, the language service does not share the same code path as the compiler for some of the checks. This is a problem that we are aware of, and we plan to fully integrate the Ivy compiler into the language service in version 10.

We are tracking this work in this issue angular/vscode-ng-language-service#380

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Nov 27, 2019

Why does the language service have special knowledge of ngFor at all?

To build on Keen's comment above, for the particular case of ngFor, see angular/vscode-ng-language-service#144. Some syntaxes of ngFor do not allow for immediately-evident variable types with the parsing pipeline the language service currently runs.

@Airblader

This comment has been minimized.

Copy link
Contributor

Airblader commented Nov 27, 2019

Thanks, I appreciate both of you taking the time to explain!

@kyliau
kyliau approved these changes Nov 28, 2019
@mhevery mhevery closed this in 1425e63 Dec 2, 2019
mhevery added a commit that referenced this pull request Dec 2, 2019
…es (#34089)

Currently, variables of an unknown type in an `*ngFor` expression are
refined to have the type of the iterable binding of the `*ngFor`
expression. Unfortunately, this is a bug for variables aliasing
[values exported by
`*ngFor`](https://angular.io/api/common/NgForOf#local-variables),
including `index` and `first`, because they are also given the type of
the binding expression, but they are not of the binding type. For
example, in

```typescript
@component({
  selector: 'test',
  template: `
    <div *ngFor="let hero of heroes; let i = index; let isFirst = first">
      {{ hero }}
    </div>
  `
})
export class TestComponent {
  heroes: Hero[];
}
```

The local variables `i` and `isFirst` are determined to have a type of
`Hero`, when actually their types are `number` and `boolean`,
respectively.

This commit fixes this bug by checking if the value of a variable in an
`*ngFor` expression is known to be an export and assigning the variable
the type of that export value. Only if the variable does not alias an
export is it typed with the binding value of the `*ngFor` expression.

Closes angular/vscode-ng-language-service#460

PR Close #34089
@ayazhafiz ayazhafiz deleted the ayazhafiz:fix/template-index-value branch Dec 2, 2019
@angular-automatic-lock-bot

This comment has been minimized.

Copy link

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.
You can’t perform that action at this time.