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): don't mutate validators array #47830
Conversation
7bce4ac
to
88fd6ee
Compare
@@ -632,12 +632,6 @@ | |||
{ | |||
"name": "cleanUpView" | |||
}, | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure why this was dropped since the functions are still used inside setValidators
and setAsyncValidators
. My guess is because the calls were removed from the constructor.
@@ -372,15 +372,15 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T | |||
* | |||
* @internal | |||
*/ | |||
private _composedValidatorFn: ValidatorFn|null; | |||
private _composedValidatorFn!: ValidatorFn|null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could also initialize these to null
, but it seemed redundant since they're guaranteed to be defined in the constructor, TS just isn't able to figure it out statically.
@@ -401,7 +401,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T | |||
* | |||
* @internal | |||
*/ | |||
private _rawAsyncValidators: AsyncValidatorFn|AsyncValidatorFn[]|null; | |||
private _rawAsyncValidators!: AsyncValidatorFn|AsyncValidatorFn[]|null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the type be:
private _rawAsyncValidators!: AsyncValidatorFn|AsyncValidatorFn[]|null; | |
private _rawAsyncValidators: ReadonlyArray<AsyncValidatorFn> = []; |
Always an array and always readonly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array can be mutated later on by the addValidators
and removeValidators
methods. We just don't want to mutate the original array given to us by the author.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always an array and always readonly
There is a code path where _rawValidators
and _rawAsyncValidators
may become a function:
https://github.com/angular/angular/blob/main/packages/forms/src/model/abstract_model.ts#L441-L446
However, we can do further refactoring (in a followup PR if needed) to replace the implementation of the validator
and asyncValidator
setters with setValidators
and setAsyncValidators
calls, in which case the _rawValidators
and _rawAsyncValidators
would always be arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this code path is relevant for the fix here though. We only need to clone the value when it's being set to an array and the validator
getter/setter is a ValidatorFn|null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I've left a proposal on additional refactoring, but we can do that in a followup PR.
@@ -401,7 +401,7 @@ export abstract class AbstractControl<TValue = any, TRawValue extends TValue = T | |||
* | |||
* @internal | |||
*/ | |||
private _rawAsyncValidators: AsyncValidatorFn|AsyncValidatorFn[]|null; | |||
private _rawAsyncValidators!: AsyncValidatorFn|AsyncValidatorFn[]|null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always an array and always readonly
There is a code path where _rawValidators
and _rawAsyncValidators
may become a function:
https://github.com/angular/angular/blob/main/packages/forms/src/model/abstract_model.ts#L441-L446
However, we can do further refactoring (in a followup PR if needed) to replace the implementation of the validator
and asyncValidator
setters with setValidators
and setAsyncValidators
calls, in which case the _rawValidators
and _rawAsyncValidators
would always be arrays.
This PR was merged into the repository by commit 0329c13. |
This reverts commit 0329c13.
control.setAsyncValidators(originalValidators); | ||
control.addAsyncValidators(asyncValidator('two')); | ||
|
||
expect(originalValidators.length).toBe(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that better way is to check the deep equality of the array before and after the manipulations.
Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827. PR Close angular#47830
…ngular#47845) This reverts commit 0329c13. PR Close angular#47845
Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827.
88fd6ee
to
93405c2
Compare
I've fixed the issue that caused the PR to be reverted previously. I also have a passing TGP for it. |
Caretaker Note: Please ignore the |
This PR was merged into the repository by commit 779a76f. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827. PR Close angular#47830
Fixes that the `AbstractControl` was mutating the validators arrays being passed into the constructor an helper methods like `setValidators`. Fixes angular#47827. PR Close angular#47830
Fixes that the
AbstractControl
was mutating the validators arrays being passed into the constructor an helper methods likesetValidators
.Fixes #47827.