Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

null is a valid date #537

Closed
raine opened this issue Aug 5, 2017 · 18 comments
Closed

null is a valid date #537

raine opened this issue Aug 5, 2017 · 18 comments
Milestone

Comments

@raine
Copy link

raine commented Aug 5, 2017

Seemed like unexpected behavior to me.

% replem date-fns@2.0.0-alpha.1:D
Installed into REPL context:
 - date-fns@2.0.0-alpha.1 as D
> D.isValid(null)
true
@kossnocorp
Copy link
Member

It not as straightforward as it looks.

In ECMAScript, new Date(null) equals to new Date(0):

new Date(null)
//=> "Thu Jan 01 1970 04:00:00 GMT+0400 (+04)"

new Date(0)
//=> "Thu Jan 01 1970 04:00:00 GMT+0400 (+04)"

// but:

new Date(undefined)
//=> "Invalid Date"

new Date('asd')
//=> "Invalid Date"

This behavior is consistent across all date-fns functions, so if you add a day to null, it will be Jan 02 1970.

The library sticks to ECMAScript behavior, even in edge cases like this one. It could sound controversial, but it ensures consistency both across date-fns API and with ES.

Initially, I wanted to close this issue as "Won't fix", but there is a nuance that stops me: there is no such thing as undefined in JSON so that it could be a common pitfall. I will consider changing this behavior if there are more pros than cons.

Please share your thoughts.

@kossnocorp kossnocorp added the 🤔 Feedback wanted Community feedback is required label Sep 1, 2017
@zhuangya
Copy link

zhuangya commented Sep 12, 2017

IMHO: since null == 0 is false, new Date(null) equals to new Date(0) is not so sensible for me.

so, i think, D.isValid(null) should be false

@frontsideair
Copy link

I think null should not be considered a valid date. null can be an indicator of a failure in a computation that may fail and it should be caught before it propagates.

@ai
Copy link

ai commented Sep 12, 2017

I think null should be an invalid date. null is not a value, it is an empty value. Its meaning is very different from 0. So for my opinion Date(null) is a mistake in JS design. And we should fix it in this API.

@jquense
Copy link

jquense commented Sep 12, 2017

Having built a few date pickers the native New Date behavior around null has only ever caused bugs. DateFns is good place to adjust these unintuitive native behaviors. I'd vote for null to be not a valid a date

@threepointone
Copy link

I think date-fns has the opportunity here to correct the spec; my vote, like the above others, is for Date(null) not to be valid.
(thanks for a great lib!)

@bpierre
Copy link

bpierre commented Sep 12, 2017

Is there any known example of null being used for a good reason, using new Date or date-fns? If not, I vote for null not to be valid.

@lski
Copy link

lski commented Sep 12, 2017

For me as well, D.isValid(null) === false, as long as it is consistent in your functions from this library then it can be considered expected behaviour.

A reason for creating a library in the first place is to make reasoned 'fixes' and normalisations to bizarre behaviours in the underlying language in my opinion anyway.

@brentvatne
Copy link

I agree with the popular sentiment in this thread -- unless there are some solid practical reasons that we can enumerate (as suggested by @bpierre), D.isValid(null) should return false.

@SirMcPotato
Copy link

I agree with pretty much all that was said before, especially with @ai comment. Null is not 0, and Date(null) treated as a valid date has always been an erratic behavior in my opinion.

@zhuangya
Copy link

zhuangya commented Sep 12, 2017 via email

@maggiepint
Copy link

For what it's worth, Moment's behavior invalidates null - but we've never attempted to follow the Date object for any reason (except our terrible choice to have zero-indexed months :-)).

I don't recall any GitHub issues where people complained about this. I think it would be safe to move forward by having null returned an invalid date, and in fact, I think it is what most users would expect.

@maggiepint
Copy link

Note: I'm looking at the ECMAScript spec, and while it is indeed the case that null is treated the same as 0, I think it was completely unintentional. The spec passes 1 parameter, non-string invocations of the date constructor through the toNumber function to normalize them - thus you get this result. Probably an edge case nobody thought about at the time.

If you wish to bikeshed this for the future of JavaScript - I threw an issue in the Temporal proposal to address: tc39/proposal-temporal#49

@lpalmes
Copy link

lpalmes commented Sep 12, 2017

I agree in considering ‘new Date(null)’ as an invalid value. Truthy and falsely values are a bit cumbersome in JS.

@AutoSponge
Copy link

Unless the value implements the Date interface (via prototype) or is a string/JSON representation of a valid Date, IMO it's not a Date. Null does neither.

@charliesbot
Copy link

I agree with most of the comments. Null shouldn’t be a valid valid to create a date. It’s the reason of several bugs that are hard to track.

This is a great opportunity to solve that.

My two cents.

@kossnocorp
Copy link
Member

Thanks, everyone who replied! It's great to see unity about the issue, isValid(null) === false it is!

I'll go through API and apply this change everywhere.

@kossnocorp
Copy link
Member

The fix is released v2.0.0-alpha.3. See the change log: https://gist.github.com/kossnocorp/a307a464760b405bb78ef5020a4ab136#v200-alpha3

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

No branches or pull requests