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

Time Zone Support #38

Closed
joeydong opened this issue Dec 12, 2013 · 57 comments
Closed

Time Zone Support #38

joeydong opened this issue Dec 12, 2013 · 57 comments

Comments

@joeydong
Copy link

Is there a way to get a .between for an RRULE that is in a time zone other than the browser's current time zone? could the library make use of timezone-js to implement this functionality?

For example, if I'm in America/New_York but I have a recurrence that should be interpreted in Europe/Paris, the resulting dates should account for the daylight savings changes in the Europe/Paris time zone.

Edit: Digging deeper, would a change to

tzOffset: function(date) {
  return date.getTimezoneOffset() * 60 * 1000
}

fix this issue?

@jkbrzt
Copy link
Owner

jkbrzt commented Jan 18, 2014

Hi @joeydong, this is unfortunately a limitation of the native JavaScript Date object. I believe switching to timezoneJS.Date would allow different timezones. I will see how difficult that conversion would be.

(duplicate: #36)

@joeydong
Copy link
Author

Thanks for getting back to me. I was looking into the timezone-js Date implementation as well. The "drop in" replacement should work as long as the javascript date api methods you use are fully re-implemented in that library.

@jkbrzt
Copy link
Owner

jkbrzt commented Jan 20, 2014

That is my understanding too. In addition to that, it will also require some changes to the RRule API. Also, timezone-js should Ideally be an optional, soft dependency.

@chotiwat
Copy link

Hello, are any of you guys working on this one? If not, maybe I could submit a pull request for you to review.

@joeydong
Copy link
Author

No, I haven't made any progress, I would be happy to see what you've come up with.

@jkbrzt
Copy link
Owner

jkbrzt commented Feb 15, 2014

@chotiwat That would be awesome! A couple of pointers:

  • The original python-dateutil library provides timezone support, so it would be good to adopt a similar API for timezone handling.
  • Testing date-related functionality can be tricky. Our test suite is mostly a port of the very comprehensive one from python-dateutil, but everything timezone-related has been removed. So an easy way to go about testing the new timezone stuff would be to port also the timezone-related tests from the Python version.
  • Timezone-js should be an optional, soft dependency: When not installed, RRule should behave as it does now, plus it should throw an error when a timezone-related method is called.

This will be quite a big change, please feel free to open a pull request early for feedback 👍

@chotiwat
Copy link

OK, here it is: wavify/rrule@0f7c812.

Adhering to the implementation of python's dateutil, I use the timezone information from the dtstart options if it is an instance of timezoneJS.Date. I also added some tests regarding this feature.

There are still some issues though:

  • Converting rrule option to string with timezone data. (Currently, I include the timezone data as TZID=timezone_name;)
  • If timezone-js is present, the resulting date instances will be of timezoneJS.Date class regardless of the dtstart's class.
  • I tried to make timezone-js a soft dependency, but the code gets messy, as you can see. This implementation also requires timezone-js to be initialized before use.

Any suggestions?

@jkbrzt
Copy link
Owner

jkbrzt commented Feb 15, 2014

Great job! Would you mind opening a pull request? It's easier to go through the changes and provide feedback.

@pierre-elie
Copy link

FTR I tried @chotiwat branch with timezoneJS and it solved issues I was having with wrong occurrences because of different timezones. I didn’t test it thoroughly though.

@jgornick
Copy link

Would it make sense to update rrule to always return UTC dates and allow the consumer to then convert to their timezone?

Another project like later.js (https://github.com/bunkat/later) returns dates as UTC by default.

@jsilvestre
Copy link

I 👍 @jgornick , this is especially weird when you expect the date not to change. I think it's not rrule's job to manage timezones.

I suggest we replace all the new Date(.... to Date.UTC(..... What do you think?

@jkbrzt
Copy link
Owner

jkbrzt commented Jun 1, 2014

@jgornick @jsilvestre 👍

@chotiwat
Copy link

chotiwat commented Jun 1, 2014

How should the consumer handle the resulting UTC dates?

If they use timezone offset to convert to their timezone, they would have a problem with recurrence rules like "every 9 a.m., London time", since UTC time doesn't account for daylight saving time.

On the other hand, if they create dates according to their timezone using information from .getDay(), .getHour(), etc., they would have a problem with recurrence rules like "every 3 hours, London time" when the time span they are querying contains daylight saving time changes,
e.g. 9 p.m. 0 a.m. (DST starts) 4 a.m. 7 a.m. ... (months later) ... 10 p.m. 1 a.m. (DST ends) 3 a.m. 6 a.m.

@jsilvestre
Copy link

The consumer would use other librairies like node-time, moment, whatever they like to push a date in UTC to rrule and convert them back afterwards.
They can translate "every 9 a.m. London time" to "every 8 a.m. UTC time" (tell me if I'm wrong, timezones are tricky and I may miss something). For that very specific point (timezone in recurrence rules), rrule could provide helpers/automation.
My point is that default should be UTC and more generally dates should be processed as UTC.

@jgornick
Copy link

@jsilvestre No, you're right.

@jakubroztocil Any ideas how much effort/time a change like this may take?

@pierre-elie
Copy link

If rrule only manipulates dates in UTC, what happens with DST changes?

Example:

  1. Client has a recurrence every day at 9am London time, beginning in February 1st, 2015
  2. In February 2015, Europe/London is UTC+0:00
  3. Client converts 2015-02-01 9am Europe/London to 2015-02-01 9am UTC and gives this to rrule
  4. rrule computes occurrences in UTC, so every day 9am UTC
  5. Client asks for the occurrence on April 1st 2015
  6. rrule returns 2015-04-01 9am UTC, which makes total sense regarding rrule
  7. client converts that back to London time, so 2015-04-01 10am Europe/London, because London is in BST (UTC+1:00) in April 2015
  8. the correct occurrence should be 2015-04-01 9am Europe/London

@celalo
Copy link

celalo commented Jun 27, 2014

Also how will be days of week calculated? Let's say someone in Pacific time needs a recurring schedule for Fridays 8PM. When converted to UTC it will be 3AM (Saturday) http://time.is/compare/UTC/PT. In that case, you will need to set a recurring email for UTC 3AM Saturdays, it will be tricky.

@jgornick
Copy link

@pierre-elie Good points.

I can see the benefits to providing a timezone in the constructor of RRule and building the dates off that specified timezone.

A small test I was using to reproduce @pierre-elie scenario: http://jsfiddle.net/jgornick/LXG77/

@jsilvestre
Copy link

I forgot to keep up with this issue. Indeed, good points @pierre-elie, DST are an issue with the suggestion of returning UTC by default. I believe there is no other choice then to add support for timezone in rrule, except if someone has a better idea ;-)

@jgornick
Copy link

jgornick commented Sep 2, 2014

@jsilvestre I agree that it needs to be built-in.

However, I would approach it with being able to write adapters to handle the timezone support. For example, I already use Moment Timezone for all of my timezone conversions. It would be nice if I could enable the adapter in rrule.js to use Moment Timezone. As other comments have suggested using timezone-js, there could be an adapter for that as well.

Thoughts?

@mattcasey
Copy link

I use moment-timezone, but I liked how simple @chotiwat's fork was, so I used his work as inspiration and made the Date class used by rrule overridable. I then wrote my own 'moment plugin' which is in the comments of my pull request.

#78

@FrEaKmAn
Copy link

any update about this?

@visionscaper
Copy link

Very interested as well!

@aramk aramk mentioned this issue May 3, 2015
@aramk
Copy link
Contributor

aramk commented May 3, 2015

I've added #97 to add support for time zones. It needed some tinkering across the entire library but I've added tests and think it's a pretty decent solution. I'll be using it in production shortly.

@Yatufo
Copy link

Yatufo commented Jan 26, 2016

I used the workaround using http://momentjs.com/timezone/
var options = RRule.parseString(RULE_STRING_WITH_DT_START)
options.dtstart = moment.tz(ORIGINAL_DATE, TIME_ZONE).toDate();
var rule = new RRule(options)
Cheers

@davidgoli
Copy link
Collaborator

@jakubroztocil any reason tzinfos and ignoretz are collected (and documented) but not used? Seems like line 627 is the spot for it:

            // python:
            // datetime.time(hour, minute, second,
            // tzinfo=self._tzinfo))
            this.timeset.push(new dateutil.Time(hour, minute, second, millisecondModulo))

@davidgoli
Copy link
Collaborator

Also seems like implementing python's more recent tests might be a good first approach: https://github.com/dateutil/dateutil/blob/97dd3697df28bc50baaa2a098124a7428218f74e/dateutil/test/test_rrule.py#L4548

@davidgoli
Copy link
Collaborator

davidgoli commented Jul 24, 2018

as it turns out, by not running in a UTC environment (eg by running npm test without prepending TZ=UTC, existing tests fail. Fixing them should already get the implementation pretty far along.

@jorroll
Copy link

jorroll commented Jul 24, 2018

@davidgoli fyi, this library is not actively maintained at the moment

@davidgoli
Copy link
Collaborator

@thefliik that certainly seems to be the case! but there's no indication on the README that this is so. Are you a maintainer?

@davidgoli
Copy link
Collaborator

related: do you know of any alternatives that are maintained that implement the RFC?

@jorroll
Copy link

jorroll commented Jul 24, 2018

@davidgoli I'm not a maintainer. I believe github always adds a tag next to someone's name if they've contributed to a project (or is officially involved with it).

And I've looked for alternatives and I'm fairly confident in saying this is still the best opensource javascript based recurrence library.

@davidgoli
Copy link
Collaborator

@jakubroztocil @arolson101 care to weigh in? Is this library officially abandoned, or are there plans to take submissions from the community?

@jorroll
Copy link

jorroll commented Jul 24, 2018

This library is not officially abandoned, and I didn't mean to imply that it was. This library is not actively maintained. I imagine at some point in the future, PR's will be accepted again. I just wanted to chime in because it's quite likely you're specific issue will not be fixed in a timely manner. If that's a dealbreaker for you, you should look elsewhere. Of course, one of the maintainers is free to correct me if I'm wrong.

@davidgoli
Copy link
Collaborator

That's good to know, the question we're struggling with is not whether to adopt this library (our sunk costs are already too great for that), but rather whether to fork and patch or to try and get the fix merged. We'd rather not maintain our own fork if we can help it.

@jorroll
Copy link

jorroll commented Jul 24, 2018

Having watched this library for a while now. (Over a year? Maybe) I'm pretty confident in saying you should fork and patch. Definitely look at some of the forks others have already made. And, obviously, a maintainer may correct me, but just taking a look at the commit history can give you a sense of activity on the repo.

@davidgoli
Copy link
Collaborator

That's fair - combined with the fact that this issue is nearing 5 years old at this point. It's a bit surprising, I would think there would be bigger stakeholders willing to either implement their own rrule library or support one like this. I wonder if it's just the case that companies try to avoid building calendar-type apps? or are there more popular alternatives nowadays?

@jorroll
Copy link

jorroll commented Jul 24, 2018

Well there are a number of server side options available, so that might lesson the need for a javascript based one. Obviously there is the python library this packaged is based off of. Ruby has a good one called ice_cube. I'm sure there are others.

My guess is that there just isn't a whole lot of need for a super power recurrence library, and for those folks that do need it, it's probably pretty central to their business model and they haven't committed to open sourcing it.

Just for curiosities sake, while the library is owned by @jakubroztocil, it has multiple collaborators @arolson101, @espen, @Zizzamia, @http-teapot. I don't think anyone has the time to actively maintain it though, so most questions / PR's will go unanswered for a while. It definitely receives occasional attention, but I just wanted to help set your expectations.

(Also note, I'm not faulting anyone involved with this library, I 100% understand and appreciate all that has already been done, especially by @jakubroztocil).

@davidgoli
Copy link
Collaborator

@thefliik Thanks for all the context. Granted, this rrule library could work if we committed to only using it server-side since we could run it under UTC and all would be well. However, we're trying to use it client-side for performance reasons (a user's list of events is orders of magnitude smaller and more cacheable compared to the number of recurrences), so we're hoping a patch might be considered. It looks like there was activity here as recently as February, so while we're not expecting a quick turnaround, we're holding out hope there might be a possibility of a merge within weeks or maybe a few months.

@jgornick
Copy link

It looks like there are other forks that added TZ support too but just haven't contributed back.

Is the issue that this project is not active and there isn't a place for this TZ work to land?

Does it make sense then to ask @jakubroztocil for others to become maintainers or to have an official fork?

@jorroll
Copy link

jorroll commented Jul 24, 2018

@jgornick you might want to read through the full conversation in issue #160. As I just mentioned, this library already has ~5 maintainers I believe (much more than most libraries I've used with this many stars). The issue is that adding TZ support is a large update and no one has the time to deal with it this year. Or last year. I also don't think there are any forks that have a complete TZ implementation (though feel free to prove me wrong). @jakubroztocil has made clear he doesn't have time for this library at the moment (which again, is completely fair).

@jamesdixon
Copy link

@davidgoli we are using this library client side for the same reasons you mention. I can get you details but I believe we basically strip off the Z on everything and deal with it in local time. Let me know if you need details and I'll see what I can do

@jamesdixon
Copy link

@davidgoli are you using Ember?

@davidgoli
Copy link
Collaborator

@jamesdixon Current project is using React Native. For our purposes, we'd like to deal with everything in UTC "zoneless" time and simply present it in whatever timezone the user happens to be in currently. The issue we're seeing in particular is that because this library uses JS Date, which in turn has the runtime timezone baked in, we get daylight savings time applied around the local DST date boundary even though UTC doesn't have DST. So for example, an event that repeats at 7PM every day suddenly occurs at 6PM the day after daylight savings ends, even though it was stored in dtstart as UTC (with Z suffix).

I suppose a broader issue is whether it makes sense to use JS Date at all, given its limitations, or whether to provide the option to "hot swap" a library like Luxon (the successor to Moment, with robust timezone support) as @aramk 's (3 year old) fork does. Or if taking a 3rd party dependency is not an option, attempting to implement a proper "aware" datetime abstraction like the original Python RRULE is built on.

An even broader one is why did we choose to build a calendar in the first place, but it's too late for that, here we are. I can fully understand why devoting one's time and resources to this library long-term may not make the most sense without funding to support, but I'm willing to take a crack at it as long as I can get some pointers on what some acceptance criteria might be.

@aramk
Copy link
Contributor

aramk commented Jul 24, 2018

For what it's worth, I'm still using my forked version with the TimeZoneDate class in production since I created the PR. From what I can tell this library doesn't merge PRs often. #97

@davidgoli
Copy link
Collaborator

A simpler approach that might serve a common range of needs without needing robust, universal timezone support would be to provide an option to support either UTC or local. That would probably be enough for 90% of use cases requiring timezone support.

@davidgoli
Copy link
Collaborator

@aramk have you tried merging upstream into your fork lately? I took a look at it today and it seems complicated...

@aramk
Copy link
Contributor

aramk commented Jul 24, 2018

Yeah, I haven't bothered to try. I'm guessing it might be easier to re-apply my fixes on top with a new branch, but I'm not sure what fixes I'd be using from the latest version to warrant that.

@davidgoli
Copy link
Collaborator

Published in 2.4.0.

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