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

feat(core): Allow typeguards on QueryList.filter #48042

Closed

Conversation

JeanMeche
Copy link
Member

To match the behaviour of Array.filter, typeguards can now be used on QueryList.filter to narrow the return type.

Fixes #38446

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Nov 14, 2022
@JeanMeche JeanMeche force-pushed the feature/query-list-filter-typeguard branch from b6b16fd to c6c458e Compare November 14, 2022 09:18
@JeanMeche
Copy link
Member Author

JeanMeche commented Nov 14, 2022

I'm also thinking about including the fix for #39602 in this PR.

Edit: No need for that, it was fixed from TS 4.2 onwards.

@jessicajaniuk jessicajaniuk added the area: core Issues related to the framework runtime label Nov 14, 2022
@ngbot ngbot bot added this to the Backlog milestone Nov 14, 2022
@AndrewKushnir
Copy link
Contributor

Just a quick note to myself: the typings for the Array.filter are here: https://github.com/microsoft/TypeScript/blob/main/lib/lib.es5.d.ts#L1256

@AndrewKushnir AndrewKushnir modified the milestones: Backlog, v16-candidates Nov 30, 2022
@AndrewKushnir AndrewKushnir added feature Issue that requests a new feature breaking changes action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Nov 30, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeanMeche thanks for the PR! I've checked that the typings of the QueryList.filter are similar to the typings of the Array.filter method.

This change might introduce a breaking change in apps code. The possibility is very low, so we probably won't need a schematic to migrate the code (we'll be able to tell precisely after running tests in Google's codebase), but we'd be able to include this change only into the next major version (v16) according to the SemVer. I've added this PR to the "v16-candidates" Github milestone and we'll get back to this PR once the main branch is open for v16 changes (which will happen after releasing the last v15.x minor version).

Thank you.

@AndrewKushnir AndrewKushnir self-assigned this Mar 6, 2023
@AndrewKushnir AndrewKushnir added action: global presubmit The PR is in need of a google3 global presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 6, 2023
@AndrewKushnir
Copy link
Contributor

Global Presubmit.

@pkozlowski-opensource
Copy link
Member

@AndrewKushnir this look it is good to go, right?

@AndrewKushnir AndrewKushnir force-pushed the feature/query-list-filter-typeguard branch from c6c458e to d6e4993 Compare March 30, 2023 23:29
@AndrewKushnir
Copy link
Contributor

@pkozlowski-opensource yes, it should be ready for merge after one more TGP (just in case). Will update the PR once I have TGP results.

@AndrewKushnir
Copy link
Contributor

Global Presubmit #2.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: fw-core, public-api

Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reviewed-for: public-api

@AndrewKushnir AndrewKushnir force-pushed the feature/query-list-filter-typeguard branch from d6e4993 to ee0728c Compare March 31, 2023 16:45
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Mar 31, 2023
@AndrewKushnir AndrewKushnir removed the action: global presubmit The PR is in need of a google3 global presubmit label Mar 31, 2023
@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 31, 2023
packages/core/src/linker/query_list.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 31, 2023
@JeanMeche JeanMeche force-pushed the feature/query-list-filter-typeguard branch from ee0728c to 57d8823 Compare March 31, 2023 17:14
@JeanMeche JeanMeche requested a review from alxhub March 31, 2023 17:14
To match the behaviour of Array.filter, typeguards can now be used on QueryList.filter to narrow the return type.

Fixes angular#38446

BREAKING CHANGE: QueryList.filter now supports type guard functions, which will result in type narrowing. Previously if you used type guard functions, it resulted in no changes to the return type. Now the type would be narrowed, which might require updates to the application code that relied on the old behavior.
@AndrewKushnir AndrewKushnir force-pushed the feature/query-list-filter-typeguard branch from 57d8823 to a5fd73e Compare April 1, 2023 02:18
@AndrewKushnir
Copy link
Contributor

Global Presubmit #3.

Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed-for: public-api

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Apr 3, 2023

Caretaker note: TGP is "green", this PR is ready for merge.

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Apr 3, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Apr 4, 2023

This PR was merged into the repository by commit b2327f4.

@dylhunn dylhunn closed this in b2327f4 Apr 4, 2023
@JeanMeche JeanMeche deleted the feature/query-list-filter-typeguard branch April 4, 2023 06:16
@angular-automatic-lock-bot
Copy link

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 May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime breaking changes detected: breaking change PR contains a commit with a breaking change detected: feature PR contains a feature commit feature Issue that requests a new feature merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

QueryList's filter method doesn't support type guards
6 participants