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

Promise-based validations & Multi-stage validation API for ngModel #8267

Closed
wants to merge 2 commits into from

Conversation

matsko
Copy link
Contributor

@matsko matsko commented Jul 19, 2014

No description provided.

@mary-poppins
Copy link

Baaaam!

@matsko matsko added cla: yes and removed cla: no labels Jul 19, 2014
@Narretz
Copy link
Contributor

Narretz commented Jul 19, 2014

Hey @matsko I will check this out tomorrow, but it would be nice if you could explain the PR a little more, what it does and how it integrates with current validation etc.

@shahata
Copy link
Contributor

shahata commented Jul 19, 2014

@matsko I believe this will not work properly in case an async validator is run twice due to input change and gets resolved out of order (old value validation is resolved after new value validation is resolved). I'll try to send a test that fails later on today.

@shahata
Copy link
Contributor

shahata commented Jul 19, 2014

In general, I think we should think about reseting the pendingCount when the value changes and completely ignore resolutions of promises that we got so far.

@shahata
Copy link
Contributor

shahata commented Jul 19, 2014

The failing test...

    iit('should ignore old async validator promises that get resolved', inject(function($q) {
      var defer, oldDefer, newDefer;
      ctrl.$addAsyncValidator('async', function(value) {
        defer = $q.defer();
        return defer.promise;
      });

      scope.$apply('value = ""');
      oldDefer = defer;
      scope.$apply('value = "123"');
      newDefer = defer;

      newDefer.reject();
      scope.$digest();
      oldDefer.resolve();
      scope.$digest();

      expect(ctrl.$valid).toBe(false);
      expect(ctrl.$invalid).toBe(true);
      expect(ctrl.$pending).toBeUndefined();
    }));

@IgorMinar IgorMinar modified the milestones: 1.3.0-beta.17, 1.3.0 Jul 21, 2014
@Narretz
Copy link
Contributor

Narretz commented Jul 22, 2014

I like the PR a lot, but I think we should first solve the open issues with the current validation before we add another feature, especially first convert all the validations in $parsers to $validators to see if the new architecture is sufficient.

@matsko matsko changed the title feat(ngModel): provide support for promise-based validators Promise-based validations & Multi-stage validation API for ngModel Jul 23, 2014
@Narretz
Copy link
Contributor

Narretz commented Jul 24, 2014

Can you explain what the difference between native and sync is? The validators in this PR are now changed to native, but when I hear native, I think of the Constaint Validation API, not our own validators (which mimic the CV, but use own regex etc.)
The only place where we use CV is the number validator which is untouched by this. Number and Date still live in parsers / formatters, aren't they supposed to be moved to $validators?
Sorry if I sound too critical, but I'd really like more background on the design decisions here.

@matsko
Copy link
Contributor Author

matsko commented Jul 25, 2014

@shahata I think the failing test is correct (the failure is expected), because it should always ignore the previous async' response and only consider the latest one. This wouldn't be an issue normally if we could cancel promises.

@matsko
Copy link
Contributor Author

matsko commented Jul 25, 2014

@Narretz this solution is actually to provide a better foundation to fix some of the native-based validators. Native validators are stuff like required, maxlength, minlength and so on. Async are validators that return promises. The number validator will be changed in a later commit once I can ensure it doesn't break anything. The date stuff still needs to be converted over in a smart way.

@matsko
Copy link
Contributor Author

matsko commented Jul 25, 2014

@Narretz do you still need a more detailed explanation of it?

@shahata
Copy link
Contributor

shahata commented Jul 25, 2014

@matsko I think you misunderstood my comment. The problem is that with the test I sent you, the controller remains in pending validation state even after both promises are resolved. The root cause of this is that you consider the pending validation complete only if currentValue === ctrl.$viewValue AND pendingCount === 0. Since this condition does not happen for any of the resolutions in the test, we are left in pending validation state.

I proposed solving this by resetting pendingCount back to zero when the the view value changes. Obviously this will require some more adjustments and there are also other ways to solve this.

I adjusted the unit test above to the latest semantics in this PR so you can reproduce easily.

@matsko
Copy link
Contributor Author

matsko commented Jul 27, 2014

@shahata I see. The code is now up to date and your spec has been put in.

@petebacondarwin
Copy link
Contributor

@matsko - Has there been a design discussion about this with the core team? How does this deal with the numerous issues that have been discussed recently around things like

@caitp
Copy link
Contributor

caitp commented Jul 27, 2014

we had a meeting specifically about the features implemented by this PR, I haven't reviewed this one yet though. I think we're all on the same page about what is needed though, I hope

* method for resolve and a status level of `400` in order to reject the validation.
*
* ```js
* ngModel.$asyncVaidators.usernameTaken = function(modelValue, viewValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in "asyncVaidators"

Copy link
Contributor

Choose a reason for hiding this comment

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

the name of the validator should likely be something like "uniqueUsername"- you don't validate the the username is taken :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

var value = ctrl.$viewValue || '';
if (ctrl.$pending && ctrl.$pending[validationErrorKey] && currentValue === value) {
delete ctrl.$pending[validationErrorKey];
pendingCount = Object.keys(ctrl.$pending).length;
Copy link
Contributor

Choose a reason for hiding this comment

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

If all you actually care about is if ctrl.$pending is empty or not, it seems redundant to have a pendingCount

@tbosch
Copy link
Contributor

tbosch commented Aug 4, 2014

Hi @matsko,
I have the following suggestions/questions:

  1. Please change the distribution of the code changes to commits: Put everything re async processing into the second commit as well, or maybe create a third commit in between. This is important, as with that we should already be able to solve all the open issues around validation and it's less changes to understand
  2. Refactor min/max validation for date and number input into validators as right now they are registered as parsers. Besides the native validation, those were the last missing validators that are registered as parsers AFAIK.

@matsko @caitp Why do we need to extra phase for native validators again? The case that we have an input[type=number] with bad input that we never see when reading input.value should be read out and reported as error by the corresponding parser (via input.validity.typeMismatch or .badInput ), right?
Given that, all the other validations should actually be possible directly in JS, without reading out input.validity any more (e.g. required, min-length, ...). This is good as we will always need the JS implementation for polyfilling browsers that don't support the constraint validation API, and we have a consistent behavior.

@caitp
Copy link
Contributor

caitp commented Aug 4, 2014

@tbosch because when we have a native ValidityState, we need to check those flags in a specific order (and abort checking them in the case of certain failures) --- in order to avoid over-reporting errors --- and we really need to always run those first, because the 'badInput' error is going to cause user validators to behave unexpectedly.

@tbosch
Copy link
Contributor

tbosch commented Aug 4, 2014

Yes, we should check badInput as part of the parser first, and the not run other validators at all, agree.

But why should we run required, min-length, min, ... before all the other validators and not run other validators if one of those fail? How do we determine this list of validators to run before the user validators? Is it the angular built in ones? Or the ones that are supported by native validation (in browsers that have this feature)?

@tbosch
Copy link
Contributor

tbosch commented Aug 4, 2014

@matsko I went though all of the open issues that were tagged with component: forms and added a list to #6928 (#6928 (comment)).

To validate your design (e.g. how native validation is handled), it would probably be best to solve them in this PR as well, as each one of those seems really simple. Otherwise we would land an abstract API and would detect later on that we need more changes to it...

@matsko
Copy link
Contributor Author

matsko commented Aug 4, 2014

@tbosch thank you for going through all the code. Yes I agree that this PR should contain the additional commits that fix the existing list of validation bugs.

Regarding the "native" validators. The point is to detect the errors early on in the badInput stage before moving onwards to the custom validators present in $validators. Right now we have just about all the standard HTML5 validators within the native validators collection and I think this is correct. However, I'm not fully aware of the issue regarding the over-abundance of reported errors and how having a native validation stage can help. My guess is that by having an earlier stage of validators we can break the validation cycle early on. @caitp am I missing anything here?

Regarding the date validators. Yes I will try and get them into the $validators pipeline today, but the issue is that the validators are run after the parsers/formatters. This means that there still needs to be some level of value checking within the date parser and, if the value is set to null within it, then that null value will be processed as invalid within any existing native validators before the date validator is executed.

@tbosch
Copy link
Contributor

tbosch commented Aug 5, 2014

Re date validators:
I mean the min/max validators, not the parser that converts strings to
dates. If the parser fails, we should probably never run any validators at
all, right?

Re native validators:
I still don't get why they should be handled in a special phase. Yes,
reading badInput is important for eg a number field as that means that the
entered number is not a number. However, this check is part of the parser
if we have no native support, right? So for native validators the parser
would read input.valueAsNumber and check validity.badInput. And given that
validators are not executed when a parser failed, we don't need an extra
stage for this.

For the other native validators like required, pattern, ...: I still don't
see a reason for having them in a special phase before everyone else. What
is different to a custom pattern validator from a user? Especially when the
browser does not have native validation? Btw, as I mentioned before, we
probably should always use our JS validators for required, pattern, ... to
be consistent across all browsers...

Sent from my iPhone

On Aug 4, 2014, at 6:30 PM, "Matias Niemelä" [email protected]
wrote:

@tbosch https://github.com/tbosch thank you for going through all the
code. Yes I agree that this PR should contain the additional commits that
fix the existing list of validation bugs.

Regarding the "native" validators. The point is to detect the errors early
on in the badInput stage before moving onwards to the custom validators
present in $validators. Right now we have just about all the standard HTML5
validators within the native validators collection and I think this is
correct. However, I'm not fully aware of the issue regarding the
over-abundance of reported errors and how having a native validation stage
can help. My guess is that by having an earlier stage of validators we can
break the validation cycle early on. @caitp https://github.com/caitp am I
missing anything here?

Regarding the date validators. Yes I will try and get them into the
$validators pipeline today, but the issue is that the validators are run
after the parsers/formatters. This means that there still needs to be some
level of value checking within the date parser and, if the value is set to
null within it, then that null value will be processed as invalid within
any existing native validators before the date validator is executed.


Reply to this email directly or view it on GitHub
#8267 (comment).

matsko added 2 commits August 26, 2014 17:55
With this commit, ngModel will now handle parsing first and then validation
afterwards once the parsing is successful. If any parser along the way returns
`undefined` then ngModel will break the chain of parsing and register a
a parser error represented by the type of input that is being collected
(e.g. number, date, datetime, url, etc...). If a parser fails for a standard
text input field then an error of `parse` will be placed on `model.$error`.

BREAKING CHANGE

Any parser code from before that returned an `undefined` value
(or nothing at all) will now cause a parser failure. When this occurs
none of the validators present in `$validators` will run until the parser
error is gone.
…lidations

This commit introduces a 2nd validation queue called `$asyncValidators`. Each time a value
is processed by the validation pipeline, if all synchronous `$validators` succeed, the value
is then passed through the `$asyncValidators` validation queue. These validators should return
a promise. Rejection of a validation promise indicates a failed validation.
@matsko matsko force-pushed the async_validations branch from 5f731a5 to 04ab51c Compare August 26, 2014 22:01
@matsko
Copy link
Contributor Author

matsko commented Aug 26, 2014

MERGED

db044c4
2ae4f40

@matsko matsko closed this Aug 26, 2014
@matsko matsko deleted the async_validations branch August 26, 2014 23:04
@btford btford mentioned this pull request Aug 28, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants