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

Inconsistent handling of invalid initial ngModel values #8949

Closed
Narretz opened this issue Sep 5, 2014 · 15 comments · Fixed by #9375
Closed

Inconsistent handling of invalid initial ngModel values #8949

Narretz opened this issue Sep 5, 2014 · 15 comments · Fixed by #9375

Comments

@Narretz
Copy link
Contributor

Narretz commented Sep 5, 2014

An ngModelController created with an invalid inital value from scope behaves differently depending on input type:

http://jsfiddle.net/PK3QH/27/

Type invalid model view behavior error good
text {} [object Object] calls toString correctly set ⁉️
url 'notanurl' notanurl calls toString correctly set
number 'notanumber' (empty) ❌ throws if model is not type number ⁉️ no errors are set because of exception ❌
number+min=5 3 3 ^ correctly set
date 'notadate' (empty) ❌ expects a date and uses the date filte, else returns nothing required set
date new Date('123456') 0NaN-NaN-NaN ^ $error.date is not set ❌

I think we should always try to set the $viewValue to something. This is important if you have server-side data that may be invalid.
So number and date should not discard invalid values. If we pass garbage to the validators, the correct errors will be set.
Regarding calling toString on text-based input. I think that's okay as you will rarely set an object to a model when you know it'll be displayed in an input. But we should do "the right thing" and try to format values that are good enough, so in short:

  • number should not throw and call toString regardless
  • date should return call toString if not a Date object
  • date should also get the bug fixed where NaN gets output

I'll think I'll be able to throw a PR together over the weekend.

@Narretz Narretz added this to the 1.3.0-rc.2 milestone Sep 5, 2014
@caitp
Copy link
Contributor

caitp commented Sep 5, 2014

You won't be able to fix a number of these that you're claiming to be "not good" (otherwise we'd have inconsistencies between user specified values and model-specified values)

@Narretz
Copy link
Contributor Author

Narretz commented Sep 5, 2014

I admit, I haven't looked into it. Do you mean because of native validators?

@caitp
Copy link
Contributor

caitp commented Sep 5, 2014

You can't set a date or number input's values to something which doesn't fit the date or number parsing rules (I'm pretty sure that's the end result of what we do in setViewValue) http://jsfiddle.net/egedyh1v/

@Narretz
Copy link
Contributor Author

Narretz commented Sep 5, 2014

Ah, I see what you mean. However, I think I can improve the handling a little bit.
And the last entry, the date, that looks like a bug in the date filter.

@Narretz
Copy link
Contributor Author

Narretz commented Sep 5, 2014

Well, yeah I see what you mean. I think the code needs some comments why we're doing this.

@IgorMinar
Copy link
Contributor

This is related to #9218

We should consider just throwing an error when we detect incorrect type rather than calling toString to convert the value. But need a discussion about this.

@jeffbcross jeffbcross removed their assignment Sep 30, 2014
@jeffbcross
Copy link
Contributor

Assigned to @tbosch since he's working on #9218, which should resolve most of these issues.

@tbosch
Copy link
Contributor

tbosch commented Oct 1, 2014

Yes, we should throw if the model has an invalid type. This would do the following:

  • For input[text], input[url], input[email]: throw if the model value is not a string (to be implemented in a formatter).
  • For input[date]: throw if the model value is not a date. Invalid Dates in the model (e.g. created via new Date('1234567') will not throw but result in an empty input with an error InvalidDate set.
  • For input[number]: throw if the model value is not a number. NaN in the model will not throw but result in an empty input with an error NaN set.

@Narretz Does this sound good to you?

@Narretz
Copy link
Contributor Author

Narretz commented Oct 1, 2014

I'm not really a fan of throwing in these cases. Just to be clear, the reason is that we don't want the formatting process to change the model? Otherwise we could at least try to "parse" the model. Or is the parsing part what makes this hard?
Anyway, it's definitely a breaking change for text and date. I expect it'll break quite a few people's apps that rely on a number displaying "okay" in a text input.

@tbosch
Copy link
Contributor

tbosch commented Oct 1, 2014

Ok, talked again with @IgorMinar and we came up with this:

  • throw for input[date] and input[number] if the model value is not a date / number, as we have no good way to convert the model value. We don't add validation errors as the user was not responsible for the incorrect types in the model and would rather only be confused.
  • for input[text] we keep the old behavior of just converting the value to a string as too many people rely on this (as @Narretz pointed out).

tbosch added a commit to tbosch/angular.js that referenced this issue Oct 1, 2014
…e/…]`

Similar to `input[number]` Angular will throw if the model value
for a `input[date]` is not a `Date` object.
For `Invalid Date`s (dates who’s `getTime()` is `NaN`) `input[date]` 
will render an empty string.

Closes angular#8949
tbosch added a commit to tbosch/angular.js that referenced this issue Oct 1, 2014
…e/…]`

Similar to `input[number]` Angular will throw if the model value
for a `input[date]` is not a `Date` object.
For `Invalid Date`s (dates whose `getTime()` is `NaN`) `input[date]` 
will render an empty string.

Closes angular#8949
Closes angular#9375
tbosch added a commit to tbosch/angular.js that referenced this issue Oct 1, 2014
…e/…]`

Similar to `input[number]` Angular will throw if the model value
for a `input[date]` is not a `Date` object.
For `Invalid Date`s (dates whose `getTime()` is `NaN`) `input[date]` 
will render an empty string.

Closes angular#8949
Closes angular#9375
tbosch added a commit to tbosch/angular.js that referenced this issue Oct 1, 2014
…e/…]`

