From bb290cefae154fc7a2b0903b8371da4eb4adebc0 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Fri, 1 Nov 2019 12:31:56 -0700 Subject: [PATCH] fix(core): make QueryList implement Iterable in the type system (#33536) Originally, QueryList implemented Iterable and provided a Symbol.iterator on its prototype. This caused issues with tree-shaking, so QueryList was refactored and the Symbol.iterator added in its constructor instead. As part of this change, QueryList no longer implemented Iterable directly. Unfortunately, this meant that QueryList was no longer assignable to Iterable or, consequently, NgIterable. NgIterable is used for NgFor's input, so this meant that QueryList was not usable (in a type sense) for NgFor iteration. View Engine's template type checking would not catch this, but Ivy's did. As a fix, this commit adds the declaration (but not the implementation) of the Symbol.iterator function back to QueryList. This has no runtime effect, so it doesn't affect tree-shaking of QueryList, but it ensures that QueryList is assignable to NgIterable and thus usable with NgFor. Fixes #29842 PR Close #33536 --- .../test/ngtsc/fake_core/index.ts | 4 +++ .../test/ngtsc/template_typecheck_spec.ts | 28 +++++++++++++++++-- packages/core/src/linker/query_list.ts | 9 +++++- packages/core/test/linker/query_list_spec.ts | 16 +++++++++++ tools/public_api_guard/core/core.d.ts | 3 +- tools/ts-api-guardian/index.bzl | 2 +- 6 files changed, 57 insertions(+), 5 deletions(-) diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts index 884553dcad..5f1a45dc80 100644 --- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts +++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts @@ -85,3 +85,7 @@ export const NO_ERRORS_SCHEMA: any = false; export class EventEmitter { subscribe(generatorOrNext?: any, error?: any, complete?: any): unknown { return null; } } + +export interface QueryList/* implements Iterable */ { [Symbol.iterator]: () => Iterator; } + +export type NgIterable = Array| Iterable; diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index a7d4cb52fb..d583fc1e0d 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -28,7 +28,7 @@ import * as i0 from '@angular/core'; export declare class NgForOfContext { $implicit: T; - ngForOf: T[]; + ngForOf: i0.NgIterable; index: number; count: number; readonly first: boolean; @@ -44,7 +44,7 @@ export declare class IndexPipe { } export declare class NgForOf { - ngForOf: T[]; + ngForOf: i0.NgIterable; static ngTemplateContextGuard(dir: NgForOf, ctx: any): ctx is NgForOfContext; static ɵdir: i0.ɵɵDirectiveDefWithMeta, '[ngFor][ngForOf]', never, {'ngForOf': 'ngForOf'}, {}, never>; } @@ -714,6 +714,30 @@ export declare class AnimationEvent { env.driveMain(); }); + it('should accept NgFor iteration over a QueryList', () => { + env.tsconfig({fullTemplateTypeCheck: true, strictTemplates: true}); + env.write('test.ts', ` + import {CommonModule} from '@angular/common'; + import {Component, NgModule, QueryList} from '@angular/core'; + + @Component({ + selector: 'test', + template: '
{{user.name}}
', + }) + class TestCmp { + users!: QueryList<{name: string}>; + } + + @NgModule({ + declarations: [TestCmp], + imports: [CommonModule], + }) + class Module {} + `); + + env.driveMain(); + }); + it('should report an error with an unknown local ref target', () => { env.write('test.ts', ` import {Component, NgModule} from '@angular/core'; diff --git a/packages/core/src/linker/query_list.ts b/packages/core/src/linker/query_list.ts index f8af2b7e1a..f89d35cd42 100644 --- a/packages/core/src/linker/query_list.ts +++ b/packages/core/src/linker/query_list.ts @@ -42,7 +42,7 @@ function symbolIterator(this: QueryList): Iterator { * * @publicApi */ -export class QueryList/* implements Iterable */ { +export class QueryList implements Iterable { public readonly dirty = true; private _results: Array = []; public readonly changes: Observable = new EventEmitter(); @@ -142,4 +142,11 @@ export class QueryList/* implements Iterable */ { (this.changes as EventEmitter).complete(); (this.changes as EventEmitter).unsubscribe(); } + + // The implementation of `Symbol.iterator` should be declared here, but this would cause + // tree-shaking issues with `QueryList. So instead, it's added in the constructor (see comments + // there) and this declaration is left here to ensure that TypeScript considers QueryList to + // implement the Iterable interface. This is required for template type-checking of NgFor loops + // over QueryLists to work correctly, since QueryList must be assignable to NgIterable. + [Symbol.iterator] !: () => Iterator; } diff --git a/packages/core/test/linker/query_list_spec.ts b/packages/core/test/linker/query_list_spec.ts index c344f30f5a..b538271839 100644 --- a/packages/core/test/linker/query_list_spec.ts +++ b/packages/core/test/linker/query_list_spec.ts @@ -135,6 +135,22 @@ import {beforeEach, describe, expect, it} from '@angular/core/testing/src/testin expect(queryList.some(item => item === 'four')).toEqual(false); }); + it('should be iterable', () => { + const data = ['one', 'two', 'three']; + queryList.reset([...data]); + + // The type here is load-bearing: it asserts that queryList is considered assignable to + // Iterable in TypeScript. This is important for template type-checking of *ngFor + // when looping over query results. + const queryListAsIterable: Iterable = queryList; + + // For loops use the iteration protocol. + for (const value of queryListAsIterable) { + expect(value).toBe(data.shift() !); + } + expect(data.length).toBe(0); + }); + if (getDOM().supportsDOMEvents()) { describe('simple observable interface', () => { it('should fire callbacks on change', fakeAsync(() => { diff --git a/tools/public_api_guard/core/core.d.ts b/tools/public_api_guard/core/core.d.ts index 02f47bd571..22d1a7d156 100644 --- a/tools/public_api_guard/core/core.d.ts +++ b/tools/public_api_guard/core/core.d.ts @@ -1136,7 +1136,8 @@ export interface Query { export declare abstract class Query { } -export declare class QueryList { +export declare class QueryList implements Iterable { + [Symbol.iterator]: () => Iterator; readonly changes: Observable; readonly dirty = true; readonly first: T; diff --git a/tools/ts-api-guardian/index.bzl b/tools/ts-api-guardian/index.bzl index c1ffb5d7dd..041754f9a5 100644 --- a/tools/ts-api-guardian/index.bzl +++ b/tools/ts-api-guardian/index.bzl @@ -17,7 +17,7 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary", "nodejs_test") -COMMON_MODULE_IDENTIFIERS = ["angular", "jasmine", "protractor"] +COMMON_MODULE_IDENTIFIERS = ["angular", "jasmine", "protractor", "Symbol"] def ts_api_guardian_test( name,