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
Daylight Savings change not handled correctly despite "Timezone Support" #300
Comments
Are you getting any console warnings? eg:
Luxon is in When I run your code in the rrule test suite, I get:
which is what I'd expect, since the time you requested is 1900 in the Denver timezone. (Since this is JavaScript and not PHP, we can't give you |
The fact that you're seeing local (MST/MDT) dates and not UTC dates is a bit baffling to me. As far as I know, this library is currently configured to only ever return UTC dates, though it previously used to return local dates instead. |
No console warnings. It appears Luxon is provided properly. As far as the timezones, I get MST/DST as a result of running my code directly in the Chrome devtools console command line. Steps: Open Chrome --> Navigate to page with RRule and Luxon provided --> Cmd+Opt+I --> "Console" tab --> copy and paste above snippet into command line at bottom --> Enter |
Do you know which version this changed at? Why would that change be made? Returning only UTC dates actually introduces this daylight savings discrepancy! An event that always occurs at 13:00 UTC pretty much guarantees that the event will start at a different time before/after a daylight savings switch. I think this change should be reverted, or else another alternative method should be provided that allows Daylight Savings to be accounted for. UTC is extremely useful but it is not always the correct approach in all situations. When working with recurring events we HAVE to take into account the specific timezone the event occurs in specifically because daylight savings is a thing. |
Indeed, I get different behavior from Chrome than I do from Node. I would say I don't understand why your given "expected" output is what you're really expecting. Those times appear incorrect. What would be correct is the times if you added the TZ offset for each time to the given local time. Then all the times will be 19:00. This is why RRule deals in UTC, and why its usage of UTC has nothing to do with this particular bug. Indeed, if you do: const dates = RRule.fromString(
"DTSTART;TZID=America/Denver:20181101T190000;\n"
+ "RRULE:FREQ=WEEKLY;BYDAY=MO,WE,TH;INTERVAL=1;COUNT=7"
).all()
dates.map((date) => date.toISOString()) you'll see the resulting ISO strings with consistently the correct dates and times. It appears, moreover, that Chrome does not support returning Dates in UTC at all: > new Date(Date.UTC(2016, 10, 5))
Fri Nov 04 2016 17:00:00 GMT-0700 (Pacific Daylight Time)
// ^ I asked for a date in UTC, not in PDT! RRule uses Luxon to generate the correct local time for all timezones in the world, not just your local timezone. This is why its usage of UTC is critical: if the timezone code returned "correct" dates for your local timezone, but you needed to generate dates in a different timezone (given with the What is concerning is the browser compatibility issue you bring up. I would ask you to now try your own sample code in a running Node console, or in Firefox for that matter. You'll see very different (and correct) results. So the real issue here is the results are confusing in Chrome. I think that's worth addressing, though it's not immediately clear to me exactly how to do so. What I can tell you is both environments generate the exact same ISO strings and timestamps. |
@davidgoli You're right, Firefox does show a different result in the console when running my original code. However, the result is ultimately the same time, it's just that the result is expressed in UTC time in Firefox whereas in Chrome it is expressed in
Let me attempt to explain why we do NOT want 19:00 UTC time for every recurring event. Let's say a user wants to schedule a repeating event at a specific location somewhere in the However, rrule is resolving the recurrences according to UTC time -- even when a timezone is passed in. By doing this, it is basically saying, "Oh, 1:00pm America/Denver evaluates to 19:00. Therefore, 19:00 must ALWAYS refer to 1:00pm in America/Denver, all year round." But that's just not true. Prior to Nov 4, 2018 that is true, but after Nov 4, America/Denver goes off daylight savings time, at which point, 19:00 UTC time now evaluates to 12:00pm America/Denver time. So our user is told, incorrectly, that the event is happening at 12pm, not 1pm. In other words, 1pm America/Denver can be either 19:00 or 20:00 UTC depending on whether we are on daylight savings time or not, and it's impossible to use rrule to get the correct recurrences because it wants to say that 19:00 is the correct time year round. I feel like rrule is buying into the myth most programmers believe, which is that UTC is always the correct solution when dealing with dates and times. But there are certain instances where it's not the best solution, and this instance (repeating future events in a specific location) is one of them. Edit: In theory, I am ok with rrule returning the times in UTC, but those times should be correct for the given timezone, when a timezone is passed in. That means we should see 19:00 UTC switch to 20:00 UTC at some point in the result set, for the example being discussed here. |
Lemme back up here - I think this is a documentation issue. RRule always returns JS "UTC" dates. This does not mean, however, that those dates are meant to represent UTC time. Rather, due to the limitations of the only alternative - local dates - the decision has been made that only JS dates with 0 timezone offset are usable for the date math this library is doing. This has the misleading side effect that it appears that these times actually represent the time in UTC, when in fact they are meant to be interpreted in the timezone represented by the This is clearer when you are using timezones outside of your local machine's zone. For example, if you set up your RRule with zone name JavaScript has no way to "pin" a date to a specific timezone, so we always represent the correct date and time no matter what, and you can discard the fact that the Date reports itself as UTC. When timezones are used in RRule, it is, in fact a local time. This is why Firefox gives you the correct local time as 19:00 (even though that date is reported as UTC, it's actually Denver time). Does this clarify things at all? I understand that it's confusing and not intuitive, but hopefully it's simple enough to grasp that once it's understood it becomes easy to reason about. We should definitely document this better. |
One thing I'd like to explore from this is the possibility of using Luxon to rezone the resulting dates into the local timezone, so it's at least consistent with your expectations. I think this shouldn't be too hard, and should help clarify misunderstandings such as this one. The Chrome issue is genuinely new to me, and it's bad news. This would be a breaking change, however, so I'd want to offer opportunity for community feedback before shipping. |
@shorlbeck This diff, I believe, implements the behavior you're expecting to see: diff --git a/src/datewithzone.ts b/src/datewithzone.ts
index 8ae3ed0..d9b917c 100644
--- a/src/datewithzone.ts
+++ b/src/datewithzone.ts
@@ -38,7 +38,10 @@ export class DateWithZone {
const rezoned = datetime.setZone(this.tzid!, { keepLocalTime: true })
- return rezoned.toJSDate()
+ return rezoned
+ .toUTC()
+ .setZone('local', { keepLocalTime: true })
+ .toJSDate()
} catch (e) {
if (e instanceof TypeError) {
console.error('Using TZID without Luxon available is unsupported. Returned times are in UTC, not the requested time zone')
diff --git a/test/rrule.test.ts b/test/rrule.test.ts
index 7774b8a..a794e02 100644
--- a/test/rrule.test.ts
+++ b/test/rrule.test.ts
@@ -3804,4 +3804,17 @@ describe('RRule', function () {
expect(() => rule.between(invalidDate, validDate)).to.throw('Invalid date passed in to RRule.between')
expect(() => rule.between(validDate, invalidDate)).to.throw('Invalid date passed in to RRule.between')
})
+
+ it('#300', () => {
+ const rule = RRule.fromString(
+ "DTSTART;TZID=America/Denver:20181101T190000;\n"
+ + "RRULE:FREQ=WEEKLY;BYDAY=MO,WE,TH;INTERVAL=1;COUNT=3"
+ )
+
+ expect(rule.all()).to.deep.equal([
+ DateTime.utc(2018, 11, 2, 1, 0, 0).toJSDate(),
+ DateTime.utc(2018, 11, 6, 2, 0, 0).toJSDate(),
+ DateTime.utc(2018, 11, 8, 2, 0, 0).toJSDate(),
+ ])
+ })
}) However, I'm not entirely convinced this is the way to go for this library. given the library's general approach of returning "floating" times that are always intended to be interpreted in the client's local timezone (albeit with 0 offset). The appropriate getter for the dates returned by I'm leaning toward thinking the correct fix here is not to change this library's behavior, but to document its somewhat idiosyncratic usage of pseudo-"UTC" (actually "floating") times more clearly. I'm open to persuasion on this, however. |
I've updated the README with some clarification and instructions for your use case. I'm interested in continuing this discussion, however, and open to possibly changing this behavior in a future version of the library. |
Thank you for the reply and the detailed explanation. Unfortunately, I have never been more confused. Working with dates and times is hard enough when those date times are either actually UTC or actually a local time, but having to also remember that sometimes a UTC time is not actually a UTC time is more than my brain can handle. I have spent all day today trying to understand what you wrote, but at the end of the day, it is so completely counterintuitive that I just can't wrap my mind around it. Whenever I look at the code and I see a "UTC" date I can't remember if it is actually UTC or just pseudo-UTC, especially when you add the Chrome issue in there. It's making it almost impossible for me to code. It's ruining my trust in the readability of my code. Here is my question, then... Going back to my original example... In true UTC, Is it pseudo-UTC going in as well as coming out? Or true UTC going in but pseudo-UTC coming out? And how can I ever reliably remember this?? |
Your rrule is specified as:
But according to the RRULE RFC5545, this indicates an rrule in the Denver timezone with local time of 1900: https://tools.ietf.org/html/rfc5545#section-3.3.5
If you want a DTSTART at 1300 local Denver time, you should specify it as 1300 in your rrule. The library should thoroughly conform to the spec in that regard. (Well, except for the minor fact that it will return 1300 UTC...) Additionally, be sure to read the section on "floating time":
JavaScript, unfortunately, does not offer an implementation of "floating time". The closest option we have to a "pure", timezone-less time is UTC, hence "psuedo-UTC". (For the record, The switch to UTC was fully begun with this commit: 850ed07 If you check out the previous commit, and cherry-pick the added test from #850ed075, you'll see it fail. In fact, hundreds of tests were failing unless the entire suite was run in UTC. That's because, in order to calculate future dates that lay across DST boundaries, we had to instantiate dates when the current clock was running in DST and then calculate math that put us into Standard Time, resulting in an incorrect 1-hour offset. The current implementation always gives correct local times. Now, the entire test suite will in fact pass in any local environment, with the caveat that to get the correct date/hour you must use the I'm investigating changing this in a future version, but the change is not trivial without adopting a required 3rd party library like Luxon. |
In an upcoming version I'd love to change the behavior to return dates that are always correct in your local timezone, so for example:
Is this the behavior you're expecting? |
One big problem is a given local JS environment can only represent dates in a single timezone. Thus, neither So what's the behavior supposed to be with: new RRule({
dtstart: new Date(2018, 10, 1, 10, 0, 0),
tzid: 'America/Denver'
}) if the local machine is in Pedantically, a An advantage to chucking the entire concept of local dates out the window is that you don't have to deal with this; the date and time represented can be treated as a floating time, capable of being projected into any timezone without doing any conversion. This is complicated, of course, by the behavior of engines like Chrome. |
For your purposes, you can probably get the values you want by doing: rule.all().map(d => new Date(
d.getUTCFullYear(),
d.getUTCMonth(),
d.getUTCDate(),
d.getUTCHours(),
d.getUTCMinutes(),
d.getUTCMilliseconds()
)) I'm presently trying to decide if this belongs in the library proper... |
Thank you! Your response really clarified things for me. I think I was tired and burnt out yesterday when I was trying to understand everything. I now see what is happening, and you're right, I was passing in the wrong time when using the
This worked for me! Thanks so much. It would be nice if perhaps there was a way to configure what is returned by UPDATE: The |
So, your solution works when my system's timezone is the same as the requested timezone, but I am having a horrible time trying to figure out how to get RRule to return the dates in the requested timezone when it differs from the system timezone. It always returns dates represented in the system timezone, instead of represented in the requested timezone. Example:
Result is correct (for what I expect/need):
Result differs, because it is represented in system timezone:
But I want the result to represent
But this returns the same thing while in New York time, because |
@shorlbeck If you notice, both the result in MDT and the EDT are the same in UTC, so my question is: are you sure you want to use the |
13:00 pseudo-UTC that is. If you want to get around the Chrome issue, you'll still have to convert to a local Date using the method in my prior comment. |
@davidgoli Oh wow, you're right. I didn't realize I don't actually need the tzid param after I understood how floating time/pseudo-UTC actually worked. And that means I don't need the I'd be lying if I said this wasn't the most confusing thing I've worked on in a long time, but I'm just glad to be headed in the right direction now. |
Have you considered using an explicit representation of what you need (floating dates without timezone)? Using native JS Date with UTC timezone and claiming it to be UTC in the docs is extremely confusing because it sort of works until it hits you with DST bugs. I would suggest an object, let's call it "RRuleDate" that takes a This RRuleDate is what |
I've been comparing the behavior of this library (v2.5.6) on the front-end to the equivalent PHP library (v2.3.3) on the back-end. There is a definite inconsistency with the way that the daylight savings change is handled, and I believe the PHP version handles it better.
Code Example:
In the
America/Denver
timezone, a daylight savings switch occurs on Sunday, Nov 4, 2018 (going from GMT-6 to GMT-7). So let's set up a recurring series starting at 1pm, repeating every Mon, Wed, Thu, starting on Nov 1 which is before the time switch:In this case I would expect all recurrences to start at 1:00pm as below. (As per the docs, I am using the Luxon library. I didn't see anything saying I had to initialize it somehow, so I am assuming it is "installed" correctly through yarn.)
Expected result (all starting at 13:00:00):
Actual result:
When I set up a similar situation in the PHP library, the result was as expected above, with all instances starting at 13:00:00 in the America/Denver timezone, not UTC.
Other details:
Edit: Fix typo
The text was updated successfully, but these errors were encountered: