refactor(forms): Fix weak helper types in the Forms package by dylhunn · Pull Request #44370 · angular/angular

dylhunn · 2021-12-03T18:39:03Z

Fix weak helper types in the Forms package, mostly Function types that can be strengthened to specific callbacks. This was originally proposed as part of Typed Forms, but it should be possible to submit at least some of these changes separately and unconditionally (i.e. without relying on TypedOrUntyped.)

It is desirable to land this separately to reduce the scope of the Typed Forms PR, by focusing it only on the new type parameters (rather than incidental strictness fixes).

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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:

What is the current behavior?

Many forms methods have unnecessarily weak types (e.g. Function).

Issue Number: #13721

What is the new behavior?

The types of these methods are strengthened to specific callbacks.

Does this PR introduce a breaking change?

No

Other information

dylhunn · 2021-12-03T18:42:23Z

Presubmit for this change is green.

jessicajaniuk

LGTM 🍪

reviewed-for: public-api

AndrewKushnir

@dylhunn thanks for improving types! 👍
The change looks great, just left a couple comments/suggestions.

packages/forms/src/directives/shared.ts

packages/forms/src/model.ts

AndrewKushnir

LGTM, thanks @dylhunn 👍

AndrewKushnir · 2021-12-07T19:05:49Z

packages/forms/src/model.ts

I believe we could also specify the return type?

Suggested change

_reduceChildren<T>(initValue: T, fn: (acc: T, control: AbstractControl, name: string) => T) {

_reduceChildren<T>(initValue: T, fn: (acc: T, control: AbstractControl, name: string) => T): T {

packages/forms/src/model.ts

AndrewKushnir

Reviewed-for: public-api

dylhunn · 2021-12-08T17:45:43Z

@alxhub I'm actually just going to drop the public API change from this PR -- I think it's still worth fixing the @internal methods to clean things up, and we can do the public method in the next major.

alxhub · 2021-12-10T21:35:15Z

FYI, this refuses to merge because the commit type is feat, but the PR is target: patch. We should change the commit to a refactor.

…Function` types that can be strengthened to specific callbacks. This was originally proposed as part of Typed Forms, but it may be possible to submit at least some of these changes separately and unconditionally (i.e. without relying on `TypedOrUntyped`.) It is desirable to land this separately to reduce the scope of the Typed Forms PR, by focusing it only on the new type parameters (rather than incidental strictness fixes).

dylhunn · 2021-12-10T22:02:33Z

@alxhub Fixed.

alxhub · 2021-12-10T22:13:26Z

This PR was merged into the repository by commit 7df9127.

…Function` types that can be strengthened to specific callbacks. This was originally proposed as part of Typed Forms, but it may be possible to submit at least some of these changes separately and unconditionally (i.e. without relying on `TypedOrUntyped`.) (#44370) It is desirable to land this separately to reduce the scope of the Typed Forms PR, by focusing it only on the new type parameters (rather than incidental strictness fixes). PR Close #44370

I previously strengthened some weak types in angular#44370. One of these fixes exposed an incorrect call into `_reduceChildren` from `_reduceValue`. This was caught in google3 by a caller who was extending `FormGroup` and overriding these methods. Special thanks to Bart G for catching this issue and suggesting a fix.

) I previously strengthened some weak types in #44370. One of these fixes exposed an incorrect call into `_reduceChildren` from `_reduceValue`. This was caught in google3 by a caller who was extending `FormGroup` and overriding these methods. Special thanks to Bart G for catching this issue and suggesting a fix. PR Close #44483

angular-automatic-lock-bot · 2022-01-14T16:10:46Z

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

google-cla bot added the cla: yes label Dec 3, 2021

dylhunn force-pushed the typed-cleanups branch from e8884fd to 1d7a967 Compare December 3, 2021 18:44

dylhunn added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms cross-cutting: types labels Dec 3, 2021

ngbot bot modified the milestone: Backlog Dec 3, 2021

dylhunn added the target: minor This PR is targeted for the next minor release label Dec 3, 2021

dylhunn marked this pull request as ready for review December 3, 2021 19:47

pullapprove bot requested review from AndrewKushnir, alxhub, jelbourn and jessicajaniuk December 3, 2021 19:47

jessicajaniuk approved these changes Dec 3, 2021

View reviewed changes

AndrewKushnir reviewed Dec 3, 2021

View reviewed changes

packages/forms/src/directives/shared.ts Outdated Show resolved Hide resolved

packages/forms/src/model.ts Outdated Show resolved Hide resolved

packages/forms/src/model.ts Outdated Show resolved Hide resolved

pullapprove bot requested a review from AndrewKushnir December 3, 2021 22:13

dylhunn force-pushed the typed-cleanups branch from 1d7a967 to 32121b5 Compare December 7, 2021 17:52

AndrewKushnir approved these changes Dec 7, 2021

View reviewed changes

pullapprove bot requested a review from AndrewKushnir December 7, 2021 19:08

AndrewKushnir approved these changes Dec 7, 2021

View reviewed changes

dylhunn force-pushed the typed-cleanups branch from 32121b5 to 383af71 Compare December 7, 2021 21:56

dylhunn added target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Dec 8, 2021

dylhunn removed the request for review from jelbourn December 8, 2021 17:26

pullapprove bot requested a review from jelbourn December 8, 2021 17:27

dylhunn force-pushed the typed-cleanups branch from 383af71 to da86895 Compare December 8, 2021 17:30

dylhunn removed request for alxhub and jelbourn December 8, 2021 17:31

dylhunn added target: patch This PR is targeted for the next patch release and removed 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 Dec 8, 2021

dylhunn changed the title ~~feat(forms): Fix weak helper types in the Forms package~~ refactor(forms): Fix weak helper types in the Forms package Dec 8, 2021

dylhunn force-pushed the typed-cleanups branch from da86895 to 30f15a5 Compare December 8, 2021 18:32

dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: merge The PR is ready for merge by the caretaker labels Dec 8, 2021

dylhunn force-pushed the typed-cleanups branch from 30f15a5 to cbe02fb Compare December 8, 2021 18:40

dylhunn added the action: merge The PR is ready for merge by the caretaker label Dec 8, 2021

dylhunn force-pushed the typed-cleanups branch from cbe02fb to eb731cf Compare December 10, 2021 21:42

alxhub closed this in 7df9127 Dec 10, 2021

dylhunn deleted the typed-cleanups branch December 14, 2021 23:06

dylhunn mentioned this pull request Dec 15, 2021

fix(forms): _reduceValue arrow function now has correct types. #44483

Closed

14 tasks

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 14, 2022

	_reduceChildren<T>(initValue: T, fn: (acc: T, control: AbstractControl, name: string) => T) {
	_reduceChildren<T>(initValue: T, fn: (acc: T, control: AbstractControl, name: string) => T): T {

Conversation

dylhunn commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

dylhunn commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jessicajaniuk left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Dec 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

dylhunn commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alxhub commented Dec 10, 2021

Uh oh!

dylhunn commented Dec 10, 2021

Uh oh!

alxhub commented Dec 10, 2021

Uh oh!

angular-automatic-lock-bot bot commented Jan 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylhunn commented Dec 3, 2021 •

edited

Loading

dylhunn commented Dec 3, 2021 •

edited

Loading

dylhunn commented Dec 8, 2021 •

edited

Loading