Doesn't seem to follow RFC 5545 with timezones

[tmshkr]

It seems that in the RFC 5545 spec, when the TZID parameter is provided, DTSTART should be specified in that timezone (i.e., not UTC, note that there is no Z at the end). For example:

      Monthly on the first Friday for 10 occurrences:

       DTSTART;TZID=America/New_York:19970905T090000
       RRULE:FREQ=MONTHLY;COUNT=10;BYDAY=1FR

       ==> (1997 9:00 AM EDT) September 5;October 3
           (1997 9:00 AM EST) November 7;December 5
           (1998 9:00 AM EST) January 2;February 6;March 6;April 3
           (1998 9:00 AM EDT) May 1;June 5

rrule seems to just pass the UTC time along, without converting it to the specified timezone:

const { RRule } = require("rrule");

const rule = new RRule({
  dtstart: new Date(Date.UTC(1997, 8, 5, 13, 0)), // 9/5/1997, 9:00:00 AM America/New_York
  freq: RRule.MONTHLY,
  count: 10,
  tzid: "America/New_York",
  byweekday: RRule.FR.nth(+1),
});

const string = rule.toString();
console.log(string);
/*
DTSTART;TZID=America/New_York:19970905T130000
RRULE:FREQ=MONTHLY;COUNT=10;BYDAY=+1FR
*/

[sunshineo]

In the README https://github.com/jakubroztocil/rrule#timezone-support

Optionally, it also supports use of the TZID parameter in the RFC when the Luxon library is provided. The specification and support matrix for Luxon apply.

So did you provide Luxon in your project?

[tmshkr]

@sunshineo including Luxon doesn't seem to make a difference for this.

See the following REPL:
https://replit.com/@tmshkr/rrule-issue-501#index.js

[sunshineo]

According to #427
You have to use 2.6.4 when using CommonJS
I think https://github.com/jakubroztocil/rrule/pull/410/files broke it when try to fix build issue for some other system but I'm not sure

[tmshkr]

Using 2.6.4 makes the Using TZID without Luxon available is unsupported. Returned times are in UTC, not the requested time zone error go away, but it doesn't resolve the issue.

The issue has to do with the "UTC" dates that are used to represent local time, as mentioned in the README:

The bottom line is the returned "UTC" dates are always meant to be interpreted as dates in your local timezone. This may mean you have to do additional conversion to get the "correct" local time with offset applied.

It's confusing to use dates in UTC format that aren't intended to represent UTC time. Ideally, dates in local time would be represented with their respective timezone.

[sunshineo]

So the project does not strictly follow the RFC 5545 spec. The README.md explained that it is a conscious choice because it's hard to deal with timezone in JS and don't want to introduce "large required 3rd party dependencies"

@tmshkr , are you suggesting to follow the spec by writing timezone support code without using a 3rd party lib?

[tmshkr]

@sunshineo it would probably help to add a required dependency, to deal with timezones and clear up any confusion.

Day.js has pretty good timezone support and is only 2kb.

[sunshineo]

I agree @tmshkr that it would be better if the timezone support is built in rather than the optional luxon lib which is 3.74 MB according to https://www.npmjs.com/package/luxon . The size could be why luxon wasn't baked in from the beginning

[sunshineo]

I would like to take a stab at this but given my past experience of trying to contribute to open source project but get rejected/dismissed and even worse ignored, I prefer the owner @jakubroztocil to give a green light first for this direction.

I see @KrisLau put this project on https://github.com/pickhardt/maintainers-wanted and I want to be a maintainer. I messaged @jakubroztocil on Twitter a few days ago but have not received any reply yet.

[davidgoli]

Hello @sunshineo, I've just taken a swing at this with #508. Please review and test, and if this goes well I think I can add you as a maintainer.

[davidgoli]

It's confusing to use dates in UTC format that aren't intended to represent UTC time. Ideally, dates in local time would be represented with their respective timezone.

Yes, 100% agree. Would it be better to not use date objects at all, but rather parseable date strings perhaps? At the end of the day, we have the limitation that JS Date objects cannot carry timezone information.

[tmshkr]

@davidgoli Day.js date objects seem to use a similar approach internally, with pseudo "UTC" dates, but they have the timezone attached to the object:

M {
  '$L': 'en',
  '$d': 1997-09-05T09:00:00.000Z,
  '$x': { '$timezone': 'America/New_York' },
  '$y': 1997,
  '$M': 8,
  '$D': 5,
  '$W': 5,
  '$H': 9,
  '$m': 0,
  '$s': 0,
  '$ms': 0,
  '$offset': -240,
  '$u': false
}

Day.js also has some useful methods for formatting display strings and converting back to actual UTC JavaScript Dates, which you can see in this REPL.

I've attempted to start integrating Day.js into #502 but it seems that much of the codebase depends on the pseudo "UTC" dates, so a lot of the tests are failing now.

[sunshineo]

Hi @tmshkr , you may take a look at @davidgoli 's PR at #508 which does not use Day.js and have all the tests still passing.

However after read RFC 5545 for like an hour, I believe even with @davidgoli 's change, we are only supporting 2 of 3 formats specified in the spec:
RRULE property is value type RECUR and it has a rule part (not 100% sure on the terminology) called "UNTIL" which is type date or date-time

Timezone only matters when the type is date-time and in the spec there are 3 allowed formats:

FORM #1: DATE WITH LOCAL TIME
FORM #2: DATE WITH UTC TIME
FORM #3: DATE WITH LOCAL TIME AND TIME ZONE REFERENCE

We are still not support FORM #1 , and for inputs of FORM #1 we simply treat it as FORM #2. The example for FORM #1 in the spec 19980118T230000 will simply be treated as 19980118T230000Z
And on the output side, we will never output FORM #1 and only output FORM #2 or FORM #3 depending on if TZID exists or not.

We can try to support FORM #1 in a separate effort (I can try to take a stab). In the mean time, we can improve this section of README.md to explain a little more clearly. I think what I wrote above is a little more clear than what we have now.

[sunshineo]

I realized there are inaccuracy in my previous comments right after I post it of course. Yes there are 3 formats and we support only 2. I verified using the DTSTART property, but the spec has a much more complicated logic for the UNTIL

It seems UNTIL will be in either FORM #1 or FORM #2 when DTSTART is present. It does not say what if DTSTART is not present. And DTSTART is optional

[davidgoli]

I'm not too concerned with incomplete spec implementation at this point, but I would be alarmed about incorrect spec implementation. Still, the pseudo-UTC dates are fully convertible into local datetimes using the instructions in the README. It is necessary when using this library to ignore any JS Date builtin "timezone" concept, including UTC; they are not useful for you. Instead, you can use a 3rd party library (like day.js or luxon) to convert the output of RRule to the expected local timezone. If you follow those instructions, you should receive the correct dates. However, I believe it would be less correct for Rrule to return local Date objects. Rrule returns year, month, day, hour, minute, second and timezone - and all of these should be correct in combination; the "timezone" in the JS Date object does not carry necessary information here.

[davidgoli]

An alternative I might propose is ditching the JS builtin Date altogether in favor of a custom DateTime object for input and output. This could be done without changing the internal implementation of recurrence calculation.

[sunshineo]

@davidgoli I agree with your on the severity of incomplete vs incorrect. But would you call the library's current behavior of treating FORM #1 input as FORM #2 incomplete or incorrect ?
For example right now the code below is working

const rset = rrulestr('DTSTART:20120201T023000\nRRULE:FREQ=MONTHLY;COUNT=5\nRDATE:20120701T023000,20120702T023000\nEXRULE:FREQ=MONTHLY;COUNT=2\nEXDATE:20120601T023000')
console.log(rset.toString())
// DTSTART:20120201T023000Z
// RRULE:FREQ=MONTHLY;COUNT=5
// EXRULE:FREQ=MONTHLY;COUNT=2
// RDATE:20120701T023000Z,20120702T023000Z
// EXDATE:20120601T023000Z

If we actually throw error because DTSTART:20120201T023000 does not have Z at the end, it is clearly a case of incomplete , as we do not support FORM #1 in the spec. But right now we take it and output UTC time, which feels more like incorrect to me.
And the suggested conversion process in README.md probably has been confusing to library users because they might have been converting the output time from UTC to their local timezone, but they actually should drop the Z at the end, take the year month day time info and attach the local timezone.

But I don't think we should start rejecting input like DTSTART:20120201T023000 now. We should move towards following the spec more completely.

==Edit==
JS builtin Date does not have timezone and is always UTC. To support Form #2 and Form #3, we already created a class DateWithZone. It has optional tzid for distinguish Form #2 and Form #3. Should we simply edit that class to support Form #1? Like add a boolean localTime and if true, strip the Z when produce output?

[juni0r]

I'm sure this library can do a lot of things. Creating date recurrences that respect timezones and daylight saving times unfortunately isn't one of them. If you want to do this, you've picked the wrong tool.

Anyone who ends up here, might want to check out rschedule. It comes with adapters for many popular date libraries, such as moment.js and Luxon, as well as vanilla JS Date. It doesn't see much activity and documentation isn't exactly exhaustive, but from the few usage examples and the codesandbox one will quickly figure out how to use it. It sports a modular approach and pluggable features, such as JSON and iCal support.

Hope this helps.