refactor(ngModelController,formController): #8941

tbosch · 2014-09-05T00:51:15Z

refactor(ngModelController,formController): centralize and simplify l…
…ogic

The previous logic for async validation in
ngModelController and formController was not maintainable:

control logic is in multiple parts, e.g. ctrl.$setValidity
waits for end of promises and continuous the control flow
for async validation
logic for updating the flags ctrl.$error, ctrl.$pending, ctrl.$valid
is super complicated, especially in formController

This refactoring makes the following changes:

simplify async validation: centralize control logic
into one method in ngModelController:
- remove counters invalidCount and pendingCount
- use a flag currentValidationRunId to separate
  async validator runs from each other
- use $q.all to determine when all async validators are done
centralize way how ctrl.$modelValue and ctrl.$invalidModelValue
is updated
simplify ngModelController/formCtrl.$setValidity and merge
$$setPending/$$clearControlValidity/$$clearValidity/$$clearPending
into one method, that is used by ngModelController AND
formController
- remove diff calculation, always calculate the correct state anew,
  only cache the css classes that have been set to not
  trigger too many css animations.
- remove fields from ctrl.$error that are valid and add ctrl.$success:
  allows to correctly separate states for valid, invalid, skipped and pending,
  especially transitively across parent forms.
fix bug in ngModelController:
- only read out input.validity.badInput, but not
  input.validity.typeMismatch,
  to determine parser error: We still want our email
  validator to run event when the model is validated.
fix bugs in tests that were found as the logic is now consistent between
ngModelController and formController

Narretz · 2014-09-05T08:05:27Z

src/ng/directive/input.js

-        ctrl.$valid = true;
-        ctrl.$invalid = false;
+  // TODO: Add new values 'pending' and 'skipped' to docs
+  // TODO: make this an internal method, i.e. rename to $$setValidity


I'd be careful with making $setValidity private. There are many custom validation implementations that rely on this. With $validators, many have become obsolete, but there is at least one (that I can can think of now) that's not so easily emulated: when you $http the whole form to the server for validation, and then use $setValidity to apply the results (legacy apps). I don't think you can do that with $validators.

Narretz · 2014-09-05T08:16:35Z

Hey @tbosch I think it's a great idea to clean up the code. IIRC @caitp, you were also a big fan of refactoring ngmodel et. al., so maybe you could have a look too?

IgorMinar · 2014-09-05T19:16:31Z

src/ng/directive/form.js

