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

Daily times are wrong around DST when system is not UTC #233

Closed
davidgoli opened this issue Aug 3, 2018 · 5 comments
Closed

Daily times are wrong around DST when system is not UTC #233

davidgoli opened this issue Aug 3, 2018 · 5 comments

Comments

@davidgoli
Copy link
Collaborator

davidgoli commented Aug 3, 2018

There are a number of much older issues around this (#65 , #157 , #213) and I was hoping it would be resolved with a fix for #38 et al., and a fix (without a test) was even merged in for it (#232), but it continues to be an issue.

Repro:

const rrule = RRule.fromString('DTSTART=20181101T100000Z;UNTIL=20181106T100000Z;FREQ=DAILY')
expect(rrule.all()).to.deep.equal([
  new Date('2018-11-01T10:00:00.000Z'),
  new Date('2018-11-02T10:00:00.000Z'),
  new Date('2018-11-03T10:00:00.000Z'),
  new Date('2018-11-04T10:00:00.000Z'),
  new Date('2018-11-05T10:00:00.000Z'),
  new Date('2018-11-06T10:00:00.000Z')
])

Result:

      AssertionError: expected [ Array(5) ] to deeply equal [ Array(6) ]
      + expected - actual

       [
         [Date: 2018-11-01T10:00:00.000Z]
         [Date: 2018-11-02T10:00:00.000Z]
         [Date: 2018-11-03T10:00:00.000Z]
      -  [Date: 2018-11-04T11:00:00.000Z]
      -  [Date: 2018-11-05T11:00:00.000Z]
      +  [Date: 2018-11-04T10:00:00.000Z]
      +  [Date: 2018-11-05T10:00:00.000Z]
      +  [Date: 2018-11-06T10:00:00.000Z]
       ]

The resulting times for a daily repeating routine are off by 1 hour after "fall back". This only reproduces when the machine's time zone is one with DST, so it does not exist under UTC, for example.

@davidgoli
Copy link
Collaborator Author

davidgoli commented Aug 3, 2018

This is because timeset is setting the time to be a consistent time (which is the daily routine's time in UTC), but the JavaScript Date constructor is then applying the local timezone offset (here, changing from PDT to PST):

> new Date(2018, 10, 3, 4, 0, 0, 0)
2018-11-03T11:00:00.000Z
> new Date(2018, 10, 4, 4, 0, 0, 0)
2018-11-04T12:00:00.000Z

The combine function should not naively combine dates and times, but rather needs to adjust the time for the appropriate timezone offset on that date.

@davidgoli
Copy link
Collaborator Author

One possible solution would involve avoiding the new Date(year, month, day, hour, minute, second) constructor altogether, instead building the ISO date string, which does not show this buggy behavior.

@davidgoli
Copy link
Collaborator Author

davidgoli commented Aug 3, 2018

The initial conversion comes in parsing the byhour Options field, which for daily/weekly/monthly/yearly frequencies, calls dtstart.getHours(), which behaves like this:

> const date = new Date('2018-11-01T11:00:00.000Z')
undefined
> date.getHours()
4

It would seem most logical that the internal representation of the desired time should be the requested time in UTC and not converted to local time (then back later).

@davidgoli
Copy link
Collaborator Author

I have a fix for this, pending #228

@davidgoli
Copy link
Collaborator Author

Fixed by #235 .

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

No branches or pull requests

1 participant