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

Support use of timezones specified in ICS #579

Merged
merged 1 commit into from
Mar 13, 2023

Conversation

leftmostcat
Copy link
Collaborator

There is currently no support in ical.js for using a timezone defined within an ICS file for datetimes within that same ICS file. Instead, we are reliant on the TZID used in the file matching one in our time zone database.

In order to prevent bloating the timezone service with one-off time zones or using conflicting definitions, this patch restricts time zones to a single component tree, storing them on the root component and retrieving them from there when referenced.

@coveralls
Copy link

coveralls commented Mar 7, 2023

Pull Request Test Coverage Report for Build 4377790006

  • 83 of 87 (95.4%) changed or added relevant lines in 2 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.06%) to 98.074%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/ical/component.js 56 60 93.33%
Files with Coverage Reduction New Missed Lines %
lib/ical/timezone_service.js 4 96.67%
Totals Coverage Status
Change from base Build 3275454147: -0.06%
Covered Lines: 9271
Relevant Lines: 9438

💛 - Coveralls

Copy link
Owner

@kewisch kewisch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, with one comment. I imagine that in a large file we may have quite a few timezone lookups, caching on the root element avoids reinstanciating the Timezone each time.

lib/ical/time.js Outdated Show resolved Hide resolved
@leftmostcat leftmostcat merged commit ac6e512 into kewisch:main Mar 13, 2023
@leftmostcat leftmostcat deleted the support-embedded-timezones branch March 13, 2023 21:16
if (prop.parent.name === 'standard' || prop.parent.name === 'daylight') {
// Per RFC 5545 3.8.2.4 and 3.8.2.2, start/end date-times within
// these components MUST be specified in local time.
zone = Timezone.floating;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is Timezone.floating defined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be an error on my part; it should probably be Timezone.localTimezone.

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

Successfully merging this pull request may close these issues.

4 participants