@@ -75,12 +74,38 @@ function FormController(element, attrs, $scope, $animate) {
  toggleValidCss(true);

  // convenience method for easy toggling of classes
+  function toggleClass(className, setUnset) {


what's the meaning of setUnset variable? it's a weird name

I actually don't follow the logic of the conditions in the following code. if it's supposed to really toggle the classes then I'd expect the conditions to be different.

Renamed the method to cachedToggleClass. It sets/removes the css class based on the given argument. The name is the same as the one in jQuery, although this method does not support to be called with no argument, see http://api.jquery.com/toggleclass/#toggleClass-className-switch
I don't know a better name for this :-(

Renamed to parameter to switchValue, just like in the jQuery method...

ah. ok. now it makes sense. thanks

IgorMinar · 2014-09-05T20:56:35Z

overall I like this change. it needs a bit more cleanup though.

tbosch · 2014-09-05T22:50:41Z

Ok, just did. All tests are passing again, and I updated the commit message.
From my standpoint this is merge able!
Could you do a final pass and LGTM this?

Narretz · 2014-09-05T22:52:07Z

Wait a minute ... I assume this also addresses #8861 and #8892?

tbosch · 2014-09-05T22:58:06Z

Ok, also rebased on top of master...

tbosch · 2014-09-05T23:00:47Z

@Narretz it could, although I have not explicitly checked. When I work on those other PRs, I will add their tests and see what happens.

shahata · 2014-09-05T23:03:55Z

As far as I can see this PR fixes the issue addressed by those PR's

IgorMinar · 2014-09-08T01:05:19Z

👍

…ogic The previous logic for async validation in `ngModelController` and `formController` was not maintainable: - control logic is in multiple parts, e.g. `ctrl.$setValidity` waits for end of promises and continuous the control flow for async validation - logic for updating the flags `ctrl.$error`, `ctrl.$pending`, `ctrl.$valid` is super complicated, especially in `formController` This refactoring makes the following changes: - simplify async validation: centralize control logic into one method in `ngModelController`: * remove counters `invalidCount` and `pendingCount` * use a flag `currentValidationRunId` to separate async validator runs from each other * use `$q.all` to determine when all async validators are done - centralize way how `ctrl.$modelValue` and `ctrl.$invalidModelValue` is updated - simplify `ngModelController/formCtrl.$setValidity` and merge `$$setPending/$$clearControlValidity/$$clearValidity/$$clearPending` into one method, that is used by `ngModelController` AND `formController` * remove diff calculation, always calculate the correct state anew, only cache the css classes that have been set to not trigger too many css animations. * remove fields from `ctrl.$error` that are valid and add private `ctrl.$$success`: allows to correctly separate states for valid, invalid, skipped and pending, especially transitively across parent forms. - fix bug in `ngModelController`: * only read out `input.validity.badInput`, but not `input.validity.typeMismatch`, to determine parser error: We still want our `email` validator to run event when the model is validated. - fix bugs in tests that were found as the logic is now consistent between `ngModelController` and `formController` BREAKING CHANGE: - `ctrl.$error` does no more contain entries for validators that were successful. - `ctrl.$setValidity` now differentiates between `true`, `false`, `undefined` and `null`, instead of previously only truthy vs falsy.

…ogic The previous logic for async validation in `ngModelController` and `formController` was not maintainable: - control logic is in multiple parts, e.g. `ctrl.$setValidity` waits for end of promises and continuous the control flow for async validation - logic for updating the flags `ctrl.$error`, `ctrl.$pending`, `ctrl.$valid` is super complicated, especially in `formController` This refactoring makes the following changes: - simplify async validation: centralize control logic into one method in `ngModelController`: * remove counters `invalidCount` and `pendingCount` * use a flag `currentValidationRunId` to separate async validator runs from each other * use `$q.all` to determine when all async validators are done - centralize way how `ctrl.$modelValue` and `ctrl.$invalidModelValue` is updated - simplify `ngModelController/formCtrl.$setValidity` and merge `$$setPending/$$clearControlValidity/$$clearValidity/$$clearPending` into one method, that is used by `ngModelController` AND `formController` * remove diff calculation, always calculate the correct state anew, only cache the css classes that have been set to not trigger too many css animations. * remove fields from `ctrl.$error` that are valid and add private `ctrl.$$success`: allows to correctly separate states for valid, invalid, skipped and pending, especially transitively across parent forms. - fix bug in `ngModelController`: * only read out `input.validity.badInput`, but not `input.validity.typeMismatch`, to determine parser error: We still want our `email` validator to run event when the model is validated. - fix bugs in tests that were found as the logic is now consistent between `ngModelController` and `formController` BREAKING CHANGE: - `ctrl.$error` does no more contain entries for validators that were successful. - `ctrl.$setValidity` now differentiates between `true`, `false`, `undefined` and `null`, instead of previously only truthy vs falsy. Closes #8941

mary-poppins added cla: yes and removed cla: no labels Sep 5, 2014

Narretz reviewed Sep 5, 2014
View reviewed changes

IgorMinar reviewed Sep 5, 2014
View reviewed changes

tbosch force-pushed the asyncvalidation branch 2 times, most recently from 3dc38b7 to cdbab90 Compare September 5, 2014 22:48

tbosch force-pushed the asyncvalidation branch from cdbab90 to 09c8079 Compare September 5, 2014 22:57

tbosch changed the title ~~WIP: refactor(ngModelController,formController):~~ refactor(ngModelController,formController): Sep 5, 2014

tbosch added this to the 1.3.0-rc.1 milestone Sep 5, 2014

tbosch force-pushed the asyncvalidation branch from 09c8079 to 4f4af0c Compare September 5, 2014 23:02

tbosch mentioned this pull request Sep 5, 2014

feat(ngModelOptions): add allowInvalid option #8313

Closed

tbosch self-assigned this Sep 5, 2014

tbosch added the component: $location label Sep 5, 2014

tbosch added component: forms and removed component: $location labels Sep 5, 2014

tbosch mentioned this pull request Sep 5, 2014

Document ngModel.$setValidity breaking change in the Changelog #8357

Closed

petebacondarwin mentioned this pull request Sep 7, 2014

refactor(ngModelController): removed redundant this.$validators initialisation #8969

Closed

tbosch force-pushed the asyncvalidation branch 2 times, most recently from 4a8e051 to 24e8115 Compare September 8, 2014 18:45

tbosch force-pushed the asyncvalidation branch from 24e8115 to a3b3b68 Compare September 8, 2014 18:51

tbosch closed this Sep 8, 2014

tbosch deleted the asyncvalidation branch September 8, 2014 22:10

tbosch mentioned this pull request Sep 8, 2014

fix(input): $asyncValidators should allow $parsers #8892

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ngModelController,formController): #8941

refactor(ngModelController,formController): #8941

tbosch commented Sep 5, 2014

Narretz Sep 5, 2014

Narretz commented Sep 5, 2014

IgorMinar Sep 5, 2014

IgorMinar Sep 5, 2014

tbosch Sep 5, 2014

tbosch Sep 5, 2014

IgorMinar Sep 5, 2014

IgorMinar commented Sep 5, 2014

tbosch commented Sep 5, 2014

Narretz commented Sep 5, 2014

tbosch commented Sep 5, 2014

tbosch commented Sep 5, 2014

shahata commented Sep 5, 2014

IgorMinar commented Sep 8, 2014

refactor(ngModelController,formController): #8941

refactor(ngModelController,formController): #8941

Conversation

tbosch commented Sep 5, 2014

Narretz Sep 5, 2014

Choose a reason for hiding this comment

Narretz commented Sep 5, 2014

IgorMinar Sep 5, 2014

Choose a reason for hiding this comment

IgorMinar Sep 5, 2014

Choose a reason for hiding this comment

tbosch Sep 5, 2014

Choose a reason for hiding this comment

tbosch Sep 5, 2014

Choose a reason for hiding this comment

IgorMinar Sep 5, 2014

Choose a reason for hiding this comment

IgorMinar commented Sep 5, 2014

tbosch commented Sep 5, 2014

Narretz commented Sep 5, 2014

tbosch commented Sep 5, 2014

tbosch commented Sep 5, 2014

shahata commented Sep 5, 2014

IgorMinar commented Sep 8, 2014