fix(ngModel): don't run parsers when executing $validate #10048

Narretz · 2014-11-13T20:39:41Z

Previously, $validate would execute the parsers to obtain a
modelValue for validation. This was necessary, because a validator
that is called outside of model / view update (e.g. from an observer)
otherwise might only an undefined modelValue, because a previous
view update has found a validation $error and set the model
to undefined (as is tradition in angular)

This is problematic as validators that are run immediately after
the ngModelController initializes would parse the modelValue
and replace the model, even though there had been no user input.

The solution is to go back to an older design: the ngModelController
will now internally record the $$rawModelValue. This means a model
or view update will store the set / parsed modelValue regardless
of validity, that is, it will never set it to undefined because of
validation errors.

When $validate is called, the $$rawModelValue will passed to the
validators. If the validity has changed, the usual behavior is kept:
if it became invalid, set the model to undefined, if valid,
restore the last available modelValue - the $$rawModelValue.

Additionally, $validate will only update the model when the validity
changed. This is to prevent setting initially invalid models other
than undefined to undefined (see #9063)

Fixes: #9063
Fixes: #9959
Fixes: #9996
Fixes: #10025

Closes: #9890
Closes: #9913
Closes: #9997

Bonus: the commit which introduced parsing when validating (I actually wanted $$invalidModelValue to be removed, oh well): 9ad7d74

@caitp @petebacondarwin @lgalfaso

Narretz · 2014-11-13T20:41:15Z

test/ng/directive/inputSpec.js

@@ -934,6 +934,209 @@ describe('NgModelController', function() {
  });
 });

+describe('initialization', function() {


I need to clean up the tests, they are way to complex and still to specific - most of them apply to calling $validate in the controller, no input needed.

caitp · 2014-11-13T21:04:30Z

src/ng/directive/input.js

@@ -2110,6 +2137,7 @@ var NgModelController = ['$scope', '$exceptionHandler', '$attrs', '$element', '$
    }
    var prevModelValue = ctrl.$modelValue;
    var allowInvalid = ctrl.$options && ctrl.$options.allowInvalid;
+    ctrl.$$rawModelValue = modelValue;


I think you should clarify (with comments) what rawModelValue is, and why we need it. Otherwise it's not super clear

caitp · 2014-11-14T16:27:22Z

it looks "okay", but I'm not super happy about populating the ngModelController with more gunk, it is really confusing. So, it's okay if we have new fields on that object, but I'd appreciate if you document them (not with dgeni annotations, but just so that they're understandable to maintainers).

Should get another look from Pete, too.

caitp · 2014-11-14T16:27:56Z

I don't think we'll get it into 1.3.3 though :(

Narretz · 2014-11-14T17:01:29Z

I will add some comments; I just wanted to push it yesterday so someone can look at it before the weekend. Since it's such a "delicate" component, we shouldn't rush to get it into a release, but anyway do we even have enough material for a 1.3.3 release?

Regarding the gunkiness of ngModelController, I think it's a fair trade-off: add another field instead of calling a function ($$parseAndValidate) out of context (inside $validate).

Narretz · 2014-11-16T10:47:27Z

I added some comments, updated the docs, refactored the tests and fixed a bug where calling $validate would always unset parser errors.

@petebacondarwin can you take a look?

petebacondarwin · 2014-11-17T01:23:15Z

Superficially it looks good to me. I haven't tried it locally yet.
@shahata you have spent a lot of time in this bit of the code. Can you take a look?

shahata · 2014-11-17T10:51:10Z

LGTM

petebacondarwin · 2014-11-17T16:38:19Z

Let's get this merged

Narretz · 2014-11-17T18:20:06Z

I can merge in ca. 1 hour. Should this be part of 1.3.3?

Previously, $validate would execute the parsers to obtain a modelValue for validation. This was necessary, because a validator that is called outside of model / view update (e.g. from an observer) otherwise might only an undefined modelValue, because a previous view update has found a validation $error and set the model to undefined (as is tradition in angular) This is problematic as validators that are run immediately after the ngModelController initializes would parse the modelValue and replace the model, even though there had been no user input. The solution is to go back to an older design: the ngModelController will now internally record the $$rawModelValue. This means a model or view update will store the set / parsed modelValue regardless of validity, that is, it will never set it to undefined because of validation errors. When $validate is called, the $$rawModelValue will passed to the validators. If the validity has changed, the usual behavior is kept: if it became invalid, set the model to undefined, if valid, restore the last available modelValue - the $$rawModelValue. Additionally, $validate will only update the model when the validity changed. This is to prevent setting initially invalid models other than undefined to undefined (see angular#9063) Fixes: angular#9063 Fixes: angular#9959 Fixes: angular#9996 Fixes: angular#10025 Closes: angular#9890 Closes: angular#9913 Closes: angular#9997 Closes: angular#10048

shahata · 2014-11-18T07:35:44Z

@Narretz @petebacondarwin - Thinking about this some more, do you think it is okay that $validate changes $modelValue to undefined even if the last model value was programmatically assigned? I'm starting to think that we shouldn't test whether validation state has changed, but rather if the last assignment to the model was through user input...

Narretz · 2014-11-18T10:27:23Z

@shahata I think setting to undefined when validation fails is in line with how currently validation handles invalid values, no matter how they are introduced. Basically, I didn't want to special case $validate in this respect, or introduce a breaking change. But that doesn't mean that could change in the future, see also the discussion in #9959 and #10035. Respecting the source sounds like an idea we should keep in mind.

petebacondarwin · 2014-11-18T12:59:21Z

Can we start to look at these ideas more abstractly outside of the current implementation?

I have started a document here: https://docs.google.com/document/d/1-MhomULgCFOXVsqAGrI7l-cXYMYmJM4BMzgy_anC93o/edit?usp=sharing

shahata · 2014-11-18T18:40:13Z

@Narretz the current implementation of ng-model sets the model value to undefined only if validation fails in view updates (and $validate) and never in programmatic model updates.

Narretz · 2014-11-18T22:48:37Z

@shahata With current implementation do you mean what got merged now?

There are basically 3 cases:

programmatic change: the model will be formatted and validated, but not changed to undefined if invalid
$validate: the $$rawModelValue will be validated, and set to undefined if invalid
$setViewValue. The Model will be parsed and validated, and set to undefined if invalid

Before this PR, $validate also included parsing (undocumented), but behavior related to the setting model to undefined did not change. Reading my last comment again, I might haven't made this clear.

shahata · 2014-11-19T09:25:33Z

Yeah, I think we're saying the same thing :) I'm just a bit worried that ppl won't like it that $validate will change the model value to undefined even when there isn't any input from the user. It doesn't make a lot of sense since the general idea is not to change model value to undefined when the change is programmatic.

Narretz · 2014-11-19T10:27:28Z

There are some people who don't like it, but the specific behavior - setting invalid modelValues to undefined - hasn't changed for a long time, so we should have gotten some complaints about this much earlier if it was such a big deal. in the future, we should think about making this more customizable in a sane way (ideally without more ngModelOptions).

Narretz reviewed Nov 13, 2014
View reviewed changes

Narretz mentioned this pull request Nov 13, 2014

wip: fix(ngModel): don't update when neither validity nor modelValue changed #9890

Closed

caitp reviewed Nov 13, 2014
View reviewed changes

mary-poppins added cla: yes and removed cla: no labels Nov 13, 2014

caitp added component: forms type: bug labels Nov 14, 2014

caitp modified the milestones: 1.3.3, 1.3.4 Nov 14, 2014

Narretz force-pushed the fix-ngModel-ngChange-new branch from dab714d to 3ffeeb6 Compare November 15, 2014 22:52

Narretz added 2 commits November 17, 2014 20:26

test(ngModel): group validation tests

c9899c5

Narretz force-pushed the fix-ngModel-ngChange-new branch from 3ffeeb6 to e3764e3 Compare November 17, 2014 19:26

Narretz merged commit e3764e3 into angular:master Nov 17, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngModel): don't run parsers when executing $validate #10048

fix(ngModel): don't run parsers when executing $validate #10048

Narretz commented Nov 13, 2014

Narretz Nov 13, 2014

caitp Nov 13, 2014

caitp commented Nov 14, 2014

caitp commented Nov 14, 2014

Narretz commented Nov 14, 2014

Narretz commented Nov 16, 2014

petebacondarwin commented Nov 17, 2014

shahata commented Nov 17, 2014

petebacondarwin commented Nov 17, 2014

Narretz commented Nov 17, 2014

shahata commented Nov 18, 2014

Narretz commented Nov 18, 2014

petebacondarwin commented Nov 18, 2014

shahata commented Nov 18, 2014

Narretz commented Nov 18, 2014

shahata commented Nov 19, 2014

Narretz commented Nov 19, 2014

fix(ngModel): don't run parsers when executing $validate #10048

fix(ngModel): don't run parsers when executing $validate #10048

Conversation

Narretz commented Nov 13, 2014

Narretz Nov 13, 2014

Choose a reason for hiding this comment

caitp Nov 13, 2014

Choose a reason for hiding this comment

caitp commented Nov 14, 2014

caitp commented Nov 14, 2014

Narretz commented Nov 14, 2014

Narretz commented Nov 16, 2014

petebacondarwin commented Nov 17, 2014

shahata commented Nov 17, 2014

petebacondarwin commented Nov 17, 2014

Narretz commented Nov 17, 2014

shahata commented Nov 18, 2014

Narretz commented Nov 18, 2014

petebacondarwin commented Nov 18, 2014

shahata commented Nov 18, 2014

Narretz commented Nov 18, 2014

shahata commented Nov 19, 2014

Narretz commented Nov 19, 2014