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(forms): allow control state as reset argument for FormGroup #55860

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bougwal
Copy link
Contributor

@bougwal bougwal commented May 17, 2024

fix(forms): Allow ControlState as reset arguments for FormGroup/FormRecord

This change also decorelate the reset type argument from TValue by adding a 3rd generic parameter to AbstractControl.
This improves the typings overall.

This PR closes #55577.

@pullapprove pullapprove bot requested a review from dylhunn May 17, 2024 17:16
@bougwal bougwal force-pushed the fix/form-group-state-type-inference branch from 319e8c2 to 46312f2 Compare May 17, 2024 18:07
Copy link
Member

@JeanMeche JeanMeche left a comment

Choose a reason for hiding this comment

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

See my suggestions.

@JeanMeche JeanMeche added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms labels May 17, 2024
@ngbot ngbot bot added this to the Backlog milestone May 17, 2024
@bougwal bougwal force-pushed the fix/form-group-state-type-inference branch 3 times, most recently from 23aa90b to 590481c Compare May 17, 2024 19:09
@bougwal bougwal force-pushed the fix/form-group-state-type-inference branch from 590481c to 8fdb80f Compare May 17, 2024 19:28
@bougwal bougwal requested a review from JeanMeche May 17, 2024 19:30
@JeanMeche
Copy link
Member

Thank for the changes Walid !
We'll do some checks against the Google codebase and we'll let you know if we can go forward with the change.

@dylhunn
Copy link
Contributor

dylhunn commented May 17, 2024

We're investigating an issue illustrated by the following snippet:

function valueChangesWrapper<T>(control: AbstractControl<T>) {
  return control.valueChanges;
}
const fg = new FormGroup({foo: new FormControl('')});
fg.valueChanges;
const a = valueChangesWrapper(fg);

Notice that the inferred type of valueChangesWrapper inappropriately includes FormControlState. We think this is due to type unification of FormGroup.reset with AbstractControl.reset.

This is super tricky to debug/understand.

@JeanMeche JeanMeche force-pushed the fix/form-group-state-type-inference branch from 8fdb80f to eeab1c7 Compare May 17, 2024 23:30
@JeanMeche
Copy link
Member

I've allowed myself to push the changes to fix the issues that were brought up by testing the fix against Google's code base.

Basically what happens is that changing the reset() signature had implicationg on the TValue generic of AbstractControl.

@JeanMeche JeanMeche force-pushed the fix/form-group-state-type-inference branch from eeab1c7 to 144f0cf Compare May 17, 2024 23:43
@JeanMeche JeanMeche changed the title fix(forms): Type inference issue on ɵFormGroupValue in formGroup.reset due to usage of FormControlState<T> fix(forms): allow control state as reset argument for FormGroup May 18, 2024
@JeanMeche JeanMeche force-pushed the fix/form-group-state-type-inference branch from 144f0cf to 6bfaa7c Compare May 18, 2024 00:36
…rmRecord`

This change also decorelate the `reset` type argument from `TValue` by adding a 3rd generic parameter to `AbstractControl`.
This improves the typings overall.
@JeanMeche JeanMeche force-pushed the fix/form-group-state-type-inference branch from 6bfaa7c to 51f080c Compare May 18, 2024 00:57
@JeanMeche JeanMeche added the target: minor This PR is targeted for the next minor release label May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: forms target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FormGroup.reset() input typings are incorrect
3 participants