Allow string date timestamps in `intlDate`, resolves #25

[clarle]

Quick fix for #25 to allow date timestamps as strings.

There's a failing test, but was there in master beforehand.

[yahoocla]

CLA is valid!

[ericf]

Does Dust really only support data being passed as a String to helpers? If Dust does support passing numbers, then I don't think we should make this change, and instead people should pass their timestamp as a number.

[caridy]

-1

I don't think the helper should be doing bold assumptions about the value of it. It is too ambiguous, e.g:

$ node
> new Date("1/23/2014").toString()
'Thu Jan 23 2014 00:00:00 GMT-0500 (EST)'
> new Date("1390518044403").toString()
'Invalid Date'
> new Date("2014").toString()
'Tue Dec 31 2013 19:00:00 GMT-0500 (EST)'
> new Date(2014).toString()
'Wed Dec 31 1969 19:00:02 GMT-0500 (EST)'

[fhemberger]

The helper could explicitly check if the argument passed is a timestamp string and handle this special case (add after #L261):

// Detect if `val` is a timestamp string. Convert to Number.
var parsedVal = parseInt(val, 10);
if (new Date( parsedVal ).getTime() == val) {
   val = parsedVal;
}

This lets you narrow things down to the handling of this specific use case without causing other issues or side effects.

[caridy]

@fhemberger the problem is that any integer parsed by parseInt() will be a valid timestamp, e.g.:

$ node
> new Date(2014).getTime()
2014

which means that your condition will do nothing to prevent this for happening, again, this should be in the user-land, not in the helper. If they want to handle this case, they can easily create another helper to modify the value by applying the proper parsing during the template definition, or just before calling render().

[fhemberger]

Ok, but in this case it should be clearly documented in the README that timestamps as strings are not supported. I'll add it to #28.

[caridy]

@fhemberger no, a timestamp is NOT a string, it is a numeric value, period. why should we clarify something like that?

[fhemberger]

… because several people obviously already stumbled on this, which may lead to further similar issue reports in the future. When you publish a library for others to use, being a bit more explicit in the docs doesn't hurt. Not everybody who ends up using it might have the same level of experience as you have.

I added a simple suggestion in #28 (35e130d8bb8aff80d750f7984f36b2e46e8237e6), just have a look at it and think about it. You don't need to merge it if you don't like it ;)

[caridy]

@fhemberger sure, I will not oppose to that change, which is just a line. I just don't want a section in the docs going in circles about this.