refactor: ESM migration#523
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe package is migrated to ESM with explicit CommonJS compatibility, shared parser and recurring-event modules, generated build artifacts, updated timezone tooling, converted examples and tests, and revised package documentation. ChangesESM package and calendar processing
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 8
🧹 Nitpick comments (2)
test/module-formats.test.js (1)
49-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated key-parity check across two tests.
The key-parity loop at Lines 61–63 (
for (const key of Object.keys(esm.default)) { assert.ok(Object.hasOwn(cjs, key), ...) }) is repeated verbatim at Lines 73–75 in the next test. Consider consolidating into a shared helper or merging the assertions into a single test to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/module-formats.test.js` around lines 49 - 76, Consolidate the duplicated ESM/CJS key-parity loop currently present in both tests. Either merge the assertions into one test or extract a shared helper and reuse it from the tests, while preserving the existing missing-key assertion message and API checks.node-ical.d.ts (1)
112-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider more precise types for internal properties currently typed
unknown.
objectHandlers,handleObject, andparseLinesare typed asunknown, which gives consumers no type safety if they access these properties. If this is intentional to discourage direct use, consider adding a brief comment explaining that. Otherwise, typing them (e.g., asRecord<string, (...args: unknown[]) => unknown>and(...args: unknown[]) => unknown) would improve the developer experience for advanced consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node-ical.d.ts` around lines 112 - 125, Improve the declarations for objectHandlers, handleObject, and parseLines in the default export object: either replace unknown with accurate callable/record types for these internal properties, or add a brief comment documenting that unknown is intentional to discourage direct consumer use. Preserve the existing exported API shape and type the properties consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build/update-windows-zones.js`:
- Around line 18-45: Update fetchText to enforce a request timeout so stalled
CLDR requests reject instead of hanging CI. Prefer replacing the manual
https.get redirect flow with Node 20’s fetch and AbortSignal.timeout(), while
preserving non-success HTTP errors and returning the response text for
successful requests.
In `@ical.js`:
- Around line 463-475: Update the named exports for handleObject, parseLines,
and parseICS in ical.js to bind each function to the ical object so calls retain
the required this context; apply the same binding treatment to the equivalent
exports in node-ical.js, while leaving objectHandlers unchanged.
In `@lib/expand-recurring-event.js`:
- Around line 94-120: Update getEventDurationMs and calculateEndTime to
represent full-day durations as calendar-day counts rather than fixed
milliseconds. For full-day events, compute the span between local calendar dates
and advance the start using local date arithmetic so DST transitions preserve
the correct end date; keep timed-event millisecond duration behavior unchanged.
- Around line 345-358: Update the recurring-instance expansion around
buildRecurringInstance so it also inspects event.recurrences overrides whose
effective start falls within the requested range, even when their RECURRENCE-ID
is outside searchFrom..searchTo. Merge these override-derived instances with the
RRULE results, deduplicate instances already produced through RRULE expansion,
then retain the existing range filtering and start-time sorting.
In `@lib/ical-parser-utils.js`:
- Around line 41-46: Update copyRecordFields to iterate and copy only source own
properties using Object.hasOwn, and avoid assignment-based writes for the
__proto__ UID by defining UID-keyed properties with Object.defineProperty or
using null-prototype records throughout the related merge/recurrence path. Apply
the same protection to the code around the UID handling at the referenced later
section, preserving normal field-copy behavior for other keys.
- Around line 721-733: Update freebusyParameter so it invokes the FREEBUSY
storage handler returned or registered by storeParameter, rather than merely
calling storeParameter and discarding its result. Ensure the raw value and
parameters are persisted on fb before dateParameter processes the start and end
parts.
- Around line 185-194: Update applyImplicitEndDate for DATE entries to advance
entry.start by one calendar day while preserving local midnight, rather than
using utcAdd(..., 1, 'days'). Keep the existing date-time cloning behavior and
return flow unchanged.
In `@package.json`:
- Around line 4-24: Update the package.json engines.node constraint to require
Node.js >=20.10.0, ensuring the ESM entry using JSON import attributes is not
exposed to unsupported Node 20.0–20.9 versions.
---
Nitpick comments:
In `@node-ical.d.ts`:
- Around line 112-125: Improve the declarations for objectHandlers,
handleObject, and parseLines in the default export object: either replace
unknown with accurate callable/record types for these internal properties, or
add a brief comment documenting that unknown is intentional to discourage direct
consumer use. Preserve the existing exported API shape and type the properties
consistently.
In `@test/module-formats.test.js`:
- Around line 49-76: Consolidate the duplicated ESM/CJS key-parity loop
currently present in both tests. Either merge the assertions into one test or
extract a shared helper and reuse it from the tests, while preserving the
existing missing-key assertion message and API checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b141778-ebaf-4a2d-b1e9-418028e5ec77
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.github/workflows/nodejs.yml.gitignoreREADME.mdbuild/README.mdbuild/build-cjs.jsbuild/update-windows-zones.jsexamples/example-rrule-basic.jsexamples/example-rrule-datefns.jsexamples/example-rrule-dayjs.jsexamples/example-rrule-luxon.jsexamples/example-rrule-moment.jsexamples/example-rrule-vanilla.jsexamples/example.jsical.jslib/core-api.jslib/date-utils.jslib/expand-recurring-event.jslib/ical-parser-utils.jslib/public-api.jslib/temporal.jslib/tz-utils.jsnode-ical.d.tsnode-ical.jspackage.jsontest/advanced.test.jstest/async-error-handling.test.jstest/basic.test.jstest/date-only-rrule-until.test.jstest/examples-snapshot.test.jstest/expand-recurring-event.test.jstest/extended-components.test.jstest/from-url.test.jstest/google-calendar-until-bug.test.jstest/issue-459-exchange-recurrence-id.test.jstest/meta-preservation.test.jstest/module-formats.test.jstest/monthly-bymonthday-multiple.test.jstest/non-utc-until.test.jstest/polyfill-check.test.jstest/tsconfig.jsontest/tz-utils.test.jstsconfig.json
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
🧹 Nitpick comments (2)
test/module-formats.test.js (1)
49-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated key-parity check across two tests.
The key-parity loop at Lines 61–63 (
for (const key of Object.keys(esm.default)) { assert.ok(Object.hasOwn(cjs, key), ...) }) is repeated verbatim at Lines 73–75 in the next test. Consider consolidating into a shared helper or merging the assertions into a single test to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/module-formats.test.js` around lines 49 - 76, Consolidate the duplicated ESM/CJS key-parity loop currently present in both tests. Either merge the assertions into one test or extract a shared helper and reuse it from the tests, while preserving the existing missing-key assertion message and API checks.node-ical.d.ts (1)
112-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider more precise types for internal properties currently typed
unknown.
objectHandlers,handleObject, andparseLinesare typed asunknown, which gives consumers no type safety if they access these properties. If this is intentional to discourage direct use, consider adding a brief comment explaining that. Otherwise, typing them (e.g., asRecord<string, (...args: unknown[]) => unknown>and(...args: unknown[]) => unknown) would improve the developer experience for advanced consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@node-ical.d.ts` around lines 112 - 125, Improve the declarations for objectHandlers, handleObject, and parseLines in the default export object: either replace unknown with accurate callable/record types for these internal properties, or add a brief comment documenting that unknown is intentional to discourage direct consumer use. Preserve the existing exported API shape and type the properties consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build/update-windows-zones.js`:
- Around line 18-45: Update fetchText to enforce a request timeout so stalled
CLDR requests reject instead of hanging CI. Prefer replacing the manual
https.get redirect flow with Node 20’s fetch and AbortSignal.timeout(), while
preserving non-success HTTP errors and returning the response text for
successful requests.
In `@ical.js`:
- Around line 463-475: Update the named exports for handleObject, parseLines,
and parseICS in ical.js to bind each function to the ical object so calls retain
the required this context; apply the same binding treatment to the equivalent
exports in node-ical.js, while leaving objectHandlers unchanged.
In `@lib/expand-recurring-event.js`:
- Around line 94-120: Update getEventDurationMs and calculateEndTime to
represent full-day durations as calendar-day counts rather than fixed
milliseconds. For full-day events, compute the span between local calendar dates
and advance the start using local date arithmetic so DST transitions preserve
the correct end date; keep timed-event millisecond duration behavior unchanged.
- Around line 345-358: Update the recurring-instance expansion around
buildRecurringInstance so it also inspects event.recurrences overrides whose
effective start falls within the requested range, even when their RECURRENCE-ID
is outside searchFrom..searchTo. Merge these override-derived instances with the
RRULE results, deduplicate instances already produced through RRULE expansion,
then retain the existing range filtering and start-time sorting.
In `@lib/ical-parser-utils.js`:
- Around line 41-46: Update copyRecordFields to iterate and copy only source own
properties using Object.hasOwn, and avoid assignment-based writes for the
__proto__ UID by defining UID-keyed properties with Object.defineProperty or
using null-prototype records throughout the related merge/recurrence path. Apply
the same protection to the code around the UID handling at the referenced later
section, preserving normal field-copy behavior for other keys.
- Around line 721-733: Update freebusyParameter so it invokes the FREEBUSY
storage handler returned or registered by storeParameter, rather than merely
calling storeParameter and discarding its result. Ensure the raw value and
parameters are persisted on fb before dateParameter processes the start and end
parts.
- Around line 185-194: Update applyImplicitEndDate for DATE entries to advance
entry.start by one calendar day while preserving local midnight, rather than
using utcAdd(..., 1, 'days'). Keep the existing date-time cloning behavior and
return flow unchanged.
In `@package.json`:
- Around line 4-24: Update the package.json engines.node constraint to require
Node.js >=20.10.0, ensuring the ESM entry using JSON import attributes is not
exposed to unsupported Node 20.0–20.9 versions.
---
Nitpick comments:
In `@node-ical.d.ts`:
- Around line 112-125: Improve the declarations for objectHandlers,
handleObject, and parseLines in the default export object: either replace
unknown with accurate callable/record types for these internal properties, or
add a brief comment documenting that unknown is intentional to discourage direct
consumer use. Preserve the existing exported API shape and type the properties
consistently.
In `@test/module-formats.test.js`:
- Around line 49-76: Consolidate the duplicated ESM/CJS key-parity loop
currently present in both tests. Either merge the assertions into one test or
extract a shared helper and reuse it from the tests, while preserving the
existing missing-key assertion message and API checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b141778-ebaf-4a2d-b1e9-418028e5ec77
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.github/workflows/nodejs.yml.gitignoreREADME.mdbuild/README.mdbuild/build-cjs.jsbuild/update-windows-zones.jsexamples/example-rrule-basic.jsexamples/example-rrule-datefns.jsexamples/example-rrule-dayjs.jsexamples/example-rrule-luxon.jsexamples/example-rrule-moment.jsexamples/example-rrule-vanilla.jsexamples/example.jsical.jslib/core-api.jslib/date-utils.jslib/expand-recurring-event.jslib/ical-parser-utils.jslib/public-api.jslib/temporal.jslib/tz-utils.jsnode-ical.d.tsnode-ical.jspackage.jsontest/advanced.test.jstest/async-error-handling.test.jstest/basic.test.jstest/date-only-rrule-until.test.jstest/examples-snapshot.test.jstest/expand-recurring-event.test.jstest/extended-components.test.jstest/from-url.test.jstest/google-calendar-until-bug.test.jstest/issue-459-exchange-recurrence-id.test.jstest/meta-preservation.test.jstest/module-formats.test.jstest/monthly-bymonthday-multiple.test.jstest/non-utc-until.test.jstest/polyfill-check.test.jstest/tsconfig.jsontest/tz-utils.test.jstsconfig.json
🛑 Comments failed to post (8)
build/update-windows-zones.js (1)
18-45: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify AbortSignal.timeout() and fetch() availability in Node 20+ node -e " const hasFetch = typeof fetch === 'function'; const hasAbortTimeout = typeof AbortSignal.timeout === 'function'; console.log('fetch:', hasFetch); console.log('AbortSignal.timeout:', hasAbortTimeout); if (!hasFetch || !hasAbortTimeout) process.exit(1); "Repository: jens-maus/node-ical
Length of output: 195
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the file structure and relevant symbols ast-grep outline build/update-windows-zones.js --view expanded || true # Read the file with line numbers wc -l build/update-windows-zones.js cat -n build/update-windows-zones.js | sed -n '1,220p' # Check whether https is used elsewhere in the file rg -n "\bhttps\b|fetchText|AbortSignal\.timeout|globalThis\.fetch|fetch\(" build/update-windows-zones.jsRepository: jens-maus/node-ical
Length of output: 7600
Add a timeout to
fetchTextto avoid CI hangs.
https.get()here can wait indefinitely if the CLDR endpoint stalls. Since this script runs on Node 20+, switching tofetch()withAbortSignal.timeout()would also remove the manual redirect handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@build/update-windows-zones.js` around lines 18 - 45, Update fetchText to enforce a request timeout so stalled CLDR requests reject instead of hanging CI. Prefer replacing the manual https.get redirect flow with Node 20’s fetch and AbortSignal.timeout(), while preserving non-success HTTP errors and returning the response text for successful requests.ical.js (1)
463-475: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Bind named exports that depend on
this.Directly importing
handleObject,parseLines, orparseICScalls them withthis === undefined; each accesses another method or property throughthisand will fail. Bind these exports toical. The equivalent exports innode-ical.jsneed the same treatment.Proposed fix
const { objectHandlers, - handleObject, - parseLines, - parseICS, } = ical; +const handleObject = ical.handleObject.bind(ical); +const parseLines = ical.parseLines.bind(ical); +const parseICS = ical.parseICS.bind(ical);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const { objectHandlers, } = ical; const handleObject = ical.handleObject.bind(ical); const parseLines = ical.parseLines.bind(ical); const parseICS = ical.parseICS.bind(ical); export { objectHandlers, handleObject, parseLines, parseICS, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ical.js` around lines 463 - 475, Update the named exports for handleObject, parseLines, and parseICS in ical.js to bind each function to the ical object so calls retain the required this context; apply the same binding treatment to the equivalent exports in node-ical.js, while leaving objectHandlers unchanged.lib/expand-recurring-event.js (2)
94-120: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Use calendar-day spans for full-day event ends.
Adding a fixed millisecond duration to local midnight produces 23:00 or 01:00 ends around DST transitions. Track full-day duration as a number of calendar dates and advance with local calendar arithmetic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/expand-recurring-event.js` around lines 94 - 120, Update getEventDurationMs and calculateEndTime to represent full-day durations as calendar-day counts rather than fixed milliseconds. For full-day events, compute the span between local calendar dates and advance the start using local date arithmetic so DST transitions preserve the correct end date; keep timed-event millisecond duration behavior unchanged.
345-358: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Include overrides moved into the requested range.
Only RRULE dates inside
searchFrom..searchToare inspected. An override keyed by an earlierRECURRENCE-IDbut moved into this range is therefore omitted. Merge uniqueevent.recurrenceswhose effective starts overlap the range, while deduplicating overrides already reached through RRULE expansion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/expand-recurring-event.js` around lines 345 - 358, Update the recurring-instance expansion around buildRecurringInstance so it also inspects event.recurrences overrides whose effective start falls within the requested range, even when their RECURRENCE-ID is outside searchFrom..searchTo. Merge these override-derived instances with the RRULE results, deduplicate instances already produced through RRULE expansion, then retain the existing range filtering and start-time sorting.lib/ical-parser-utils.js (3)
41-46: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Store UID keys without invoking object prototype setters.
A UID of
__proto__resolves toObject.prototype; the merge or recurrence path then writes parsed event fields onto the global prototype. UseObject.hasOwn, copy only own fields, and define UID properties withObject.definePropertyor use null-prototype records.Proposed approach
+function setRecordField(target, key, value) { + Object.defineProperty(target, key, { + value, + enumerable: true, + configurable: true, + writable: true, + }); +} + function copyRecordFields(target, source) { - for (const key in source) { - if (key !== null) { - target[key] = source[key]; - } + for (const key of Object.keys(source)) { + setRecordField(target, key, source[key]); } } ... - if (parentEntry[uid] === undefined) { - parentEntry[uid] = sourceEntry; + if (!Object.hasOwn(parentEntry, uid)) { + setRecordField(parentEntry, uid, sourceEntry);Also applies to: 99-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ical-parser-utils.js` around lines 41 - 46, Update copyRecordFields to iterate and copy only source own properties using Object.hasOwn, and avoid assignment-based writes for the __proto__ UID by defining UID-keyed properties with Object.defineProperty or using null-prototype records throughout the related merge/recurrence path. Apply the same protection to the code around the UID handling at the referenced later section, preserving normal field-copy behavior for other keys.
185-194: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Advance DATE values by a calendar day, not 24 elapsed hours.
utcAdd(..., 1, 'days')adds exactly 24 hours. Across DST transitions, the implicit end can become 23:00 or 01:00 instead of local midnight on the next date.Proposed fix
if (entry.duration === undefined) { - entry.end = entry.datetype === 'date-time' - ? cloneDateWithMeta(entry.start) - : cloneDateWithMeta(entry.start, utcAdd(entry.start, 1, 'days')); + entry.end = cloneDateWithMeta(entry.start); + if (entry.datetype !== 'date-time') { + entry.end.setDate(entry.end.getDate() + 1); + } return entry; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function applyImplicitEndDate(entry, utcAdd) { if (entry.end) { return entry; } if (entry.duration === undefined) { entry.end = cloneDateWithMeta(entry.start); if (entry.datetype !== 'date-time') { entry.end.setDate(entry.end.getDate() + 1); } return entry;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ical-parser-utils.js` around lines 185 - 194, Update applyImplicitEndDate for DATE entries to advance entry.start by one calendar day while preserving local midnight, rather than using utcAdd(..., 1, 'days'). Keep the existing date-time cloning behavior and return flow unchanged.
721-733: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invoke the FREEBUSY storage handler.
storeParameter(value, parameters, fb)only creates and discards a handler, so the raw FREEBUSY value and its parameters are never stored.Proposed fix
- storeParameter(value, parameters, fb); + storeParameter(name)(value, parameters, fb);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const freebusyParameter = function (name) { return function (value, parameters, curr) { const fb = addFBType({}, parameters); curr[name] ||= []; curr[name].push(fb); storeParameter(name)(value, parameters, fb); const parts = value.split('/'); for (const [index, partName] of ['start', 'end'].entries()) { dateParameter(partName)(parts[index], parameters, fb); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/ical-parser-utils.js` around lines 721 - 733, Update freebusyParameter so it invokes the FREEBUSY storage handler returned or registered by storeParameter, rather than merely calling storeParameter and discarding its result. Ensure the raw value and parameters are persisted on fb before dateParameter processes the start and end parts.package.json (1)
4-24: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check the engines.node field and verify compatibility with import attributes cat package.json | jq '.engines'Repository: jens-maus/node-ical
Length of output: 178
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n## package.json engines and module fields\n' cat -n package.json | sed -n '1,80p' printf '\n## referenced source snippets\n' for f in lib/tz-utils.js lib/temporal.js; do echo "\n### $f" cat -n "$f" | sed -n '1,220p' done printf '\n## shipped entry files\n' git ls-files 'node-ical.*' 'lib/*.js' 'windowsZones.json'Repository: jens-maus/node-ical
Length of output: 11636
Bump
engines.nodeto>=20.10.0.lib/tz-utils.jsuses JSON import attributes, so>=20still admits Node 20.0–20.9, which will fail to load the ESM entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` around lines 4 - 24, Update the package.json engines.node constraint to require Node.js >=20.10.0, ensuring the ESM entry using JSON import attributes is not exposed to unsupported Node 20.0–20.9 versions.Source: Learnings
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@jens-maus coderabbit seems satisfied. What do you think? |
1 participant
This change follows up on issue #521 and moves node-ical to an ESM-first setup, while keeping CommonJS support in place for now.
It turned out to take a bit more than expected. To make the module split cleanly, I moved a fair amount of logic out of the files ical.js and node-ical.js into smaller, focused modules. That makes the code easier to maintain and gives a cleaner base for the module format transition.
I checked compatibility against our main consumers (ioBroker.ical and MagicMirror), and both still work with this setup.
Summary by CodeRabbit
require(), including a CommonJS entry point.