fix(common): infer correct type when `trackBy` is used in `ngFor` #41995

JoostK · 2021-05-07T20:39:45Z

See individual commits

Fixes #40125

jessicajaniuk

LGTM 🍪

reviewed-for: public-api

jelbourn

LGTM

Reviewed-for: public-api

jelbourn · 2021-05-10T22:50:28Z

packages/core/src/change_detection/differs/iterable_differs.ts

@@ -121,7 +121,7 @@ export interface IterableChangeRecord<V> {
 * @publicApi
 */
 export interface TrackByFunction<T> {
-  (index: number, item: T): any;
+  <U extends T>(index: number, item: U): any;


Could you add a comment here that adds context to <U extends T> with a link to the issue?

petebacondarwin

Reviewed-for: public-api

petebacondarwin

Reviewed-for: public-api

jessicajaniuk

LGTM 🍪

reviewed-for: public-api

The ngtsc test targets have fake declarations files for `@angular/core` and `@angular/common` and the template type checking tests can leverage the fake common declarations instead of declaring its own types.

…e_core` The `TrackByFunction` is declared in `@angular/core` so it should also be included in `fake_core` instead of `fake_common`.

When a `trackBy` function is used that accepts a supertype of the iterated array's type, the loop variable would undesirably be inferred as the supertype instead of the array's item type. This commit adds an inferred type parameter to `TrackByFunction` to allow an extra degree of freedom, enabling the loop value to be inferred as the most narrow type. Fixes angular#40125

JoostK · 2021-06-02T16:30:31Z

@AndrewKushnir This one is ready for another round of presubmit.

AndrewKushnir · 2021-06-03T02:24:45Z

Global Presubmit #2.

AndrewKushnir · 2021-06-03T15:32:48Z

@JoostK presubmits (including a global one) went well and this PR looks ready for merge (just needs the "merge" label).

…e_core` (#41995) The `TrackByFunction` is declared in `@angular/core` so it should also be included in `fake_core` instead of `fake_common`. PR Close #41995

…1995) When a `trackBy` function is used that accepts a supertype of the iterated array's type, the loop variable would undesirably be inferred as the supertype instead of the array's item type. This commit adds an inferred type parameter to `TrackByFunction` to allow an extra degree of freedom, enabling the loop value to be inferred as the most narrow type. Fixes #40125 PR Close #41995

The ngtsc test targets have fake declarations files for `@angular/core` and `@angular/common` and the template type checking tests can leverage the fake common declarations instead of declaring its own types. PR Close #41995

…e_core` (#41995) The `TrackByFunction` is declared in `@angular/core` so it should also be included in `fake_core` instead of `fake_common`. PR Close #41995

…1995) When a `trackBy` function is used that accepts a supertype of the iterated array's type, the loop variable would undesirably be inferred as the supertype instead of the array's item type. This commit adds an inferred type parameter to `TrackByFunction` to allow an extra degree of freedom, enabling the loop value to be inferred as the most narrow type. Fixes #40125 PR Close #41995

…ByFunction Previously `DefaultIterableDiffer` was able to infer the type of it's generic by reading the type contract defined by the user defined trackBy function. This was because `TrackByFunction` can be specified with a generic that is used to type its second argument. If this generic is not provided, like in this case, typescript infers this type from the function contract directly. In this case this inferred type was `FlatNode`. This inference is then propagated to `DefaultIterableDiffer`, which is why we were not seeing this error previously. A change in the framework made it so the second argument of `TrackByFunction` is typed as a generic that extends the generic passed into `TrackByFunction`. Since we had not previously passed in this generic to `TrackByFunction`, this type was inferred as an extension of `unknown`, causing a type error when passed as an argument to the `DefaultIterableDiffer` constructor. Now the functions being used are typed directly as `TrackByFunction<FlatNode>` For clarity, the `DefaultIterableDiffer` generic is now also typed as `FlatNode`. See angular/angular#41995 for the framework change.

…ByFunction (#862) Previously `DefaultIterableDiffer` was able to infer the type of it's generic by reading the type contract defined by the user defined trackBy function. This was because `TrackByFunction` can be specified with a generic that is used to type its second argument. If this generic is not provided, like in this case, typescript infers this type from the function contract directly. In this case this inferred type was `FlatNode`. This inference is then propagated to `DefaultIterableDiffer`, which is why we were not seeing this error previously. A change in the framework made it so the second argument of `TrackByFunction` is typed as a generic that extends the generic passed into `TrackByFunction`. Since we had not previously passed in this generic to `TrackByFunction`, this type was inferred as an extension of `unknown`, causing a type error when passed as an argument to the `DefaultIterableDiffer` constructor. Now the functions being used are typed directly as `TrackByFunction<FlatNode>` For clarity, the `DefaultIterableDiffer` generic is now also typed as `FlatNode`. See angular/angular#41995 for the framework change.

…rackBy` function In angular#41995 the type of `TrackByFunction` was changed such that the declaration of a `trackBy` function did not cause the item type to be widened to the `trackBy`'s item type, which may be a supertype of the iterated type. This has introduced situations where the template type checker is now reporting errors for cases where a `trackBy` function is no longer assignable to `TrackByFunction`. This commit fixes the error by also including the item type `T` in addition to the constrained type parameter `U`, allowing TypeScript to infer an appropriate `T`. Fixes angular#42609

angular-automatic-lock-bot · 2021-07-04T16:05:49Z

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

JoostK added target: patch This PR is targeted for the next patch release area: compiler Issues related to `ngc`, Angular's template compiler P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent compiler: template type-checking labels May 7, 2021

ngbot bot modified the milestone: Backlog May 7, 2021

google-cla bot added the cla: yes label May 7, 2021

JoostK added the area: common Issues related to APIs in the @angular/common package label May 7, 2021

JoostK changed the title ~~Type checking track by~~ fix(common): infer correct type when trackBy is used in ngFor May 7, 2021

JoostK force-pushed the type-checking-track-by branch from 244caaa to 818fc8e Compare May 7, 2021 21:07

JoostK marked this pull request as ready for review May 7, 2021 21:53

pullapprove bot requested a review from jessicajaniuk May 7, 2021 21:53

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 7, 2021

jessicajaniuk approved these changes May 10, 2021

View reviewed changes

pullapprove bot requested review from alxhub, IgorMinar, jelbourn and petebacondarwin May 10, 2021 18:07

jelbourn approved these changes May 10, 2021

View reviewed changes

pullapprove bot requested a review from jessicajaniuk May 10, 2021 22:50

petebacondarwin reviewed May 11, 2021

View reviewed changes

petebacondarwin approved these changes May 11, 2021

View reviewed changes

pullapprove bot requested a review from atscott May 11, 2021 14:02

jessicajaniuk approved these changes May 11, 2021

View reviewed changes

JoostK force-pushed the type-checking-track-by branch 2 times, most recently from 43cfc55 to aa60c0b Compare May 13, 2021 18:49

JoostK added the action: rerun CI at HEAD label May 13, 2021

ngbot bot removed the action: rerun CI at HEAD label May 13, 2021

JoostK force-pushed the type-checking-track-by branch from aa60c0b to f8d277f Compare May 14, 2021 11:29

AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed state: blocked labels Jun 2, 2021

JoostK added 3 commits June 2, 2021 17:29

test(compiler-cli): load type declarations from fake_common

20db07d

The ngtsc test targets have fake declarations files for `@angular/core` and `@angular/common` and the template type checking tests can leverage the fake common declarations instead of declaring its own types.

test(compiler-cli): move TrackByFunction from fake_common to `fak…

01537d4

…e_core` The `TrackByFunction` is declared in `@angular/core` so it should also be included in `fake_core` instead of `fake_common`.

JoostK force-pushed the type-checking-track-by branch from f8d277f to c298698 Compare June 2, 2021 15:31

JoostK removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jun 2, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 3, 2021

AndrewKushnir approved these changes Jun 3, 2021

View reviewed changes

AndrewKushnir removed request for IgorMinar and atscott June 3, 2021 15:32

JoostK added the action: merge The PR is ready for merge by the caretaker label Jun 3, 2021

jessicajaniuk closed this in 481540d Jun 3, 2021

JoostK mentioned this pull request Jun 21, 2021

Angular 12 build error - Type '(index: any, item: DropDownItem<any>) => string' is not assignable to type 'TrackByFunction<any>' #42609

Closed

1 task

AleksanderBodurri mentioned this pull request Jun 21, 2021

fix: update typing of differ objects to fit the new contract of TrackByFunction rangle/angular-devtools#862

Merged

JoostK mentioned this pull request Jun 28, 2021

fix(core): allow proper type inference when ngFor is used with a trackBy function #42692

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(common): infer correct type when `trackBy` is used in `ngFor` #41995

fix(common): infer correct type when `trackBy` is used in `ngFor` #41995

JoostK commented May 7, 2021

jessicajaniuk left a comment •

edited

jelbourn left a comment

jelbourn May 10, 2021

petebacondarwin left a comment

petebacondarwin left a comment

jessicajaniuk left a comment

JoostK commented Jun 2, 2021

AndrewKushnir commented Jun 3, 2021

AndrewKushnir commented Jun 3, 2021

angular-automatic-lock-bot bot commented Jul 4, 2021

fix(common): infer correct type when trackBy is used in ngFor #41995

fix(common): infer correct type when trackBy is used in ngFor #41995

Conversation

JoostK commented May 7, 2021

jessicajaniuk left a comment • edited

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn May 10, 2021

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment

Choose a reason for hiding this comment

JoostK commented Jun 2, 2021

AndrewKushnir commented Jun 3, 2021

AndrewKushnir commented Jun 3, 2021

angular-automatic-lock-bot bot commented Jul 4, 2021

fix(common): infer correct type when `trackBy` is used in `ngFor` #41995

fix(common): infer correct type when `trackBy` is used in `ngFor` #41995

jessicajaniuk left a comment •

edited