Similar to `input[number]` Angular will throw if the model value
for a `input[date]` is not a `Date` object.
For `Invalid Date`s (dates whose `getTime()` is `NaN`) `input[date]`
will render an empty string.

Closes angular#8949
Closes angular#9375
@btford
Copy link
Contributor

btford commented Oct 1, 2014

👏

@iamdey
Copy link

iamdey commented Nov 7, 2014

woops... In my case I assume the user is responsible of his data and allow him to save invalid values. We were able (angular 1.2) to show him what's wrong each time he open the form.

Well I'm not sure how to reproduce previous behavior.

@Narretz
Copy link
Contributor Author

Narretz commented Nov 7, 2014

What is "show him what's wrong each time he open the form" supposed to mean? Please elaborate.

@iamdey
Copy link

iamdey commented Nov 7, 2014

Here is the case, the "form" is a meta query, this query cannot be executed until the parameters are not valid (a date or number depend of the parameter's type).
May be the user wants a draft, so we allow him to save whatever he wants. We use ngModel.$invalid to indicate how to fix his query.

So yes, the server must give incorrect values as it has been saved and yes I want to use native input type date/number.

I'm surprised that angular assume the provided values must be correct. May be there is a UX mistake in my design, but I should not be the only one.

@Narretz
Copy link
Contributor Author

Narretz commented Nov 7, 2014

Although I still have problems understanding exactly what you are doing, it seems like ngModelOptions.allowInvalid should solve your problems. You should provide an example of what you're doing if this is not the case.

----- Ursprüngliche Nachricht -----
Von: "David Epely" notifications@github.com
Gesendet: ‎07.‎11.‎2014 16:38
An: "angular/angular.js" angular.js@noreply.github.com
Cc: "Narretz" mjstaffa@googlemail.com
Betreff: Re: [angular.js] Inconsistent handling of invalid initial ngModelvalues (#8949)

Here is the case, the "form" is a meta query, this query cannot be executed until the parameters are not valid (a date or number depend of the parameter's type).
May be the user wants a draft, so we allow him to save whatever he wants. We use ngModel.$invalid to indicate how to fix his query.
So yes, the server must give incorrect values as it has been saved and yes I want to use native input type date/number.
I'm surprised that angular assume the provided values must be correct. May be there is a UX mistake in my design, but I should not be the only one.

Reply to this email directly or view it on GitHub.=

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