-
Notifications
You must be signed in to change notification settings - Fork 111
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
fix: ensure dates are parsed timezone agnostically #1234
Conversation
db11477
to
d089fbd
Compare
@@ -86,7 +86,7 @@ export function Datetime(props) { | |||
|
|||
switch (subtype) { | |||
case DATETIME_SUBTYPES.DATE: { | |||
date = new Date(Date.parse(value)); | |||
date = new Date(value ? value + 'T00:00' : NaN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this NaN is already what the initial Date.parse was doing. Basically we want to end up with an invalid date because that's how the implementation was initially done. So I'm just doing this to least divert from the previous behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically having this whole condition isn't necessary, because doing date of null + T00:00 will also return an invalid date, but I just wanted to be more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand what you mean, the previous code seemed fine.
What do you mean by "least divert from the previous behavior"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, we need to add T00:00 to ensure we normalize.
The date.parse was redundant except when value was null in which case it would convert it to NaN which would then get converted to 'Invalid date' by the Date constructor. Because Date(null) actually is valid and creates todays day.
So I removed the Date.parse because it obfuscated a bit of things, and I know value will always be either null or a valid date string, so this seems like the most clear way of replicating the behavior.
I could also do new Date(Date.parse(value + 'T00:00')). But then I might as well just do new Date(value + 'T00:00')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to better to add a explicit check for the type and not use this hacky solution of manually appending the time, like so:
typeof value === 'string' ? new Date(Date.parse(value)) : new Date(NaN)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this will not work. The whole point of the change is to add the time so that we treat a date like 1996-11-20
as 1996-11-20T00:00+??:??
or whatever time, but we want to treat it as that day in my timezone
.
The default behavior to parse 1996-11-20
is to treat it as 1996-11-20T00:00Z
which is not the intent of a user, because for negative timezones that would mean the day before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So adding T00:00 isn't a hack, it's specifying the behavior. That part I'm sure on. We could also set it to 12:00 it doesn't matter but JS doesn't handle dates by themselves in any kind of way so to make sure the right day is picked, we need to add T00:00
(or any time really).
This below is the behavior for an American timezone.
new Date('2015-01-23')
Date Thu Jan 22 2015 14:00:00 GMT-1000 (Hawaii-Aleutian Standard Time)
new Date(Date.parse('2015-01-23'))
Date Thu Jan 22 2015 14:00:00 GMT-1000 (Hawaii-Aleutian Standard Time)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also, date.parse is redundant in the above example. new Date(Date.parse(validDate)))
is the same as new Date(validDate)
const year = date.getFullYear(); | ||
const month = String(date.getMonth() + 1).padStart(2, '0'); | ||
const day = String(date.getDate()).padStart(2, '0'); | ||
return `${year}-${month}-${day}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use toISOString()
and split by T
and get the first element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The behavior is different, toISOString converts to UTC which would not extract the right date if the timezone difference is large enough.
@@ -86,7 +86,7 @@ export function Datetime(props) { | |||
|
|||
switch (subtype) { | |||
case DATETIME_SUBTYPES.DATE: { | |||
date = new Date(Date.parse(value)); | |||
date = new Date(value ? value + 'T00:00' : NaN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I fully understand what you mean, the previous code seemed fine.
What do you mean by "least divert from the previous behavior"?
d089fbd
to
ccd0d7a
Compare
ccd0d7a
to
fa63f97
Compare
Closes #1223