Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: lokalise/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request introduces a new Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant Contract as RouteContract
participant PathResolver as Path Resolver
participant Validator as Zod Validator
participant HTTP as HTTP Layer
participant ResponseHandler as Response Handler
Client->>Contract: sendByRouteContract(contract, params)
Contract->>PathResolver: Resolve path with pathParams
PathResolver-->>Contract: Resolved URL path
Contract->>Validator: Validate pathParams schema
Validator-->>Contract: Validated params
Contract->>Validator: Validate query schema
Validator-->>Contract: Validated query
Contract->>Validator: Validate body schema (if present)
Validator-->>Contract: Validated body
Contract->>HTTP: Send HTTP request (method, URL, headers, query, body)
HTTP-->>Contract: Receive HTTP response (statusCode, body)
Contract->>ResponseHandler: Select response schema by statusCode
ResponseHandler-->>Contract: Response schema
Contract->>Validator: Validate response body against schema
Validator-->>Contract: Parsed response
Contract-->>Client: Return typed result (success or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 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)
Comment |
packages/app/api-contracts/src/route-contract/defineRouteContract.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| async function sendResourceChange< | ||
| export async function sendResourceChange< |
There was a problem hiding this comment.
Nit: This file is already quite large, could we extract this newly exported method into an internal utilities file (or something similar) to keep things more manageable?
There was a problem hiding this comment.
I think instead of extracting this method, it might be better to remove the outdated functions that currently take up a large portion of this file. That should significantly reduce its size and keep related logic in one place.
Not sure if we want to remove depreciations within this PR, tho.
There was a problem hiding this comment.
I am not sure either, maybe we should release it as deprecated to not force migration
There was a problem hiding this comment.
I meant that the existing httpClient file has 6 deprecated functions. Once we remove them, the file itself will be way smaller.
There was a problem hiding this comment.
Anyway, agree on extracting shared code to a util file. Will do.
| @@ -0,0 +1,382 @@ | |||
| import type { Readable } from 'node:stream' | |||
| import { ContractNoBody, defineRouteContract } from '@lokalise/api-contracts' | |||
There was a problem hiding this comment.
Should we update api-contract version on package.json?
There was a problem hiding this comment.
Actually, how is the versioning managed? Do we have to manually bump versions in package.json files?
There was a problem hiding this comment.
The release pipeline updates the version based on the PR label, but if you need to adjust the version constraint in this library, that has to be done manually.
Now that I think about it, you’ll most likely need to split this PR into two: one for api-contracts so it can be published independently, and another for the remaining libraries to update their version constraints accordingly.
There was a problem hiding this comment.
All of the changes are backward compatible, which means that we will update minor versions. We know the current version, so IMO we can update dependency ranges for all dependent packages in place within the same PR. Wouldn't it work?
| type InferSchemaInput, | ||
| type InferSuccessResponse, | ||
| type RouteContract, | ||
| } from '@lokalise/api-contracts' |
There was a problem hiding this comment.
Same comment about package json
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/app/fastify-api-contracts/package.json (1)
53-53:⚠️ Potential issue | 🟠 MajorReplace wildcard dependency version with a bounded range.
Line 53 sets
@lokalise/api-contractsto"*", which makes dev builds non-deterministic and can silently pull incompatible releases. Please pin or use a bounded semver range (e.g.^6.5.3) to keep CI and local behavior stable.Suggested change
- "@lokalise/api-contracts": "*", + "@lokalise/api-contracts": "^6.5.3",#!/bin/bash set -euo pipefail # 1) Find wildcard specs for this dependency across the repo rg -n '"@lokalise/api-contracts"\s*:\s*"\*"' --glob '**/package.json' # 2) Show all declared ranges for this dependency (package manifests) rg -n '"@lokalise/api-contracts"\s*:\s*"' --glob '**/package.json' # 3) Check whether lockfiles exist to understand current pinning behavior fd '^(pnpm-lock.yaml|package-lock.json|yarn.lock)$' -H🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/fastify-api-contracts/package.json` at line 53, Replace the wildcard dependency for "@lokalise/api-contracts" in packages/app/fastify-api-contracts/package.json with a bounded semver range (for example "^6.5.3") to avoid non-deterministic installs; locate the dependency entry for "@lokalise/api-contracts" in that package.json, change the version string from "*" to a specific range, then run the package manager install (pnpm/yarn/npm) to update the lockfile so CI and local builds remain deterministic.
🧹 Nitpick comments (4)
packages/app/fastify-api-contracts/src/types.ts (1)
22-28: Consider reusing this alias to remove duplicate handler type definitions.
FastifyPayloadHandlerFnandFastifyNoPayloadHandlerFncan be expressed viaFastifyHandlerMethodto keep one canonicalRouteHandlerMethodshape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/fastify-api-contracts/src/types.ts` around lines 22 - 28, Replace the duplicated handler type definitions by expressing FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn in terms of the existing FastifyHandlerMethod alias: update the type declarations for FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn to reference FastifyHandlerMethod<...> with the appropriate type parameters (ReplyType, BodyType, ParamsType, QueryType, HeadersType) so they share the canonical RouteHandlerMethod shape defined by FastifyHandlerMethod; locate the current FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn declarations and change them to use FastifyHandlerMethod instead of re-declaring RouteHandlerMethod/ FastifyContractRouteInterface.packages/app/api-contracts/src/route-contract/inferTypes.ts (1)
9-16: Comment mentionsContractNonJsonResponseTypewhich doesn't exist.The JSDoc comment on line 12 mentions
ContractNonJsonResponseTypebut the actual type used isTypedNonJsonResponse. Consider updating the comment for accuracy:/** * Maps sentinels to their effective Zod schema: * - TypedNonJsonResponse<S> → S (the inner schema) - * - ContractNoBodyType / ContractNonJsonResponseType → undefined + * - ContractNoBodyType → undefined * - z.Schema → preserved as-is */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/api-contracts/src/route-contract/inferTypes.ts` around lines 9 - 16, Update the JSDoc for the ToZodSchema type to accurately reflect the actual sentinel types used in the code: replace the incorrect mention of ContractNonJsonResponseType with the real sentinel TypedNonJsonResponse and ensure ContractNoBodyType and z.Schema are described correctly; reference the ToZodSchema type and the TypedNonJsonResponse sentinel so the comment matches the conditional mapping implemented in the type.packages/app/backend-http-client/src/client/sendByRouteContract.ts (1)
64-85: Multiple@ts-expect-errorcomments indicate type inference limitations.The "FixMe" annotations suggest these are known type inference gaps. While functional, these comments accumulate tech debt. Consider creating a tracking issue for improving the types if not already tracked.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/backend-http-client/src/client/sendByRouteContract.ts` around lines 64 - 85, The multiple `@ts-expect-error` comments around the calls to sendResourceChange and sendNonPayload (using method, path, params, responseSchema, options) hide real typing gaps; replace them by giving params a correct type or narrowing/casting it before use (e.g. define a RequestParams type with body, headers, queryParams and ensure the function signature or local variable uses that type, or add proper overloads for sendResourceChange/sendNonPayload) so you can remove the `@ts-expect-error` lines; if you can't fix it now, create a tracking issue and add a concise TODO referencing sendResourceChange and sendNonPayload to avoid accumulating tech debt.packages/app/backend-http-client/src/client/sendByRouteContract.spec.ts (1)
189-203: Contract defines 201 but mock returns 200.The contract specifies
responseSchemasByStatusCode: { 201: responseSchema }but the mock server returns status 200. While this doesn't break the test (validation passes because response body matches), it's inconsistent and could mask issues if status-code-specific response handling is later implemented.📝 Suggested fix
- await mockServer.forPost('/products').thenJson(200, { id: 21 }, JSON_HEADERS) + await mockServer.forPost('/products').thenJson(201, { id: 21 }, JSON_HEADERS)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/app/backend-http-client/src/client/sendByRouteContract.spec.ts` around lines 189 - 203, The test's contract expects a 201 response (responseSchemasByStatusCode: { 201: responseSchema }) but the mock server returns 200 via mockServer.forPost('/products').thenJson(200, ...), causing an inconsistency; update the mock or the contract so they match—either change mockServer.forPost('/products').thenJson to return 201 or add a 200 schema to responseSchemasByStatusCode—so sendByRouteContract validation and future status-specific logic are aligned.
🤖 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/app/backend-http-client/src/client/sendByRouteContract.spec.ts`:
- Around line 309-316: The test suite recreates the client each test via
buildClient but never closes it; add an afterEach that checks for and awaits the
client shutdown to prevent resource leaks (e.g., await client.close() or await
client.stop() if close() is not available). Update the same test file’s describe
block that uses buildClient and the client variable to perform cleanup in
afterEach (guard with if (client && typeof client.close === 'function') await
client.close()) so each test properly disposes the previous client before the
next runs.
- Around line 33-36: Tests create a new undici client each beforeEach via
buildClient assigned to client but never close the previous instance; modify the
test teardown to close the existing client to avoid leaking sockets—either call
client.close() (or await client.close() if async) at the start of beforeEach
before reassigning, or add an afterEach/afterAll hook that checks if client is
set and closes it; update references to client and buildClient in
sendByRouteContract.spec.ts accordingly and ensure mockServer.reset remains
called.
In `@packages/app/frontend-http-client/src/sendByRouteContract.spec.ts`:
- Around line 305-320: The test has a mismatch between the declared TypeScript
type (expectTypeOf(result).toEqualTypeOf<undefined>()) and the runtime assertion
(expect(result).toBeNull()) for the ContractNoBody case; update the assertion in
the spec to match the declared type by replacing the runtime check with
expect(result).toBeUndefined() (or alternatively, if you prefer null semantics,
change the ContractNoBody/type in sendByRouteContract to return null and update
the type assertion to <null>), locating the test around the sendByRouteContract
call that uses defineRouteContract and ContractNoBody in
sendByRouteContract.spec.ts.
In `@packages/app/frontend-http-client/src/sendByRouteContract.ts`:
- Around line 77-79: The function currently returns null when responseSchema ===
ContractNoBody, causing a runtime/type mismatch because InferSuccessResponse for
ContractNoBody expects undefined; update the runtime to return undefined instead
of null in the responseSchema === ContractNoBody branch (in
sendByRouteContract's handling of ContractNoBody) so the returned value matches
the declared types, and adjust any tests that asserted null to expect undefined.
In `@packages/app/frontend-http-client/src/utils/headersUtils.ts`:
- Around line 3-4: The resolveHeaders implementation applies the nullish
fallback to the Promise when headers is an async factory; change resolveHeaders
to await the factory result before applying ?? {} so undefined resolutions fall
back correctly. Specifically, update the resolveHeaders function (symbol:
resolveHeaders, types: HeadersSource, HeadersObject) to obtain the value via
"const value = typeof headers === 'function' ? await headers() : headers" (or
use Promise.resolve(headers()).then(...)) and then "return value ?? {}", and
mark the function async (or ensure it always returns a resolved Promise) so
callers still get a HeadersObject or Promise<HeadersObject>.
---
Duplicate comments:
In `@packages/app/fastify-api-contracts/package.json`:
- Line 53: Replace the wildcard dependency for "@lokalise/api-contracts" in
packages/app/fastify-api-contracts/package.json with a bounded semver range (for
example "^6.5.3") to avoid non-deterministic installs; locate the dependency
entry for "@lokalise/api-contracts" in that package.json, change the version
string from "*" to a specific range, then run the package manager install
(pnpm/yarn/npm) to update the lockfile so CI and local builds remain
deterministic.
---
Nitpick comments:
In `@packages/app/api-contracts/src/route-contract/inferTypes.ts`:
- Around line 9-16: Update the JSDoc for the ToZodSchema type to accurately
reflect the actual sentinel types used in the code: replace the incorrect
mention of ContractNonJsonResponseType with the real sentinel
TypedNonJsonResponse and ensure ContractNoBodyType and z.Schema are described
correctly; reference the ToZodSchema type and the TypedNonJsonResponse sentinel
so the comment matches the conditional mapping implemented in the type.
In `@packages/app/backend-http-client/src/client/sendByRouteContract.spec.ts`:
- Around line 189-203: The test's contract expects a 201 response
(responseSchemasByStatusCode: { 201: responseSchema }) but the mock server
returns 200 via mockServer.forPost('/products').thenJson(200, ...), causing an
inconsistency; update the mock or the contract so they match—either change
mockServer.forPost('/products').thenJson to return 201 or add a 200 schema to
responseSchemasByStatusCode—so sendByRouteContract validation and future
status-specific logic are aligned.
In `@packages/app/backend-http-client/src/client/sendByRouteContract.ts`:
- Around line 64-85: The multiple `@ts-expect-error` comments around the calls to
sendResourceChange and sendNonPayload (using method, path, params,
responseSchema, options) hide real typing gaps; replace them by giving params a
correct type or narrowing/casting it before use (e.g. define a RequestParams
type with body, headers, queryParams and ensure the function signature or local
variable uses that type, or add proper overloads for
sendResourceChange/sendNonPayload) so you can remove the `@ts-expect-error` lines;
if you can't fix it now, create a tracking issue and add a concise TODO
referencing sendResourceChange and sendNonPayload to avoid accumulating tech
debt.
In `@packages/app/fastify-api-contracts/src/types.ts`:
- Around line 22-28: Replace the duplicated handler type definitions by
expressing FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn in terms of the
existing FastifyHandlerMethod alias: update the type declarations for
FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn to reference
FastifyHandlerMethod<...> with the appropriate type parameters (ReplyType,
BodyType, ParamsType, QueryType, HeadersType) so they share the canonical
RouteHandlerMethod shape defined by FastifyHandlerMethod; locate the current
FastifyPayloadHandlerFn and FastifyNoPayloadHandlerFn declarations and change
them to use FastifyHandlerMethod instead of re-declaring RouteHandlerMethod/
FastifyContractRouteInterface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a7daac91-3abe-4e10-8050-2fd2f3b4c008
📒 Files selected for processing (31)
packages/app/api-contracts/README.mdpackages/app/api-contracts/src/HttpStatusCodes.tspackages/app/api-contracts/src/contractBuilder.tspackages/app/api-contracts/src/index.tspackages/app/api-contracts/src/rest/restContractBuilder.tspackages/app/api-contracts/src/route-contract/defineRouteContract.spec.tspackages/app/api-contracts/src/route-contract/defineRouteContract.tspackages/app/api-contracts/src/route-contract/inferTypes.spec.tspackages/app/api-contracts/src/route-contract/inferTypes.tspackages/app/api-contracts/src/sse/sseContractBuilders.tspackages/app/backend-http-client/README.mdpackages/app/backend-http-client/src/client/httpClient.tspackages/app/backend-http-client/src/client/sendByRouteContract.spec.tspackages/app/backend-http-client/src/client/sendByRouteContract.tspackages/app/backend-http-client/src/index.tspackages/app/fastify-api-contracts/README.mdpackages/app/fastify-api-contracts/package.jsonpackages/app/fastify-api-contracts/src/defineFastifyRoute.spec.tspackages/app/fastify-api-contracts/src/defineFastifyRoute.tspackages/app/fastify-api-contracts/src/fastifyRouteBuilder.tspackages/app/fastify-api-contracts/src/index.tspackages/app/fastify-api-contracts/src/injectByContract.tspackages/app/fastify-api-contracts/src/injectByRouteContract.tspackages/app/fastify-api-contracts/src/types.tspackages/app/frontend-http-client/README.mdpackages/app/frontend-http-client/src/client.tspackages/app/frontend-http-client/src/index.tspackages/app/frontend-http-client/src/sendByRouteContract.spec.tspackages/app/frontend-http-client/src/sendByRouteContract.tspackages/app/frontend-http-client/src/sse.tspackages/app/frontend-http-client/src/utils/headersUtils.ts
packages/app/backend-http-client/src/client/sendByRouteContract.spec.ts
Outdated
Show resolved
Hide resolved
packages/app/frontend-http-client/src/sendByRouteContract.spec.ts
Outdated
Show resolved
Hide resolved
|
|
||
| export type CommonRouteContract = { | ||
| // biome-ignore lint/suspicious/noExplicitAny: Required for compatibility with generics | ||
| pathResolver: RoutePathResolver<any> |
There was a problem hiding this comment.
does this mean we lose type-safety for manual execution of pathResolver on the contract?
There was a problem hiding this comment.
It will be fully typed, because contracts are defined using TypedPathRouteContract
https://github.com/lokalise/shared-ts-libs/pull/890/changes#diff-7588d197d143b886c42286f074f73edd8f5f855a9396b022485e8ba0328e1cf5R49
Here is some more info #890 (comment)
| responseHeaderSchema?: z.ZodType | ||
| responseSchemasByStatusCode: ResponseSchemasByStatusCode | ||
|
|
||
| metadata?: Record<string, unknown> |
There was a problem hiding this comment.
do I understand correctly that we lose customizability here, users no longer can augment this type globally?
There was a problem hiding this comment.
We're not losing anything, the existing contract implementation is identical (but I just inlined the type)
export interface CommonRouteDefinitionMetadata extends Record<string, unknown> {}
export type CommonRouteDefinition<
...
metadata?: CommonRouteDefinitionMetadata
}
3 participants
Changes
Summary
defineRouteContract— a new way to define contracts that unifies REST and SSE use-casessendByRouteContractto bothbackend-http-clientandfrontend-http-client, andinjectByRouteContract+defineFastifyRoutetofastify-api-contracts, so the same contract drives HTTPclients and Fastify route registration end-to-end
inferTypeshelpers (InferSuccessResponse,InferSuccessSchema,HasAnyNonJsonSuccessResponse) for consuming contract response types in application code; introducesContractNoBody/ContractNonJsonResponsesymbols for non-JSON and empty-body scenariosbuildContract/buildRestContract,buildFastifyRoute, andinjectByContractin favour of the new route-contract-first pattern; updates all three package READMEs accordinglyOld vs new contract definition
The old
buildContract/buildRestContractAPI threaded every aspect of the route through explicit generic type parameters. A typical payload route required up to 9 type parameters just to get full inference:The response type lived on a single successResponseBodySchema field with no per-status-code granularity, and flags like isEmptyResponseExpected / isNonJSONResponseExpected had to be specified as both a runtime value and a boolean generic to keep inference correct.
The new defineRouteContract drops all explicit generics. Types are inferred entirely from the shape of the object you pass in — the same DX, with significantly less noise:
// New
Per-status-code responses are first-class, and ContractNoBody / ContractNonJsonResponse symbols replace the boolean flags entirely — no generics, no duplication.
▎ Note: The new API achieves the same level of type safety without being heavily dependent on explicit generics. This results in cleaner contract definitions that are easier to read, write, and maintain, while TypeScript still infers everything correctly.
Checklist
major,minor,patchorskip-releaseAI Assistance Tracking
We're running a metric to understand where AI assists our engineering work. Please select exactly one of the options below:
Mark "Yes" if AI helped in any part of this work, for example: generating code, refactoring, debugging support,
explaining something, reviewing an idea, or suggesting an approach.