rrule icon indicating copy to clipboard operation
rrule copied to clipboard

Should use DTSTART as first occurrence

Open espen opened this issue 10 years ago • 11 comments

According to the iCalendar specification:

 The "DTSTART" property defines the first instance in the recurrence set.

And it includes an example of this:

 Every other week on Monday, Wednesday and Friday until December 24,
 1997, but starting on Tuesday, September 2, 1997:

 DTSTART;TZID=US-Eastern:19970902T090000
 RRULE:FREQ=WEEKLY;INTERVAL=2;UNTIL=19971224T000000Z;WKST=SU;
  BYDAY=MO,WE,FR
 ==> (1997 9:00 AM EDT)September 2,3,5,15,17,19,29;October
 1,3,13,15,17
     (1997 9:00 AM EST)October 27,29,31;November 10,12,14,24,26,28;
                       December 8,10,12,22

rrule.js doesn't include September 2nd as an occurrence. In rrule.js the first occurrence is September 3rd.

espen avatar Jan 25 '15 15:01 espen

Plus 1 on this. RRule isn't honoring the startdate/duration on the demo app at all

GeoffreyEmery avatar Feb 19 '15 20:02 GeoffreyEmery

Agreed, this threw me off when dealing with Google RFC 2445 java lib.

bsaverino avatar May 13 '15 21:05 bsaverino

:+1: This is how the standard (and compliant systems like Google Calendar, Apple (Mac OS/iOS/iCloud) Calendar works.

tilthouse avatar Nov 19 '15 18:11 tilthouse

@jkbrzt, Right now, rrule.js is tied to python dateutil's functionality

Unlike documented in the RFC, the starting datetime (dtstart) is not the first recurrence instance, unless it does fit in the specified rules. ––dateutil docs

This would seem like a large departure from dateutil, which would seem to make sense in this case. It would involve changing almost all of the test cases. Thoughts? Would you support the change?

hwangmoretime avatar Feb 22 '16 22:02 hwangmoretime

@hwangmoretime I've merged your changes to the README documenting where rrule.js differs from the RFC, thanks for that. Yeah, It would make a lot of sense to make rrule.js compliant (this issue + the byday keyword argument). It will break backward-compatibility though so it needs to be well-documented.

jkbrzt avatar Feb 24 '16 06:02 jkbrzt

I believe this still has a bug. This works for all() but not for the method between. In the example below, I set dtstart and rdate on a RRuleSet object, as per the readme.

screen shot 2016-03-19 at 1 41 49 pm

rruleSet RRuleSet {_cache: Object, _rrule: Array[1], _rdate: Array[1], _exrule: Array[0], _exdate: Array[0]}

rruleSet.valueOf() ["RRULE:FREQ=YEARLY;DTSTART=20120122T000000Z;INTERVAL=1;WKST=0;BYMONTH=3;BYMONTHDAY=19;BYHOUR=13;BYMINUTE=39;BYSECOND=27", "RDATE:20120122T000000Z"]

rruleSet.all()[0] Sat Jan 21 2012 16:00:00 GMT-0800 (PST)

rruleSet.between(startDatetime, endDatetime) [Sat Mar 19 2016 13:39:27 GMT-0700 (PDT)]

note that for the call to method all, the returned date matches the original dtstart but for between, it defaults back to the current time

adamwong246 avatar Mar 19 '16 20:03 adamwong246

here's a bit more context. As far as I can tell, the rruleset is doing precisely the opposite of what I expected. It produces a list of dates, where the zeroth element is the start of the original desired event. But it's the current time the gets expanded into a series of dates. I expected to get a list of dates on Jan 21st, but rruleset "recurred" using the current time (march 22).

screen shot 2016-03-22 at 6 02 21 pm

I've pulled the repo and am trying to narrow this down with some tests but any advice is appreciated.

adamwong246 avatar Mar 23 '16 01:03 adamwong246

I had a similar problem with dtstart. See https://stackoverflow.com/questions/54517101/rrule-not-setting-correct-time-if-dtstart-is-set When I set dtstart in the constructor, I get correct times with .all() but if I set dtstart afterwards, I get the current time.

danydhondt avatar Feb 04 '19 16:02 danydhondt

I agree, by default the dtstart should be inclusive to be more consistent with other systems.

I made a codepen demonstrating the subtleties of the current behavior: https://codepen.io/arshaw/pen/qwEQNO?editors=0010

arshaw avatar Apr 01 '19 23:04 arshaw

by default the dtstart should be inclusive to be more consistent with other systems.

Agreed. Getting up to speed on rrule now. It's a great timesaver (thank you!), but this inclusive date issue an odd nuance that requires some custom logic to address. Would love to see this updated. Understood that current behavior consistent with python-dateutil, but I think priority should be on behavior consistent with the RFC standard, rather than perpetuating an odd deviation.

Also, per comment below:

Note that you can get the original behavior by using a RRuleSet and adding the dtstart as an rdate.

As best I can tell, this significantly understates the complexity of the workaround needed for many use cases.

For example, assume you have the following rule parameters:

new RRule({
  freq: RRule.WEEKLY,
  dtstart: new Date(Date.UTC(2020, 10, 1, 21, 0, 0)),
  count: 10,
  interval: 2,
  byweekday: RRule.MO
})

This should translate to "every 2 weeks on Monday for 10 times", with a start date of Sunday, November 1st, 2020.

Now, the very first calendar Monday after the start date is November 2nd... so, with an interval of 2, we would expect generated event dates to be Nov 2, Nov 16, Nov 30, etc.

However, the rrule package instead gives us dates of Nov 9, Nov 23, etc. In other words, our entire interval is wrong, shifted one week forward. Fixing this is not as simple as "using a RRuleSet and adding the dtstart as an rdate." There's quite a bit more work to this type of fix.

image

matwerber1 avatar Nov 24 '20 17:11 matwerber1

I personally like the way this package deals with it but the difference in behaviors is confusing when considering the way the actual protocol and calendars deal with DTSTART. Because when I generate an ical using the rrule gnerated here the calendar produces the extra first occurrence.

My two cents is that the package should include the DTSTART by default and pass a bool in the .all that determines if it inclusive or not. Example: rule.all(true) would be the current behaviour and rule.all() would follow the protocol and be inclusive

Side note: The package is looking for active maintainers if anyone is interested #450

KrisLau avatar Mar 29 '22 21:03 KrisLau