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

Should use DTSTART as first occurrence #84

Open
espen opened this issue Jan 25, 2015 · 11 comments
Open

Should use DTSTART as first occurrence #84

espen opened this issue Jan 25, 2015 · 11 comments

Comments

@espen
Copy link
Collaborator

espen commented Jan 25, 2015

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.

@GeoffreyEmery
Copy link

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

@bsaverino
Copy link

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

@tilthouse
Copy link

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

@hwangmoretime
Copy link
Contributor

@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?

@jkbrzt
Copy link
Owner

jkbrzt commented Feb 24, 2016

@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.

@adamwong246
Copy link

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
Copy link

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.

@danydhondt
Copy link

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.

@arshaw
Copy link

arshaw commented Apr 1, 2019

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

@matwerber1
Copy link

matwerber1 commented Nov 24, 2020

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

@KrisLau
Copy link

KrisLau commented Mar 29, 2022

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

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