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(core): make QueryList implement Iterable in the type system #33536

Closed
wants to merge 1 commit into from

Conversation

@alxhub
Copy link
Contributor

alxhub commented Nov 1, 2019

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

@alxhub alxhub requested review from angular/fw-compiler as code owners Nov 1, 2019
@googlebot googlebot added the cla: yes label Nov 1, 2019
@alxhub alxhub force-pushed the alxhub:core/querylist-iterable branch 2 times, most recently from 10fafd5 to 2a35376 Nov 1, 2019
@alxhub alxhub changed the title fix(ivy): make QueryList implement Iterable in the type system fix(core): make QueryList implement Iterable in the type system Nov 1, 2019
@mhevery
mhevery approved these changes Nov 1, 2019
@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Nov 2, 2019

@alxhub should this change be included in patch branch as well? Thank you.

Copy link
Member

Splaktar left a comment

LGTM!

This would allow me to remove quite a number of someQueryList.toArray() calls in templates with Ivy.

// 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<T>;

This comment has been minimized.

Copy link
@Splaktar

Splaktar Nov 3, 2019

Member

Very interesting! For anyone else not completely familiar with using TypeScript Symbols to define class members, here's the docs: https://www.typescriptlang.org/docs/handbook/symbols.html

@ngbot ngbot bot added this to the needsTriage milestone Nov 4, 2019
@alxhub alxhub force-pushed the alxhub:core/querylist-iterable branch from 2a35376 to 70a0098 Nov 18, 2019
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
@alxhub alxhub force-pushed the alxhub:core/querylist-iterable branch from 70a0098 to fa75d1a Nov 18, 2019
@alxhub alxhub requested a review from angular/dev-infra-framework as a code owner Nov 18, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor Author

alxhub commented Nov 19, 2019

Presubmit
No Ivy presubmit required; VE tests are sufficient

@alxhub alxhub closed this in bb290ce Nov 19, 2019
alxhub added a commit that referenced this pull request Nov 19, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.