Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the @ciscode/health-kit module’s public API and module registration options while expanding the test suite and tightening coverage thresholds.
Changes:
- Renames the
HealthCheckResultresponse field fromindicatorstoresults. - Adds
HealthKitModule.registerAsync()and makesregister()options more optional/defaulted. - Removes the Mongo indicator implementation/tests and adjusts package metadata (Postgres-focused) and Jest coverage thresholds.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/health.service.ts | Renames HealthCheckResult.indicators to results (public API response change). |
| src/services/health.service.spec.ts | Updates tests to assert results instead of indicators. |
| src/controllers/health.controller.spec.ts | Updates controller tests to mock results instead of indicators. |
| src/health-kit.module.ts | Adds registerAsync(), defaults path to "health", refactors provider wiring. |
| src/health-kit.module.spec.ts | Adds module-level tests for registration behavior and DI indicator routing. |
| src/index.ts | Exports HealthModuleAsyncOptions and removes Mongo indicator exports. |
| src/indicators/mongo.indicator.ts | Removes Mongo health indicator implementation. |
| src/indicators/mongo.indicator.spec.ts | Removes Mongo indicator unit tests. |
| package.json | Updates description, adds optional peer dep @nestjs/terminus. |
| jest.config.ts | Raises global Jest coverage thresholds from 80% to 85%. |
| export interface HealthCheckResult { | ||
| status: "ok" | "error"; | ||
| indicators: HealthIndicatorResult[]; | ||
| results: HealthIndicatorResult[]; | ||
| } |
There was a problem hiding this comment.
Renaming the public HealthCheckResult field from indicators to results is a breaking change for both runtime responses and TypeScript consumers. Consider keeping indicators as a deprecated alias (or returning both) until a major version bump, and ensure the package version/changeset reflects the breaking API change.
| static register(options: HealthModuleOptions = {}): DynamicModule { | ||
| const { path = "health", liveness = [], readiness = [], indicators = [] } = options; | ||
| const { indicatorProviders, livenessClasses, readinessClasses } = | ||
| HealthKitModule._resolveIndicatorClasses(indicators); |
There was a problem hiding this comment.
The module-level JSDoc @example above still imports/uses MongoHealthIndicator, but the Mongo indicator/export was removed in this PR. Please update that example to a supported indicator (e.g., Postgres) or a custom indicator so consumers don’t copy/paste broken code.
| /** Factory that returns liveness/readiness/indicators options. */ | ||
| useFactory: ( | ||
| ...args: unknown[] | ||
| ) => Promise<Omit<HealthModuleOptions, "path">> | Omit<HealthModuleOptions, "path">; |
There was a problem hiding this comment.
HealthModuleAsyncOptions.useFactory is documented/typed as returning "liveness/readiness/indicators" options, but registerAsync() cannot honor indicators returned from the factory (providers must be registered up front from options.indicators). Consider changing the return type/documentation to omit indicators (and possibly path) to avoid suggesting unsupported configuration.
| /** Factory that returns liveness/readiness/indicators options. */ | |
| useFactory: ( | |
| ...args: unknown[] | |
| ) => Promise<Omit<HealthModuleOptions, "path">> | Omit<HealthModuleOptions, "path">; | |
| /** Factory that returns liveness/readiness options (no indicators). */ | |
| useFactory: ( | |
| ...args: unknown[] | |
| ) => Promise<Omit<HealthModuleOptions, "path" | "indicators">> | Omit<HealthModuleOptions, "path" | "indicators">; |
| // ÔöÇÔöÇ Fixtures ÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇÔöÇ | ||
|
|
There was a problem hiding this comment.
The section divider comments contain garbled characters ("ÔöÇ"), which looks like a bad encoding/transcoding artifact and hurts readability/searchability. Replace these with plain ASCII (e.g., // --- Fixtures ---) or remove them.
| const liveness = module.get<HealthIndicatorResult[]>(HEALTH_LIVENESS_INDICATORS); | ||
| expect(liveness).toHaveLength(1); | ||
|
|
||
| const readiness = module.get<HealthIndicatorResult[]>(HEALTH_READINESS_INDICATORS); | ||
| expect(readiness).toHaveLength(0); |
There was a problem hiding this comment.
These DI tokens (HEALTH_LIVENESS_INDICATORS / HEALTH_READINESS_INDICATORS) provide arrays of indicator instances (IHealthIndicator[]/BaseHealthIndicator[]), but the test retrieves them as HealthIndicatorResult[]. Using the correct type here will prevent confusion and type-unsafe usage in future test edits.
| "@nestjs/terminus": "^10 || ^11", | ||
| "reflect-metadata": "^0.2.2", | ||
| "rxjs": "^7" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "@nestjs/terminus": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
@nestjs/terminus is added as an (optional) peer dependency, but there are no imports/usages of it in the codebase. Unless you’re intentionally declaring compatibility, consider removing it to avoid unnecessary dependency management for consumers.
| "@nestjs/terminus": "^10 || ^11", | |
| "reflect-metadata": "^0.2.2", | |
| "rxjs": "^7" | |
| }, | |
| "peerDependenciesMeta": { | |
| "@nestjs/terminus": { | |
| "optional": true | |
| } | |
| }, | |
| "reflect-metadata": "^0.2.2", | |
| "rxjs": "^7" | |
| }, |
|
|
||
| export { RedisHealthIndicator } from "./indicators/redis.indicator"; | ||
| export type { RedisClient } from "./indicators/redis.indicator"; | ||
|
|
There was a problem hiding this comment.
Removing MongoHealthIndicator / MongoDb from the public exports is a breaking change for consumers. If the removal is intentional, ensure the release is handled as a breaking change (major version + changeset/changelog); otherwise, consider keeping the export (or providing a migration path).
| export { MongoHealthIndicator } from "./indicators/mongo.indicator"; | |
| export type { MongoDb } from "./indicators/mongo.indicator"; |
bb09dec to
f316894
Compare
|



Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes