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

Commit

Permalink
feat(NgModel): introduce the $validators pipeline
Browse files Browse the repository at this point in the history
  • Loading branch information
matsko committed Jun 13, 2014
1 parent 545d22b commit a8c7cb8
Show file tree
Hide file tree
Showing 2 changed files with 200 additions and 15 deletions.
66 changes: 51 additions & 15 deletions src/ng/directive/input.js
Original file line number Diff line number Diff line change
Expand Up @@ -1440,6 +1440,12 @@ var VALID_CLASS = 'ng-valid',
* ngModel.$formatters.push(formatter);
* ```
*
* @property {Object.<string, function>} $validators A collection of validators that are applied
* whenever the model value changes. The key value within the object refers to the name of the
* validator while the function refers to the validation operation. The validation operation is
* provided with the model value as an argument and must return a true or false value depending
* on the response of that validation.
*
* @property {Array.<Function>} $viewChangeListeners Array of functions to execute whenever the
* view value has changed. It is called with no arguments, and its return value is ignored.
* This can be used in place of additional $watches against the model value.
Expand Down Expand Up @@ -1558,6 +1564,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
function($scope, $exceptionHandler, $attr, $element, $parse, $animate, $timeout) {
this.$viewValue = Number.NaN;
this.$modelValue = Number.NaN;
this.$validators = {};
this.$parsers = [];
this.$formatters = [];
this.$viewChangeListeners = [];
Expand Down Expand Up @@ -1637,7 +1644,8 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* Change the validity state, and notifies the form when the control changes validity. (i.e. it
* does not notify form if given validator is already marked as invalid).
*
* This method should be called by validators - i.e. the parser or formatter functions.
* This method can be called within $parsers/$formatters. However, if possible, please use the
* `ngModel.$validators` pipeline which is designed to handle validations with true/false values.
*
* @param {string} validationErrorKey Name of the validator. the `validationErrorKey` will assign
* to `$error[validationErrorKey]=isValid` so that it is available for data-binding.
Expand Down Expand Up @@ -1786,6 +1794,23 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
ctrl.$render();
};

/**
* @ngdoc method
* @name ngModel.NgModelController#$validate
*
* @description
* Runs each of the registered validations set on the $validators object.
*/
this.$validate = function() {
this.$$runValidators(ctrl.$modelValue, ctrl.$viewValue);
};

this.$$runValidators = function(modelValue, viewValue) {
forEach(ctrl.$validators, function(fn, name) {
ctrl.$setValidity(name, fn(modelValue, viewValue));
});
};

/**
* @ngdoc method
* @name ngModel.NgModelController#$commitViewValue
Expand All @@ -1798,12 +1823,12 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
* usually handles calling this in response to input events.
*/
this.$commitViewValue = function() {
var value = ctrl.$viewValue;
var viewValue = ctrl.$viewValue;
$timeout.cancel(pendingDebounce);
if (ctrl.$$lastCommittedViewValue === value) {
if (ctrl.$$lastCommittedViewValue === viewValue) {
return;
}
ctrl.$$lastCommittedViewValue = value;
ctrl.$$lastCommittedViewValue = viewValue;

// change to dirty
if (ctrl.$pristine) {
Expand All @@ -1814,13 +1839,19 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
parentForm.$setDirty();
}

var modelValue = viewValue;
forEach(ctrl.$parsers, function(fn) {
value = fn(value);
modelValue = fn(modelValue);
});

if (ctrl.$modelValue !== value) {
ctrl.$modelValue = value;
ngModelSet($scope, value);
if (ctrl.$modelValue !== modelValue &&
(isUndefined(ctrl.$$invalidModelValue) || ctrl.$$invalidModelValue != modelValue)) {

ctrl.$$runValidators(modelValue, viewValue);
ctrl.$modelValue = ctrl.$valid ? modelValue : undefined;
ctrl.$$invalidModelValue = ctrl.$valid ? undefined : modelValue;

ngModelSet($scope, ctrl.$modelValue);
forEach(ctrl.$viewChangeListeners, function(listener) {
try {
listener();
Expand Down Expand Up @@ -1894,26 +1925,31 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$

// model -> value
$scope.$watch(function ngModelWatch() {
var value = ngModelGet($scope);
var modelValue = ngModelGet($scope);

// if scope model value and ngModel value are out of sync
if (ctrl.$modelValue !== value) {
if (ctrl.$modelValue !== modelValue &&
(isUndefined(ctrl.$$invalidModelValue) || ctrl.$$invalidModelValue != modelValue)) {

var formatters = ctrl.$formatters,
idx = formatters.length;

ctrl.$modelValue = value;
var viewValue = modelValue;
while(idx--) {
value = formatters[idx](value);
viewValue = formatters[idx](viewValue);
}

if (ctrl.$viewValue !== value) {
ctrl.$viewValue = ctrl.$$lastCommittedViewValue = value;
ctrl.$$runValidators(modelValue, viewValue);
ctrl.$modelValue = ctrl.$valid ? modelValue : undefined;
ctrl.$$invalidModelValue = ctrl.$valid ? undefined : modelValue;

if (ctrl.$viewValue !== viewValue) {
ctrl.$viewValue = ctrl.$$lastCommittedViewValue = viewValue;
ctrl.$render();
}
}

return value;
return modelValue;
});
}];

Expand Down
149 changes: 149 additions & 0 deletions test/ng/directive/inputSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,155 @@ describe('NgModelController', function() {
expect(ctrl.$render).toHaveBeenCalledOnce();
});
});

describe('$validators', function() {

it('should perform validations when $validate() is called', function() {
ctrl.$validators.uppercase = function(value) {
return (/^[A-Z]+$/).test(value);
};

ctrl.$modelValue = 'test';
ctrl.$validate();

expect(ctrl.$valid).toBe(false);

ctrl.$modelValue = 'TEST';
ctrl.$validate();

expect(ctrl.$valid).toBe(true);
});

it('should perform validations when $validate() is called', function() {

This comment has been minimized.

Copy link
@lrlopez

lrlopez Jun 13, 2014

Contributor

Dupe? Both the description and the actual test are repeated just above

This comment has been minimized.

Copy link
@matsko

matsko Jun 13, 2014

Author Contributor

Ah. Right. Must've been a faulty checkout/rebase. I'll fix this with a chore commit.

ctrl.$validators.uppercase = function(value) {
return (/^[A-Z]+$/).test(value);
};

ctrl.$modelValue = 'test';
ctrl.$validate();

expect(ctrl.$valid).toBe(false);

ctrl.$modelValue = 'TEST';
ctrl.$validate();

expect(ctrl.$valid).toBe(true);
});

it('should always perform validations using the parsed model value', function() {
var captures;
ctrl.$validators.raw = function() {
captures = arguments;
return captures[0];
};

ctrl.$parsers.push(function(value) {
return value.toUpperCase();
});

ctrl.$setViewValue('my-value');

expect(captures).toEqual(['MY-VALUE', 'my-value']);
});

it('should always perform validations using the formatted view value', function() {
var captures;
ctrl.$validators.raw = function() {
captures = arguments;
return captures[0];
};

ctrl.$formatters.push(function(value) {
return value + '...';
});

scope.$apply(function() {
scope.value = 'matias';
});

expect(captures).toEqual(['matias', 'matias...']);
});

it('should only perform validations if the view value is different', function() {
var count = 0;
ctrl.$validators.countMe = function() {
count++;
};

ctrl.$setViewValue('my-value');
expect(count).toBe(1);

ctrl.$setViewValue('my-value');
expect(count).toBe(1);

ctrl.$setViewValue('your-value');
expect(count).toBe(2);
});

it('should perform validations twice each time the model value changes within a digest', function() {
var count = 0;
ctrl.$validators.number = function(value) {
count++;
return (/^\d+$/).test(value);
};

function val(v) {
scope.$apply(function() {
scope.value = v;
});
}

val('');
expect(count).toBe(1);

val(1);
expect(count).toBe(2);

val(1);
expect(count).toBe(2);

val('');
expect(count).toBe(3);
});

it('should only validate to true if all validations are true', function() {
var curry = function(v) {
return function() {
return v;
};
};

ctrl.$validators.a = curry(true);
ctrl.$validators.b = curry(true);
ctrl.$validators.c = curry(false);

ctrl.$validate();
expect(ctrl.$valid).toBe(false);

ctrl.$validators.c = curry(true);

ctrl.$validate();
expect(ctrl.$valid).toBe(true);
});

it('should register invalid validations on the $error object', function() {
var curry = function(v) {
return function() {
return v;
};
};

ctrl.$validators.unique = curry(false);
ctrl.$validators.tooLong = curry(false);
ctrl.$validators.notNumeric = curry(true);

ctrl.$validate();

expect(ctrl.$error.unique).toBe(true);
expect(ctrl.$error.tooLong).toBe(true);
expect(ctrl.$error.notNumeric).not.toBe(true);
});
});
});

describe('ngModel', function() {
Expand Down

11 comments on commit a8c7cb8

@Narretz
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen that some validators - pattern, minLength, maxLength, required have been converted to use the $validators, but all input types - email, url etc. have not. What is the intention behind this? Wouldn't it make sense to convert all of them? Because otherwise the behavior of inputs will be different depending on type + validator.

For example, an invalid email modelValue set on the scope will not display in an input, but a text with invalid minlength will (which is a good thing). Worse, an invalid email that is too short for ngMinlength will not have the minlength error set.

Another thing (bug): when I have two validators in the collection and have a model value on the scope, the ngModel will not have the correct validation output: if both are invalid, no errors are set; if one is valid, the other still doesn't have the error set. Also, both $$invalidModelValue and the modelValue on the scope are set, but the $modelValue on the ctrl is not set. When I change the input, the validation becomes correct. It also works correctly with only one validator in the collection.
See http://plnkr.co/edit/gffv2ro1pQBqb7ESnIBF?p=preview

If this is still work in progress, please disregard my notes.

@caitp
Copy link
Contributor

@caitp caitp commented on a8c7cb8 Jun 13, 2014

Choose a reason for hiding this comment

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

I believe what you're talking about is, you think people should be able to replace the email/url/number validators with their own versions, using some other directive to modify the pipeline?

That doesn't seem totally unreasonable, I guess

@Narretz
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I am worried that the current state is quite confusing. You have validators living in parsers / formatters and validators in $validators. Regarding replacing validators, that was possible (although inconvenient) before, but named validators are actually a better option, e.g. you could replace the default email validator with your own. But for that the input type validators must be converted to be in $validators first.

@caitp
Copy link
Contributor

@caitp caitp commented on a8c7cb8 Jun 13, 2014

Choose a reason for hiding this comment

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

It's never really been a supported use-case. You can tell it's not a supported use-case, because there's no simple API to make it easy. Replacing validators, even in the current version, is kind of frail because you don't really know what you're displacing, or how it will affect anything.

It might be a good thing to support, but there's no precedent for it, even this stuff isn't really an exported API. Ideally we should have something like $addValidator / $removeValidator / $replaceValidator

@Narretz
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, a real api is a good idea. Before that however, the validation handling should be consistent.

@caitp
Copy link
Contributor

@caitp caitp commented on a8c7cb8 Jun 13, 2014

Choose a reason for hiding this comment

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

Would have to be consistent for an API to work, but it's not really important currently since there's no precedent for support for replacing validators, it's never been a supported thing.

I expect more will be done on this shortly

@matsko
Copy link
Contributor Author

@matsko matsko commented on a8c7cb8 Jun 13, 2014

Choose a reason for hiding this comment

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

@Narretz yes this is still a work in progress. The email, number and url input types and date validators still need to get ported over. Regarding your plunkr. Yes there definitely is a bug. Looks like the error properties are not updating properly. I'm looking into that right now.

@caitp + @Narretz Can you not just make your own directive with a higher priority to ngModel that attaches a ngPattern attribute to the input element? I don't think allowing to replace / override validators is a good idea.

@matsko
Copy link
Contributor Author

@matsko matsko commented on a8c7cb8 Jun 13, 2014

Choose a reason for hiding this comment

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

@Narretz I fixed up some of the code in your plunkr. It looks like it's working correctly. Can you explain if I'm missing something: http://plnkr.co/edit/P6wWTNumguK82b4HZ7hM?p=preview

@Narretz
Copy link
Contributor

Choose a reason for hiding this comment

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

@matsko I think you added the email-validator? It's not really necessary for the issue. The problem is that initially, all 2 (3) validators show success, although $scope.myEmail is too short, doesn't start with s (and is not an email). If you type in the input, the validation becomes correct. Also if you remove all but one validator, the validation works correctly initially:

I updated the plunkr with another input / ngModel:
http://plnkr.co/edit/k7uHoect0HBpg2F0KUOf?p=preview

Regarding overriding validators: if you have input type=email it's very hard to remove this validator. You can obviously use your own pattern, but email will still be present. Manipulating the $parsers / $formatters queue is brittle, as soon as more are added. Since the new validators have named keys, it should be quite easy to overwrite them in another directive. I don't really see a downside there. You can do / break lots of different stuff with delegates / directive overwrites already.

@matsko
Copy link
Contributor Author

@matsko matsko commented on a8c7cb8 Jun 16, 2014

Choose a reason for hiding this comment

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

@Narretz yes you're right about the broken validations.

  1. There should be an initial validation when the form first compiles. I will make a patch today to fix this.

  2. This is a great idea. If we place the email validator on model.$validators.email then this would allow free-reign to override it. You would still need to make your own directive though that has a lower priority than the default one for inputEmail (I think it's 1000).

@Narretz
Copy link
Contributor

Choose a reason for hiding this comment

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

@matsko Great!

  1. Note that it works initially if there's only one validator. So it kind of works already, but there's a bug somewhere ...

  2. An additonal option is to make certain regexes configurable. Most people take offense at the email regex (I agree), so putting in a provider might be a good way to fend of these issues about the email regex that come once per month. Regardless, putting all validations on $validators is a very good idea!

Please sign in to comment.