form.$error.required and form.$invalid validators are not syncing

[Dean-Christian-Armada]

I was wondering if this is an angular bug. The $invalid is undefined while my $error.required is true when a custom $validators is declared on a directive.

Check this example: http://plnkr.co/edit/NuDGB64IetpcsaVB03T7?p=preview

[gkalpak]

This seems like an Angular bug indeed. BTW, it has been broken since v1.3.0-rc.1!

[Dean-Christian-Armada]

Oh ok, so that's why thanks for the response

[gkalpak]

After further investigation, I'm not sure if this is Angular's fault or your directive's.
There is definitely a bug in your directive, but I'm not sure if Angular should treat it differently - it's a gray, undocumented area 😃

Here's what happens:
In your custom validator, when modelValue is falsy (e.g. undefined, as happens initially), you return check without ever initializing it. You declare var check inside the if block, but since var is function scoped (not block scoped), the declaration gets hoisted to the top of the function block. Thus, when you return check, check is declared but not initialized (i.e. undefined).
This is obviously unintentional. You should change the validator to something like:

function (modelValue, viewValue) {
  var check = true;
  if (modelValue) {
    ...
  }
  return check;
}

Now, Angular normally expects a boolean (true or false) to be returned from a validator. Returning undefined triggers a special behavior though: It designates that the validation is pending and sets both $invalid and $valid to undefined. This behavior was probably intended (and is internally used) for async validators.
Returning undefined from a sync validator, tricks Angular into setting the valididty state as $pending (i.e. as happens with async validators).

Feature or Bug ?

I lean towards the latter. @Narretz, thoughts ?

BTW, the fix would be to change the first line here, so that result is always a boolean, regardless of what the validator returns: var result = !!validator(modelValue, viewValue);
(But it might be a breaking change...)

[Dean-Christian-Armada]

Ok, I understand your point but since the $validator is 'available' as it is declared.

ngModel.$validators.available = function (modelValue, viewValue) {

}

It should have been only reflected on the $error.available and not the whole $invalid.. That is from my point of view. I'm aware that this can be solved by doing a little extra conditional in the directive like

if(ngModel.$error.required){
check = true;
}else{
check = false;
}

But that's another additional manual coding. So as much as possible I would like to avoid it. But thanks for the response, I don't know if it still considered as a bug though

[gkalpak]

It should have been only reflected on the $error.available and not the whole $invalid.

Not really. The $valid/$invalid refer to the validity of the control as a whole; i.e. taking all validators into account. If one validator (in this case available) has not determined yet if the value is valid or not (by returning undefined), then the whole validity can't be determined.

I'm aware that this can be solved by doing a little extra conditional in the directive like
if (ngModel.$error.required) { check = true; } else { check = false; }

Not sure what you mean. required doesn't have anything to do with this issue. This is about returning a boolean value from your validator.

[Dean-Christian-Armada]

This is what I meant Just check the code below and just take a look on the 2nd to 6th line that I have added.

ngModel.$validators.available = function (modelValue, viewValue) {
if(ngModel.$error.required){
check = false;
}else{
check = true;
}
if(modelValue){
var check = modelValue.split('-');
var y = parseInt(check[0], 10);
var m = parseInt(check[1], 10);
var d = parseInt(check[2], 10);
var date = new Date(y,m-1,d);
if (date.getFullYear() == y && date.getMonth() + 1 == m && date.getDate() == d) {
check = true;
} else {
check = false;
name = ngModel.$name;
if(datePatternCounter.indexOf(name) == -1){
datePatternCounter.push(name);
element.parent().siblings("span.errors").append(' * Invalid Date
');
}
}

                    }

                    return check;
                }

It's working great so far. You can take a loot at this plunker prevew: http://plnkr.co/edit/NuDGB64IetpcsaVB03T7?p=preview

[gkalpak]

I still don't understand. The required validity state is irrelevant here. You should just make sure you return a boolean value (whatever makes sense in your usecase) even if the modelValue has not been set.

Previously, you were returning undefined and that is what was causing issues.

[Dean-Christian-Armada]

Ok, understood. Thanks for the help!

[Narretz]

Regarding your comment in #14734 (comment) @gkalpak about returning undefined from a sync validator, it's definitely not documented that you can do that. But the question is what should happen when you do it? The current behavior isn't ideal, but should we throw?

[gkalpak]

I think it makes sense to convert the value to boolean.

Note that we are already (sort of) doing this, when we determine the overall sync validators validity (here. But we are still setting the validity of the specific validator to undefined (which triggers the whole undocumented/unwanted behavior).

Essentially, I am proposing to change this line (which will only affect the behavior of that line) like this:

- var result = validator(modelValue, viewValue);
+ var result = !!validator(modelValue, viewValue);

[BobChao87]

While writing up tests for the change, I noticed something possibly unexpected. Aside from the expected true -> true, false -> false, and undefined -> undefined, the following also exist:

null -> true
0 -> true
'' -> true
NaN -> true

Since all of these values are falsey, this change will actually alter their behaviour if used as the return values of validator functions. Of course, we only ever expected true or false from a validator, so this use case should be fairly minimal. Nonetheless, this will be a breaking change.

Edit: Note: It would appear every value results in true being output except for false and undefined currently.

[gkalpak]

@BobChao87, do you have a PR or branch I can take a look at? What tests are you referring to exactly?

[BobChao87]

Hopefully that resolves any questions about what I was trying to describe. The changes are all in the negative tests.

[brijesh1ec]

Hi,

Because $error is object will have error if it is invalid. while debugging
if we check $error is true.

Thanks,
Warm Regards :Brijesh Pal
Mobile : +919790752446

On Tue, Sep 27, 2016 at 1:56 PM, BobChao87 [email protected] wrote:

Hopefully that resolves any questions about what I was trying to describe.
The changes are all in the negative tests.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#14734 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGltbJFIcEmF6kLzyvJGsNhGbUlV0J7lks5quNM7gaJpZM4IwvAH
.

[gkalpak]

@BobChao87, I see. This seems to be an unexpected side effect of an unintended use. Internally, the $setValidity treats values as follows:

• undefined: Set validity to pending.
• true: Set validity to true for the specified validator.
• false: Set validity to false for the specified validator and the parent form/ngForm.
• everything else: Clear the $error/$$success fields, which effectively sets the field's validity to true.

AFAICT, this last "feature" is intended for internal use only and specifically used with null. Since it was never documented that sync validators should return anything other than boolean values, I believe it is a totally reasonable breaking change.
(I only expect this to affect very very few people (if any) and even it's not our fault 😃)

[BobChao87]

Hmmm, okay, sounds good to me. I'll clean up the code and write up a breaking change note and get that in in the next couple of days. I agree, should be a very small cross-section of users that will be affected by this change.