Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ngModelController.$validators pipeline and refactoring #7377

Closed
wants to merge 6 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented May 7, 2014

Fixes #6750
Fixes #6304
Fixes #5164

@matsko matsko added this to the 1.3.0 milestone May 7, 2014
@matsko matsko changed the title ngModelController.$validators pipeline ngModelController.$validators pipeline and refactoring May 7, 2014
ctrl.$$deferValidation = false;
var value = ctrl.$modelValue;
forEach(ctrl.$validators, function(fn, name) {
ctrl.$setValidity(name, fn(value));
Copy link

Choose a reason for hiding this comment

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

It seems to me that this is wrong; it seems like this says that if you have two validators, and the first one returns false, but the second one returns true, that the control will be set to valid. Am I missing something here?

Copy link
Contributor

Choose a reason for hiding this comment

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

only if they're all valid will the control be valid, afaik

Copy link
Contributor

Choose a reason for hiding this comment

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

each validator has a different name, so there is no problem here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an AND operation when $valid is determined.

Copy link

Choose a reason for hiding this comment

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

OK, I looked over the code for $setValidity and discovered it was keeping track of how many times the control was valid vs. invalid. My mistake.

@Narretz
Copy link
Contributor

Narretz commented May 7, 2014

What is the relationship between $validators and setting the model & view values? Are they still only set when it is valid?

@matsko
Copy link
Contributor Author

matsko commented May 7, 2014

@Narretz validations will happen whenever the model value changes. It will always be the parsed value. The view value is ignored.

@Narretz
Copy link
Contributor

Narretz commented May 14, 2014

A minor thing: since validations are not short-circuiting anymore, pipeline isn't a very good description. It may confuse people who are familiar with parsers / validators pipeline.

@matsko
Copy link
Contributor Author

matsko commented May 14, 2014

@Narretz yes that is true. Maybe queue would be better.

@IgorMinar
Copy link
Contributor

to summarize, I have these questions:

  • async validation
    • why do we need it?
    • why is it implemented via postDigest?
  • pristine
    • why did the behavior change?
  • minlenght/maxlength
    • shouldn't the max/min value be dynamically updated?
  • validated value
    • previously it was possible to validate the value either before parsing (viewValue) or after parsing (modelValue). now it's only modelValue and the validator has no access to the viewValue. Is this not a problem?
  • modelValue update
    • previously the modelValue would not be updated if the value is invalid, this is not the case any more, which means that apps now need to guard themselves against invalid values. This is a major breaking change. Why?

@Narretz
Copy link
Contributor

Narretz commented May 21, 2014

Regarding model updates when invalid: The intention is that sometimes you want the value in the model even though it is invalid (same goes for model -> view and formatters). But this could be made configurable.

Updating the model also means that you don't see validation errors before the model is updated, which means consistency with the new model update options. A good example is entering an email address, and getting the 'invalid' error message while you are typing, which wouldn't happen with a debounce.

However that this is unflexible, as sometimes you want immediate feedback.. @shahata has a commit here that defines validateOn options similar to the model updates: #7414, so validation errors can be displayed before model updates are made. Maybe this can be integrated - which would mean $validators use the (parsed) viewValue again

@btford btford modified the milestones: 1.3.0-beta.11, 1.3.0-beta.10 May 23, 2014
BREAKING CHANGE:

If an expression is used on ng-pattern (such as `ng-pattern="exp"`) or on the
pattern attribute (something like on `pattern="{{ exp }}"`) and the expression
itself evaluates to a string then the validator will not parse the string as a
literal regular expression object (a value like `/abc/i`).  Instead, the entire
string will be created as the regular expression to test against. This means
that any expression flags will not be placed on the RegExp object. To get around
this limitation, use a regular expression object as the value for the expression.

    //before
    $scope.exp = '/abc/i';

    //after
    $scope.exp = /abc/i;
@matsko
Copy link
Contributor Author

matsko commented Jun 13, 2014

MERGED

@matsko
Copy link
Contributor Author

matsko commented Jun 13, 2014

Landed as 1be9bb9

@matsko matsko closed this Jun 13, 2014
@matsko matsko deleted the validators branch June 13, 2014 01:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.