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

Fix datepicker DST bug #2212

Merged
merged 2 commits into from Oct 17, 2018
Merged

Fix datepicker DST bug #2212

merged 2 commits into from Oct 17, 2018

Conversation

wch
Copy link
Collaborator

@wch wch commented Oct 16, 2018

Fixes #2204.

This fix is borrowed from:
uxsolutions/bootstrap-datepicker@1388539#diff-dd513a8bab7ad1033c8784c4a1b9ce15

Testing notes

Change your operating system's time zone to Melbourne, Australia. Then run this app:

library(shiny)

ui <- fluidPage(
  dateInput("date", "date:", value = "2018-10-01")
)

server <- function(input, output, session) {
  updateDateInput(session, "date", value = "2018-10-07")
}

shinyApp(ui, server)

The repro is that the date input ends up at "2018-10-06" instead of "2018-10-07".

@wch wch requested a review from jcheng5 October 16, 2018 19:59
@jcheng5
Copy link
Member

jcheng5 commented Oct 16, 2018

I'm writing up why this fix works, while it's fresh in my head.

First of all, Date objects in JavaScript DO NOT have a time zone property attached. There's no way to create a Date object that is inherently UTC or inherently EST. Instead, Date objects represent an instant in time, independent of time zone. When printed, they always print using the timezone that corresponds to the system's current locale. (If you want to see the UTC representation, you can do so with the .toUTCString() method.)

So right off the bat, the function name _utc_to_local is a little misleading. You'd think the input and output dates would represent the same instant in time, only with a different time zone (and thus potentially different minutes/hours/days/months/years). But this is not the case.

Instead, it's like this. First, we specify a nice round date/time in explicit UTC.

> let moment = new Date("2018-10-07T00:00:00Z")  // Specifying initial value in UTC

When converted to string, it shows the corresponding local time.

> moment.toString()
"Sat Oct 06 2018 17:00:00 GMT-0700 (Pacific Daylight Time)"

We can also look at the UTC string if we want to.

> moment.toUTCString()
"Sun, 07 Oct 2018 00:00:00 GMT"

What _utc_to_local does is give us a completely different moment in time, whose toString() looks like the input's toUTCString(), other than the time zone.

> _utc_to_local(moment).toString()
"Sun Oct 07 2018 00:00:00 GMT-0700 (Pacific Daylight Time)"

The bug in the previous implementation is that it simply added the timezone offset (minus 7 hours in the above case) to the input time. This works, except when the shift in time lands you in a different timezone than you started from. Moving to Melbourne time now (GMT+1000 during the winter, GMT+1100 during the summer, with the changeover from former to latter happening on Oct 7 2018 at 2AM):

> let moment = new Date("2018-10-07T00:00:00Z")  // Specifying initial value in UTC
> moment.toString()
"Sun Oct 07 2018 11:00:00 GMT+1100 (Australian Eastern Daylight Time)"
> moment.toUTCString()
"Sun, 07 Oct 2018 00:00:00 GMT"
> new Date(moment.getTime() + moment.getTimezoneOffset() * 60000).toString()
"Sat Oct 06 2018 23:00:00 GMT+1000 (Australian Eastern Standard Time)"

We started at 11AM in AEDT, but by subtracting 11 hours, we ended up moving into the AEST timezone, so the date/time don't look nice and round like we wanted them to.

The new implementation works by starting with the same logic, but double-checking the result to see if we've switched time zones; and if so, take the input time and add that second timezone's offset instead.

This feels a little weird to me, it felt like there might possibly be an edge case where using the second timezone's offset would result in the new time not ending up in the new timezone after all. I thought a more direct route would be to use an implementation like this:

return new Date(utc.getUTCFullYear(), utc.getUTCMonth(), utc.getUTCDate(),
                utc.getUTCHours(), utc.getUTCMinutes(), utc.getUTCSeconds(),
                utc.getUTCMilliseconds());

I tried iterating through all of the moments of 2018, in 10 second intervals, to look for differences between the new _utc_to_local and this implementation. The only time the results differed were in the April transition from AEDT to AEST ("fall back"), where it's ambiguous what you mean by "April 1, 2018, 02:30:00 local time in Melbourne" as there are two moments that match this description (one each for AEDT and AEST). My code ends up picking AEDT, whereas the code in this PR picks ASDT, which seems more correct since that was the starting time zone.

Update: Actually found another case; new Date(1520736900000) in Mountain time zone represents a time whose UTC representation ("Sun, 11 Mar 2018 02:55:00 GMT") can't be duplicated in local time, because at 2AM MST it jumps straight to 3AM MDT. The two functions happen to choose different directions once again.

I've recorded these tests here:
http://jsfiddle.net/jcheng/zcn8g0kL/18/

NEWS.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants