Introduce chrono handler registrar#64
Conversation
…on surface of Chrono. Chrono<TaskMapping> is invariant in TaskMapping because TaskMapping flows into scheduleTask (contravariant) and out through registerTaskHandler callbacks (covariant). This means Chrono<BigMapping> is not assignable to Chrono<SmallMapping> even when BigMapping is a strict superset — TypeScript correctly rejects it. ChronoHandlerRegistrar isolates the covariant half: it only exposes registerTaskHandler, where TaskMapping flows exclusively into handler callbacks (doubly contravariant = covariant). The out T annotation makes this explicit and allows a Chrono<BigMapping> instance to be safely narrowed to ChronoHandlerRegistrar<SmallMapping> without any type assertions. This is a breaking change (1.0.0) because it adds a new exported type to the public API surface that consumers of Chrono may need to implement or reference. Also fixed A latent bug in RegisterTaskHandlerResponse: the type was passing TaskMapping[TaskKind] (the task data type) as the second argument to ProcessorEventsMap where it expected the full TaskMapping. This was harmless before stricter variance checking surfaced it.
darrenpicard25
requested review from
maiahneo,
neo-luca and
tjamescouch
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated dev tooling and CI, bumped package versions, and added a type-only exporter/interface ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/chrono-core/package.json`:
- Line 3: The package.json "version" field in packages/chrono-core (the
"version" key currently "0.6.1") is incorrect for a breaking change release;
update the "version" value to "1.0.0" in packages/chrono-core/package.json and
ensure any release metadata (changelog/release notes) reflects the 1.0.0
breaking-change bump so the published package matches the contract.
In `@packages/chrono-mongo-datastore/package.json`:
- Line 3: Update the package version in chrono-mongo-datastore by changing the
"version" field from "0.6.1" to "1.0.0" in package.json so the package release
aligns with the breaking-change rollout and the PR's intended major version
bump.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46520bbf-fb02-4e71-ba14-5bcb8877a568
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
biome.jsonpackage.jsonpackages/chrono-core/package.jsonpackages/chrono-core/src/chrono.tspackages/chrono-core/src/index.tspackages/chrono-mongo-datastore/package.json
| { | ||
| "name": "@neofinancial/chrono", | ||
| "version": "0.6.0", | ||
| "version": "0.6.1", |
There was a problem hiding this comment.
Versioning does not match the stated breaking-change policy.
Line 3 applies a patch bump (0.6.1), but this PR is described as a breaking change targeting 1.0.0. Please align the published version with that contract before release.
📦 Proposed fix
- "version": "0.6.1",
+ "version": "1.0.0",📝 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.
| "version": "0.6.1", | |
| "version": "1.0.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chrono-core/package.json` at line 3, The package.json "version"
field in packages/chrono-core (the "version" key currently "0.6.1") is incorrect
for a breaking change release; update the "version" value to "1.0.0" in
packages/chrono-core/package.json and ensure any release metadata
(changelog/release notes) reflects the 1.0.0 breaking-change bump so the
published package matches the contract.
| { | ||
| "name": "@neofinancial/chrono-mongo-datastore", | ||
| "version": "0.6.0", | ||
| "version": "0.6.1", |
There was a problem hiding this comment.
Release version should align with the breaking-change rollout.
Line 3 is a patch bump (0.6.1), but the PR is treated as a breaking change (1.0.0). This package version should be aligned with that release decision as well.
📦 Proposed fix
- "version": "0.6.1",
+ "version": "1.0.0",📝 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.
| "version": "0.6.1", | |
| "version": "1.0.0", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/chrono-mongo-datastore/package.json` at line 3, Update the package
version in chrono-mongo-datastore by changing the "version" field from "0.6.1"
to "1.0.0" in package.json so the package release aligns with the
breaking-change rollout and the PR's intended major version bump.
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![wiz-a38520980d[bot]](https://avatars.githubusercontent.com/u/47724915?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
29-38:⚠️ Potential issue | 🟠 MajorInclude Node version in cache key to prevent cross-version restoration.
The cache key at lines 36–38 does not differentiate between Node versions (20.x, 22.x, 24.x). This allows matrix jobs to restore
node_modulesbuilt for a different Node ABI, causing native module failures and flaky CI.Suggested fix
- name: Cache Package Node Modules uses: actions/cache@v5 with: path: | ~/.pnpm-store - node_modules - packages/*/node_modules - key: pnpm-${{ hashFiles('pnpm-lock.yaml') }} + key: pnpm-${{ runner.os }}-${{ matrix.node-version }}-${{ hashFiles('pnpm-lock.yaml') }} restore-keys: | - pnpm- + pnpm-${{ runner.os }}-${{ matrix.node-version }}-🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 29 - 38, The cache key for the "Cache Package Node Modules" step doesn't include the Node version, allowing cross-version restores; update the cache key and restore-keys to incorporate the Node version identifier used in the matrix (e.g., matrix.node-version or the node-version variable) so the key becomes version-specific (for example prefixing or interpolating ${ { matrix.node-version } } into the existing pnpm-${{ hashFiles('pnpm-lock.yaml') }} key and similarly updating restore-keys), leaving the path and actions/cache@v5 usage unchanged.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
22-30: Pin Actions to immutable SHAs instead of mutable major tags.Lines 22, 25, and 30 use floating major tags (
@v6,@v6,@v5). For CI supply-chain hardening, pin each action to a full commit SHA with a comment noting the intended major/minor version.Reference docs:
- https://github.com/actions/checkout
- https://github.com/actions/setup-node
- https://github.com/actions/cache
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 22 - 30, Replace the mutable action tags with immutable commit SHAs: change uses: actions/checkout@v6, uses: actions/setup-node@v6, and uses: actions/cache@v5 to pinned references using the full commit SHA for each repo, and add an inline comment next to each use stating the intended major/minor version (e.g., // intended: v6.x) so reviewers know the intended version while ensuring supply-chain immutability; locate these usages in the workflow steps for Checkout, Setup Node, and Cache Package Node Modules and update them accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 29-38: The cache key for the "Cache Package Node Modules" step
doesn't include the Node version, allowing cross-version restores; update the
cache key and restore-keys to incorporate the Node version identifier used in
the matrix (e.g., matrix.node-version or the node-version variable) so the key
becomes version-specific (for example prefixing or interpolating ${ {
matrix.node-version } } into the existing pnpm-${{ hashFiles('pnpm-lock.yaml')
}} key and similarly updating restore-keys), leaving the path and
actions/cache@v5 usage unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 22-30: Replace the mutable action tags with immutable commit SHAs:
change uses: actions/checkout@v6, uses: actions/setup-node@v6, and uses:
actions/cache@v5 to pinned references using the full commit SHA for each repo,
and add an inline comment next to each use stating the intended major/minor
version (e.g., // intended: v6.x) so reviewers know the intended version while
ensuring supply-chain immutability; locate these usages in the workflow steps
for Checkout, Setup Node, and Cache Package Node Modules and update them
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d2656b1-907f-4e1c-9853-9af17c19ae86
📒 Files selected for processing (1)
.github/workflows/ci.yml
| - name: Install | ||
| run: | | ||
| npm install -g npm@^11.2.0 | ||
| npm install -g pnpm@latest-10 |
There was a problem hiding this comment.
so discovered node has this built in thing called corepack that does:
Corepack is a zero-runtime-dependency Node.js script that acts as a bridge between Node.js projects and the package managers they are intended to be used with during development. In practical terms, Corepack lets you use Yarn, npm, and pnpm without having to install them.
so thought that was cool
| { | ||
| "name": "@neofinancial/chrono", | ||
| "version": "0.6.1", | ||
| "version": "0.7.0", |
maiahneo
left a comment
maiahneo
left a comment
There was a problem hiding this comment.
I have some concern in the corepack usage below.
|
Weird that CI build for v22.x is failing w/ following error: |
yeah something happened in newest node 22 bundle. So just going to pin version in CI to avoid this |
…#65) This is a follow up to PR: #64 that intorduced ChronoHandlerRegistrar type Add ChronoTaskScheduler interface to solve TypeScript variance issues when passing a wide Chrono to code that only needs a subset of the task mapping. ChronoTaskScheduler exposes only scheduleTask() for producers, while ChronoHandlerRegistrar (with out variance annotation) exposes only registerTaskHandler() for consumers. The Chrono class now implements both interfaces. Update the example app to demonstrate the new narrower types by extracting registerHandlers(registrar: ChronoHandlerRegistrar) and scheduleTasks(scheduler: ChronoTaskScheduler) helper functions, showing that a full Chrono instance satisfies both interfaces. Update chrono-core documentation to list ChronoTaskScheduler and ChronoHandlerRegistrar in the Exported Types table and add a note to the Chrono Class API section explaining how the narrow interfaces can be used. Add publish:alpha root script for publishing prerelease versions with --tag alpha. Bump package versions to 0.7.1-alpha.0 for @neofinancial/chrono and @neofinancial/chrono-mongo-datastore. Dev-dependency bumps: @biomejs/biome 2.4.10 -> 2.4.11, vitest 4.1.2 -> 4.1.4, vitest-mock-extended 3.1.1 -> 4.0.0
2 participants
Introduce ChronoHandlerRegistrar interface
Adds a new covariant interface that represents the handler-registration surface of Chrono:
Chrono now declares implements ChronoHandlerRegistrar.
Why this is needed
Chrono is invariant in TaskMapping because TaskMapping flows into scheduleTask (contravariant) and out through registerTaskHandler callbacks (covariant). This means Chrono is not assignable to Chrono even when BigMapping is a strict superset — TypeScript correctly rejects it.
ChronoHandlerRegistrar isolates the covariant half: it only exposes registerTaskHandler, where TaskMapping flows exclusively into handler callbacks (doubly contravariant = covariant). The out T annotation makes this explicit and allows a Chrono instance to be safely narrowed to ChronoHandlerRegistrar without any type assertions.
This is a breaking change (1.0.0) because it adds a new exported type to the public API surface that consumers of Chrono may need to implement or reference.
Also fixed
A latent bug in RegisterTaskHandlerResponse: the type was passing TaskMapping[TaskKind] (the task data type) as the second argument to ProcessorEventsMap where it expected the full TaskMapping. This was harmless before stricter variance checking surfaced it.
Summary by CodeRabbit
Chores
New Features