refactor: leverage Temporal across the codebase - simplify timezone internals#462
Merged
jens-maus merged 8 commits intojens-maus:masterfrom Feb 21, 2026
Merged
Conversation
parseDateTimeInZone() previously used a manual fixed-point iteration
(convergeLocalInstant) plus a helper (ymdhmsToUtcMs) to work around the
fact that JS Date has no "wall-clock → UTC" API. The algorithm ran up to
3 rounds of Intl formatting and subtraction to converge on the correct
UTC instant, with special-case handling for DST gaps/folds.
Replace the entire mechanism with a single Temporal call:
Temporal.PlainDateTime.from(fields)
.toZonedDateTime(tz, { disambiguation: 'later' })
.epochMilliseconds
Temporal handles DST disambiguation natively. disambiguation:'later' is
used to preserve the existing fold behaviour (post-DST / second
occurrence for repeated hours).
Also require temporal-polyfill directly in both ical.js and tz-utils.js
instead of going through the @js-temporal/polyfill alias. Add
temporal-polyfill as an explicit dependency in package.json (the alias
stays for rrule-temporal's transitional require).
Removed: ymdhmsToUtcMs (~20 lines), convergeLocalInstant (~35 lines),
getLocalParts closure in parseDateTimeInZone (~30 lines).
…tDateKey
The full-day EXDATE exclusion loop in processRecurringInstance used a
local Intl.DateTimeFormat('en-CA') call to extract the calendar date in
the event's timezone. This duplicated the logic already present in
getDateKey() (ical.js), which was introduced to fix issue jens-maus#459.
Replace the inline Intl formatter with a call to getDateKey(exdateValue),
exported from ical.js. The two functions now share a single source of
truth for timezone-aware date key extraction.
generateDateKey() is kept separate: it serves RRULE-generated Date objects
that carry no .tz/.dateOnly metadata, requiring an explicit isFullDay flag.
Temporal.Instant.toZonedDateTimeISO() handles both IANA zones and
fixed-offset strings natively, eliminating two separate code paths
and the Intl formatter cache (dtfCache / getFormatter).
The normalizeMidnightParts workaround for the Node <22 Intl bug
("24:00" at midnight) is no longer needed and is removed together
with its __test__ export and unit test.
…DTSTART construction with Temporal Bug: getTimezoneOffset()-based compensation shifted VALUE=DATE events by the server's UTC offset on machines east of UTC (e.g. 02:00:00 on UTC+2). Silent on UTC servers, so never caught in CI. Fix: extract local date components directly, consistent with how dateParameter creates DATE-only Dates. Refactor (no behavior change): replace Intl.DateTimeFormat formatToParts block for DATE-TIME RRULE DTSTART with Temporal.Instant.toZonedDateTimeISO(), matching the existing pattern in getDateKey(). Update example.txt snapshot: "02:00:00" for "foobarTV broadcast starts" was the bug manifesting on a UTC+2 machine. Add regression test: fails with old code on UTC+x, was silent on UTC servers.
📝 WalkthroughWalkthroughReplace Intl-based timezone/date logic with Temporal (via temporal-polyfill); extract getDateKey into Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as ICal Parser (ical.js)
participant TZ as TZ Utils (tz-utils.js)
participant Utils as Date Utils (lib/date-utils.js)
participant RR as RRULE Engine (node-ical)
participant Store as EXDATE Store
Parser->>TZ: resolve TZID / convert DTSTART using Temporal
TZ-->>Parser: Date (+tz metadata) or local-midnight for DATE
Parser->>RR: pass normalized DTSTART and RRULE
RR->>Utils: generateDateKey(instanceDate, isFullDay?)
Utils-->>RR: return YYYY-MM-DD or full ISO key
RR->>Store: compare instance key vs EXDATE keys
alt match
Store-->>RR: exclude instance
else
Store-->>RR: include instance
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ical.js (1)
1153-1154: Consider addinggetDateKeyto the mainmodule.exportsobject instead of appending separately.The main export block is at line 592. Appending
module.exports.getDateKey = getDateKeyat the bottom works but is inconsistent with how all other exports are declared in the object literal. Moving it into the object improves discoverability.Suggested change
Add
getDateKeyinside themodule.exports = { ... }block at line 592:module.exports = { objectHandlers: { // ... }, handleObject(name, value, parameters, ctx, stack, line) { ... }, parseLines(lines, batchSize = 0, ctx, stack, startIndex = 0, cb) { ... }, parseICS(string, cb) { ... }, + getDateKey, }; - -module.exports.getDateKey = getDateKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ical.js` around lines 1153 - 1154, The export for getDateKey is appended separately; move it into the main module.exports object so exports are consistent and discoverable: locate the module.exports = { ... } object and add getDateKey as a property (e.g., getDateKey: getDateKey) alongside the other exports, then remove the separate module.exports.getDateKey = getDateKey line at the bottom; ensure the symbol getDateKey is referenced exactly as the function name and that module.exports remains a single object literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ical.js`:
- Around line 1153-1154: The export for getDateKey is appended separately; move
it into the main module.exports object so exports are consistent and
discoverable: locate the module.exports = { ... } object and add getDateKey as a
property (e.g., getDateKey: getDateKey) alongside the other exports, then remove
the separate module.exports.getDateKey = getDateKey line at the bottom; ensure
the symbol getDateKey is referenced exactly as the function name and that
module.exports remains a single object literal.
Contributor
Author
|
The nitpick comment sounds legit. I'll look into it ASAP 🙂 |
Owner
|
Thanks again @KristjanESPERANTO for this (once again) great PR! I really appreciate all your continous efforts in getting node-ical more and more complete and stabilized and it improved considerably since you went onboard the maintainers! Regarding this PR, it sounds pretty reasonable to me and quickly looking over the proposed changes makes me feel that it is ready for review rather than being a draft, isn't it? So feel free to make it complete and let me know when you are ready for the review and potential merge. Afterward I will then probably bump node-ical to 0.26.0 since this qualifies a bit more than just a plain bug fix, I guess... |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ical.js (1)
934-945: Consider emitting a warning when TZID resolves to neither IANA nor offset.When
tzInfois non-null but bothtzInfo.ianaandtzInfo.offsetare falsy,timeZonesilently falls back to'UTC'.getDateKey(lines 38–43) emits aconsole.warnin this identical situation — keeping the same pattern here aids debugging.♻️ Proposed alignment with
getDateKeywarning pattern- const tzInfo = curr.start.tz ? tzUtil.resolveTZID(curr.start.tz) : undefined; - const timeZone = tzInfo?.iana || tzInfo?.offset || 'UTC'; + const tzInfo = curr.start.tz ? tzUtil.resolveTZID(curr.start.tz) : undefined; + const tzId = tzInfo?.iana || tzInfo?.offset; + if (tzInfo && !tzId) { + console.warn( + '[node-ical] Could not resolve TZID to IANA name or UTC offset; falling back to UTC for DTSTART.', + {tzid: curr.start.tz, resolved: tzInfo}, + ); + } + const timeZone = tzId || 'UTC';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ical.js` around lines 934 - 945, The code silently falls back to 'UTC' when tzInfo exists but neither tzInfo.iana nor tzInfo.offset is set; update the block that computes timeZone (the tzInfo/timeZone logic used before creating dtstartTemporal) to detect when tzInfo is non-null and both tzInfo.iana and tzInfo.offset are falsy and emit a console.warn (matching getDateKey's warning pattern) stating that TZID resolved to neither IANA nor offset and that the code is falling back to UTC, then proceed to use 'UTC' for dtstartTemporal creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ical.js`:
- Around line 934-945: The code silently falls back to 'UTC' when tzInfo exists
but neither tzInfo.iana nor tzInfo.offset is set; update the block that computes
timeZone (the tzInfo/timeZone logic used before creating dtstartTemporal) to
detect when tzInfo is non-null and both tzInfo.iana and tzInfo.offset are falsy
and emit a console.warn (matching getDateKey's warning pattern) stating that
TZID resolved to neither IANA nor offset and that the code is falling back to
UTC, then proceed to use 'UTC' for dtstartTemporal creation.
…ther IANA nor offset
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ical.js (1)
44-53: Silentcatchswallows Temporal failures without any diagnostic signal.When
tzUtil.resolveTZIDorTemporal.Instant.toZonedDateTimeISOthrows (e.g., unrecognised timezone string), the caller receives a silently wrong UTC-based key with no indication anything went wrong. This makes timezone-related misbehaviour hard to diagnose.♻️ Suggested improvement
} catch { - // Fall through to UTC-based key if timezone resolution fails + console.warn('[node-ical] getDateKey: timezone resolution failed, falling back to UTC-based key.', {tz: dateValue.tz}); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ical.js` around lines 44 - 53, The silent catch in the timezone branch swallows errors from tzUtil.resolveTZID and Temporal.Instant.toZonedDateTimeISO so timezone failures produce an undiagnosed UTC fallback; update the catch to log the error and context (include tzId and dateValue) before falling back — e.g., in the catch for the block referencing tzUtil.resolveTZID and Temporal.Instant.toZonedDateTimeISO, call the existing logger (or console.error) with a clear message and the thrown error, then continue to the UTC-based key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Around line 934-950: The catch block for converting DTSTART uses error.message
which can be undefined if a non-Error is thrown; update the warning in the catch
inside the dtstartTemporal conversion so it logs a reliable string (e.g., use
error?.message ?? String(error) or String(error)) when warning that it is
falling back to UTC, referencing the dtstartTemporal conversion and
toZonedDateTimeISO/timeZone variables so the message always contains meaningful
error text.
- Around line 1157-1158: Add the missing TypeScript declaration for the exported
function getDateKey to the node-ical.d.ts file: declare and export the function
with the signature export function getDateKey(dateValue: DateWithTimeZone):
string; so TypeScript consumers can import getDateKey without type errors
(ensure DateWithTimeZone is the same type used elsewhere in the declarations).
---
Nitpick comments:
In `@ical.js`:
- Around line 44-53: The silent catch in the timezone branch swallows errors
from tzUtil.resolveTZID and Temporal.Instant.toZonedDateTimeISO so timezone
failures produce an undiagnosed UTC fallback; update the catch to log the error
and context (include tzId and dateValue) before falling back — e.g., in the
catch for the block referencing tzUtil.resolveTZID and
Temporal.Instant.toZonedDateTimeISO, call the existing logger (or console.error)
with a clear message and the thrown error, then continue to the UTC-based key.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ical.js`:
- Around line 1157-1158: Add a TypeScript declaration for the newly exported
getDateKey function so TypeScript consumers won't error: open the declaration
file node-ical.d.ts and add an exported function signature matching the runtime
API (export function getDateKey(dateValue: DateWithTimeZone): string;), ensuring
DateWithTimeZone is already declared or imported in that file; save and run the
provided verification commands to confirm getDateKey appears among exported
functions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
node-ical.js (1)
421-437: EXDATE full-day logic is correct;Object.valuesmay yield duplicateDatereferences.The
getDateKey(exdateValue)replacement correctly handles timezone-aware EXDATEs (e.g., Exchange O365EXDATE;TZID=CET:...). However,exdateParameterinical.jsstores the sameDateobject under bothdateKeyandisoStringfor every DATE-TIME entry, soObject.values(event.exdate)can include the sameDatereference twice. Functionally harmless (first match exits, duplicate just causes an extragetDateKeycall in the no-match path), but worth deduplicating.♻️ Deduplicate via `Set`
- for (const exdateValue of Object.values(event.exdate)) { + for (const exdateValue of new Set(Object.values(event.exdate))) { if (!(exdateValue instanceof Date)) { continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node-ical.js` around lines 421 - 437, The loop handling full-day EXDATEs in the branch guarded by excludeExdates and isFullDay should deduplicate duplicate Date references in event.exdate before comparing; when iterating Object.values(event.exdate) (in the code around event.exdate, getDateKey and dateKey) create a Set of unique identifiers (e.g., timestamp via getTime() or ISO string) to skip repeated Date objects, then only call getDateKey on each unique Date once and return null on match—this avoids redundant getDateKey calls while preserving the existing timezone-aware comparison logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ical.js`:
- Around line 891-907: When curr.start.tz is provided but
tzUtil.resolveTZID(curr.start.tz) returns null/undefined, the code silently
keeps timeZone='UTC' which can mis-zone DTSTART; detect this case right after
computing tzInfo (i.e., after const tzInfo = ...), and if curr.start.tz is
truthy and tzInfo is falsy emit a console.warn mentioning the unresolved TZID
(curr.start.tz) and that you're falling back to UTC; keep the existing
downstream logic that sets timeZone from tzInfo?.iana or tzInfo?.offset and
leaves dtstartTemporal creation via
Temporal.Instant.fromEpochMilliseconds(...).toZonedDateTimeISO(timeZone)
unchanged so the only change is the explicit warning when resolveTZID returns
null.
In `@lib/date-utils.js`:
- Around line 30-37: The current check only warns when resolveTZID(dateValue.tz)
returns a truthy object but no iana/offset, so completely unresolvable TZIDs
(resolved === null/undefined) silently fall back to UTC; update the guard around
tzUtil.resolveTZID(dateValue.tz) and tzId to emit a diagnostic whenever tzId is
falsy (i.e., change the condition to detect !tzId rather than resolved && !tzId)
so that you log a warning including dateValue.tz and the resolved value (even if
null); ensure you reference tzUtil.resolveTZID, the tzId variable and
dateValue.tz when adding the log call so the message is informative.
---
Nitpick comments:
In `@node-ical.js`:
- Around line 421-437: The loop handling full-day EXDATEs in the branch guarded
by excludeExdates and isFullDay should deduplicate duplicate Date references in
event.exdate before comparing; when iterating Object.values(event.exdate) (in
the code around event.exdate, getDateKey and dateKey) create a Set of unique
identifiers (e.g., timestamp via getTime() or ISO string) to skip repeated Date
objects, then only call getDateKey on each unique Date once and return null on
match—this avoids redundant getDateKey calls while preserving the existing
timezone-aware comparison logic.
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
Author
|
Thanks a lot for the kind words - really appreciated! 🙂 I’ve now addressed all nits and coderabbit seems satisfied. And yes, 0.26.0 sounds like a good fit given the scope. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
With
rrule-temporaland the Temporal polyfill already in the dependency tree, this PR revisits the remaining timezone code paths to replace legacyIntl.DateTimeFormatworkarounds and manual UTC arithmetic with direct Temporal API calls.What started as a pure refactoring effort uncovered a subtle, environment-dependent bug that had been invisible in CI for years — fixed in this PR alongside the cleanup.
Net result: 8 files changed, 122 insertions, 346 deletions — ~224 lines removed while adding a regression test and fixing a bug.
Commits
1. refactor(tz-utils): replace convergeLocalInstant with Temporal
parseDateTimeInZone()used a hand-rolled 3-round fixed-point iteration to convert wall-clock time to UTC. Replaced with a singleTemporal.PlainDateTime.from().toZonedDateTime()call. ~85 lines removed.2. refactor(node-ical): replace Intl EXDATE check with getDateKey
EXDATE exclusion matching duplicated timezone-aware date extraction that
getDateKey()already provides. Deduplicated to a single shared call.3. refactor(tz-utils): replace Intl-based formatDateForRrule with Temporal
Two separate code paths (IANA via Intl formatter cache, offset via manual arithmetic) plus a Node < 22 midnight workaround → one
Temporal.Instant.toZonedDateTimeISO()call. Dead helpernormalizeMidnightPartsremoved.4. fix(ical): normalize DATE-only RRULE start to midnight; replace Intl DTSTART with Temporal
Bug: The
getTimezoneOffset()compensation forVALUE=DATEevents — introduced incbb76af— shifted the time by the server's UTC offset on machines east of UTC (e.g.02:00:00instead of00:00:00on UTC+2). On UTC servers the buggy branch was dead code, so CI never caught it.Fix: Extract local date components directly — no offset involved. Regression test added.
Same commit also replaces the
Intl.DateTimeFormat.formatToPartsblock for DATE-TIME RRULE DTSTART withTemporal.Instant.toZonedDateTimeISO().Remaining Intl usage (intentionally kept)
guessLocalZone()— no Temporal equivalent for system timezone detectiongetZoneNames()— no Temporal equivalent for listing known zonesisValidIana()— more portable than Temporal; accepts fixed-offset strings; result is cachedSummary by CodeRabbit
Bug Fixes
New Features
Chores
Tests