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

Bug: $asyncValidators destroys data bound model property #10035

Open
wardbell opened this issue Nov 13, 2014 · 9 comments
Open

Bug: $asyncValidators destroys data bound model property #10035

wardbell opened this issue Nov 13, 2014 · 9 comments

Comments

@wardbell
Copy link

In v.1.3.2, when an $asyncValidator promise is rejected, Angular sets the data bound property to undefined. This effectively destroys what was a previously valid value for that property.

This seems very wrong to me. I'm curious if this was intended behavior; if so I'd like to know what could possibly justify permanently erasing the model property's value.

I've written a plunker to illustrate the behavior; it's README explains the behavior in greater detail and elaborates on the dangers of this feature.

I highly recommend avoiding $asyncValidator until this behavior is fixed.

@Narretz
Copy link
Contributor

Narretz commented Nov 13, 2014

Hi @wardbell, I don't think this is a bug. Since before 1.3.x, it has been the case that when the validation fails, the model will be set to undefined. This has been the case when $parsers were used for validation, and that wasn't changed when $validators and $asyncValidators were introduced.
I agree that the documentation doesn't make this clear - it only says that a rejected promise means the validator failed. That the model is set to undefined in this case is implicit, because invalid -> set to undefined has always been the case. We'll have to make this explicit in the docs.
We also have introcuced ngModelOptions.allowInvalid for you to keep invalid modelValues on the model.

@DanWahlin
Copy link

Thanks for the info. This is something Ward and I were working through yesterday in one of my apps and it was definitely surprising to see that the model was wiped out when a property was invalid. I would've expected it to remain intact but in an invalid state.

I just tried the ngModelOptions allowInvalid option and everything seems to work fine with that in place. It'd definitely be nice to have the docs clearly state that the model is blown away (not left intact and marked as invalid) when something is invalid in it. I suspect many others will struggle with this and wonder what's going on.

I'm not sure that I agree with that approach having worked with other frameworks that leave the model intact but mark it as invalid in that situation, but allowInvalid gives us the ability to control that which is good. Thanks again for the info!

@wardbell
Copy link
Author

I updated the plunker by referring to a new companion plunker that shows the effect of allowInvalid. That option I a life saver.

But I'm not ready to back down.

Firstly, I really don't care if this is the way it always worked this way with other forms of validation. I don't remember that and certainly didn't catch it before.

And if a component is going to do something so radically unexpected, it deserves documentation and high alert.

Doesn't matter. Wrong is wrong whenever it appears. I have yet to see an argument that it is ever acceptable to wipe out a valid model value without the developers explicit permission.

The allowInvalid gets me out of a jam. It's less than ideal because it allows the user to replace valid data with invalid data ... and that is not often the preferred approach.

I'd like to see a better argument made for the present behavior ... an argument other than "we always did it this way" ... before I surrender my claim that this is a bug.

One possibility is a new option "preserve valid value" or something like that 😊

@wardbell
Copy link
Author

Please reconsider your label "works as expected". I don't know anyone out here in the community who would say this behavior is expected. Lets think about that.

@DanWahlin
Copy link

I'd agree - that's not an expected behavior in any other framework that I've worked with over the past 15 years. :-)

@Narretz
Copy link
Contributor

Narretz commented Nov 18, 2014

The allowInvalid gets me out of a jam. It's less than ideal because it allows the user to replace valid data with invalid data ... and that is not often the preferred approach.

How would you handle the following case: There's an input which is required and the update (parsing, validation, set on scope) happens as soon as a key is pressed. The users enters text, which is validated and set on the scope. Then the user deletes all of his input. Since an empty input is invalid, should the model retain the last character the user typed, simply because it is valid?
My my main concern is that behavior like this will lead to out of sync view -> model values. This example would be especially confusing, if you where displaying the result of the input somewhere else in your app. The user would see an empty input, and a model that has the last character that he just deleted.

Can you tell me more about your application structure? Most people don't seem to have problems with the current behavior, so I am assume your structure is different.

@cruz210
Copy link

cruz210 commented Nov 18, 2014

Don't quite no what I'm doing from time to time.can you fork and pull and help me out

Sent from my android device.

-----Original Message-----
From: Narretz [email protected]
To: "angular/angular.js" [email protected]
Sent: Tue, 18 Nov 2014 4:34 AM
Subject: Re: [angular.js] Bug: $asyncValidators destroys data bound model property (#10035)

The allowInvalid gets me out of a jam. It's less than ideal because it allows the user to replace valid data with invalid data ... and that is not often the preferred approach.

How would you handle the following case: There's an input which is required nad the update (parsing, validation, set on scope) happens as soon as a key is pressed. The users enters text, which is validated and set on the scope. Then the user deletes all of his input. Since an empty input is invalid, should the model retain the last character the user typed, simply because it is valid?
My my main concern is that behavior like this will lead to out of sync view -> model values. This example would be especially confusing, if you where displaying the result of the input somewhere else in your app. The user would see an empty input, and a model that has the last character that he just deleted.

Can you tell me more about your application structure? Most people don't seem to have problems with the current behavior, so I am assume your structure is different.


Reply to this email directly or view it on GitHub:
#10035 (comment)

@frfancha
Copy link

@Narretz
The model must always be set when the parsers can run (except of course in this type of case: if the user types ABC, the parser to get a number can't run then the model is left untouched).

The idea to not set the model when the view is invalid (or to set it to null, not better), in order to get a model always valid and no code to check it isn't good because:
-in real life lot of validation rules must be coded between fields (as: field A must less than field B), not on a single field
-the code must have the incorrect values to be able to react and "do" something with these incorrect values

@wardbell
Copy link
Author

@Narretz I wasn't entirely clear on whether the flag to allow invalid data is true or false.

Invalid input blocked

The model property retains the last valid input value.

When the user clears the textbox, the UI displays the "required" indicator so the user can see what is wrong. If the bound object is simultaneously displayed elsewhere (a questionable design), it displays as the last valid value, not as null.

In this case the displayed value and the model value are out-of-sync. The UX developer knows this and designs for it.

Would a user who can see both views simultaneously be confused? I don't think so. The view with the incorrect value clearly indicates that the display value is in error while the other view shows the last known valid value. That doesn't sound confusing to me.

You certainly don't improve the situation by letting Ng zap the property value when the user enters invalid data. As my plunker shows (it has two views of the model property) it is weird that the user can be typing and suddenly the property in the alternate view goes blank.

Invalid input passed through

The model property would be null after the user clears the textbox. The UI displays the "required" indicator so the user can see what is wrong.

In this case the displayed value and the model value are in-sync.

If the bound object is simultaneously displayed elsewhere (a questionable design), it displays as null. Is this likely to be confusing? Possibly ... unless the alternate view also indicates that the value is invalid. That's easy for a BreezeJS model; not so easy for Ng.

I'd generally prefer to block invalid input when using Ng validation. The current defect prevents me from exercising that preference and forces me to allow invalid input.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants