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(forms): implement setErrors #4917

Closed
wants to merge 1 commit into from
Closed

Conversation

vsavkin
Copy link
Contributor

@vsavkin vsavkin commented Oct 26, 2015

Example:

var login = new Control("someLogin");
c.setErrors({"notUnique": true});
expect(c.valid).toEqual(false);
expect(c.errors).toEqual({"notUnique": true});

c.updateValue("newLogin");
expect(c.valid).toEqual(true);

BREAKING CHANGE:

Before:

ControlGroup.errors and ControlArray.errors returned the reduced values of their children controls' errors.

After:

ControlGroup.errors and ControlArray.errors return the errors of the group and array.
And ControlGroup.controlsErrors and ControlArray.controlsErrors return the reduce values of their children controls' errors.

Notes:

  • I had to separate errors and controlsErrors. This is to provide the following guarantee: A control group with invalid children is invalid.
  • Calling setErrors on a control will update the validity of its ancestors.

@vsavkin vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 26, 2015
*/
_updateValue() {}

setErrors(errors: {[key: string]: any}): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add docs? It is easier to do now, then during a big docs push.

@mhevery mhevery added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 27, 2015
Example:

var login = new Control("someLogin");
c.setErrors({"notUnique": true});
expect(c.valid).toEqual(false);
expect(c.errors).toEqual({"notUnique": true});

c.updateValue("newLogin");
expect(c.valid).toEqual(true);

BREAKING CHANGE:

Before:

ControlGroup.errors and ControlArray.errors returned a reduced value of their children controls' errors.

After:

ControlGroup.errors and ControlArray.errors return the errors of the group and array.
And ControlGroup.controlsErrors and ControlArray.controlsErrors return the reduce value of their children controls' errors.
@vsavkin vsavkin added the action: merge The PR is ready for merge by the caretaker label Oct 27, 2015
@mary-poppins
Copy link

Merging PR #4917 on behalf of @vsavkin to branch presubmit-vsavkin-pr-4917.

@mary-poppins mary-poppins removed the action: merge The PR is ready for merge by the caretaker label Oct 27, 2015
@@ -364,6 +420,9 @@ export class ControlArray extends AbstractControl {
_updateValue(): void { this._value = this.controls.map((control) => control.value); }

/** @internal */
_calculateControlsErrors() { return Validators.array(this); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it makes sense to move group, and array from Validators? I'm worried that a user might try to pass them in the constructor, or pass them in as a combined validator. Should these functions be used as a regular validator at this point?

@TedSander
Copy link
Contributor

LGTM I like it turned out much cleaner than the original design.

@vsavkin vsavkin closed this in ed4826b Oct 27, 2015
@Krustie101
Copy link

I notice the following things in the source code:

  • The group/array validator only take into account the errors, not the controlErrors of the abstract controls in the group/array. Since the group/array validator are used to compute the controlErrors of the group/array, I expect the validy status of the group to be incorrect in the following case:
    control A in controlGroup B in controlGroup C
    Assume control A is invalid (either by internal validation or manual validation) and that we do not set the errors object of either B or C.
    The controlGroup B is invalid because the controlErrors object of B contains the error of A, but controlGroup C is valid because the error of A is not "passed through" to C.
  • It is a good idea that setting the errors property of a control overrides the errors that arose from the validator? I presume the underlying reasoning is that the manual validation should not have been performed because the control contained a validator error so it does not make a difference.
    But what if you have a numeric input with a maxValue validator and there are some other controls in the form and depending on their values, a min value constrained kicks in. I might try to write a validator for this or I might conclude that a manual validation (before submit?) is more appropriate in this case. Then any max value error (message) is lost if the min value constraint is not satisfied.
    Not overriding the errors arising from the validator in the control case (combined with clearing of manual errors if the value is updated) would also be more consistent with the group/array behaviour.
    Even simpler, a developer can clear all the errors (validator & manual) of a control by calling setError(null). This seems strange.

@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 Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants