adding possible fix for validating against NaN

[awei01]

When using Validator.number() on a field the Class instance casts the field if it is has type: "number" converting the value to NaN. When instance.validateAll() is run, it uses _.isNumber() on the value which returns true for NaN type. This seems like confusing behavior.

[lukejagodzinski]

Yes, it shouldn't treat NaN as a number. I will merge this PR.

[lukejagodzinski]

But there is one more question that rises when talking about numbers validation. Let's say, we have following field:

fields: {
  age: 'number'
}

Now, when trying to set on it the "123" value it will be converted properly.

doc.set('age', '123');
doc.get('age'); // 123 not "123" string

However, when we try to set the "abc" value it will be converted to NaN.

doc.set('age', 'abc');
doc.get('age'); // NaN

And, in my opinion it should be set to null, or maybe I'm wrong? Or maybe it shouldn't be set at all? Maybe it shouldn't change value, because it's not a proper value?

Let's take an example of how it works with dates:

fields: {
  birthDate: 'date'
}

/* ... */

doc.set('birthDate', 'asdasdasdsasd');
doc.get('birthDate'); // null

It assigns null value, which I think is wrong. It shouldn't assign a value at all. However, if we won't assign a value, values from the form won't be assigned. User may expect that values from the form will be assigned to the doc and validation will fail.

It's confusing... I don't know what's the correct approach.

[awei01]

For date, i think it should just do a simple return new Date(value); and cast to Invalid Date. It would satisfy the issue of validation provided we update the validation check on type date to also check for Invalid Date. For type number, casting to NaN is acceptable. Feel free to gitter IM me to discuss.

[lukejagodzinski]

Yes, you're probably right. We would have to change the way the Validators.date() validator work.

return _.isDate(date) && !_.isNaN(date.getTime()); // To check if it's not Invalid Date

I think it doesn't need more discussion :). It's just a right approach :)

Date and Number are only types that there is a problem. And your solution is good.

[awei01]

Ok, I can create a PR for the date validation in a bit.

Austin Y. Wei

On Mon, Sep 7, 2015 at 2:19 PM, jagi notifications@github.com wrote:

Yes, you're probably right. We would have to change the way the Validators.date() validator work.

return .isDate(date) && !.isNaN(date.getTime()); // To check if it's not Invalid Date

I think it doesn't need more discussion :). It's just a right approach :)

Date and Number are only types that there is a problem. And your solution is good.

— Reply to this email directly or view it on GitHub https://github.com/jagi/meteor-astronomy-validators/pull/24#issuecomment-138351805 .

[lukejagodzinski]

No no, it's not necessary. I will take care of this. I don't want to fix current version because I'm gonna have a lot of work with git rebasing. So I will do it on my branch that I'm currently working on. It's not a big deal :). However thanks for your help :)