jens-maus / node-ical

NodeJS class for parsing iCalendar/ICS files
Apache License 2.0
118 stars 50 forks source link

Fix timezone issues when using rrule #329

Closed 50an6xy06r6n closed 1 week ago

50an6xy06r6n commented 1 month ago

An alternative implementation of #231. All tests seem to pass, including the ones added in #231.

Implementation is simpler, so hopefully this should be easier to review.

50an6xy06r6n commented 4 weeks ago

@Apollon77 any chance you could take another look?

Apollon77 commented 4 weeks ago

I honestly did not really looked into the change itself, but a test it´s there which is great ... And I mainly stumbled over the process.on(...) ... sooo LGTM from my side - but as said without looking too deep into the change itself and potential implications

jens-maus commented 4 weeks ago

@50an6xy06r6n Thanks for this PR. Please note, however, the failing CI checks for the windows builds. It seems some tests actually fail on the windows build of node-ical. So please correct.

jens-maus commented 4 weeks ago

@50an6xy06r6n In addition, please comment how much different this PR is compared to #231

50an6xy06r6n commented 3 weeks ago

@jens-maus Ah I think the Windows CI failures are an issue with the new tests, and have to do with this:

Moment Timezone found Etc/Unknown from the Intl api, but did not have that data loaded.

Pretty sure updating the moment-timezone import to moment-timezone-with-data will fix this, but I'll try it out. Hopefully the CI will re-run, since last time it was blocked on approval.

I think the biggest difference between this and #231 is that this is a more targeted change for the RRULE issue. The other PR I think also changes some other, unrelated behavior. I think our solutions to the RRULE issue are fundamentally the same, though we use different mechanisms for getting the local time. I copied the tests from that PR, so the behavior seems largely the same (minus the additional parsing behavior for "(GMT +XX:X0)" time zones)

The other difference is that I'm updating this PR, and that one hasn't been touched in almost 2 years 😛

jens-maus commented 3 weeks ago

@jens-maus Ah I think the Windows CI failures are an issue with the new tests, and have to do with this:

Moment Timezone found Etc/Unknown from the Intl api, but did not have that data loaded.

Pretty sure updating the moment-timezone import to moment-timezone-with-data will fix this, but I'll try it out. Hopefully the CI will re-run, since last time it was blocked on approval.

No problem. I can then start the CI check if it is still rejected for you. So please continue to fix this PR in getting the windows CI build running. However, please make sure that moment-timezone-with-data is really required since it is substantially larger than the normal moment-timezone and would substantially increase the overall size of code required when using node-ical.

50an6xy06r6n commented 2 weeks ago

@jens-maus ready for the CI checks to be re-run

jens-maus commented 2 weeks ago

@jens-maus ready for the CI checks to be re-run

Thanks for your info. However, the unit tests still fail on windows. See:

    with test23.ics (testing dtstart of rrule with timezones) recurring yearly first event (14 july) 
      ✗ dt start well set 
        » An unexpected error was caught: RangeError: Invalid time zone specified: Etc/Unknown 
jens-maus commented 1 week ago

@50an6xy06r6n it seems rrule.between is somewhat broken on windows. Similar things are reported for rrule.after, see https://github.com/jkbrzt/rrule/issues/608. Thus changed the test cases to somewhat skip on windows until rrule is fixed.

50an6xy06r6n commented 1 week ago

Oh nice, thanks for investigating and fixing it!