Fixing ApiContractMetadataToRouteMapper to make it more generic#888
Fixing ApiContractMetadataToRouteMapper to make it more generic#888CarlosGamero merged 2 commits intomainfrom
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:
📝 WalkthroughWalkthroughThe change modifies the Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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 |
| metadata: ApiContract['metadata'], | ||
| ) => Pick< | ||
| RouteType, | ||
| RouteOptions, |
There was a problem hiding this comment.
By using the generic Fastify RouteOptions, the type remains compatible with Opinionated Machine and likely with other libraries as well. If we use the custom RouteType, it introduces type issues.
For this case, both types are equivalent.
There was a problem hiding this comment.
we don't get in trouble with generics in this case?
IIRC original type comes with plenty of generics, although I might be wrong
There was a problem hiding this comment.
Yeah, those generics are actually the problem:
Here is the original type:
export type RouteType<
// biome-ignore lint/suspicious/noExplicitAny: It's ok
ReplyType = any,
// biome-ignore lint/suspicious/noExplicitAny: It's ok
BodyType = any,
// biome-ignore lint/suspicious/noExplicitAny: It's ok
ParamsType = any,
// biome-ignore lint/suspicious/noExplicitAny: It's ok
QueryType = any,
// biome-ignore lint/suspicious/noExplicitAny: It's ok
HeadersType = any,
> = RouteOptions<
RawServerDefault,
RawRequestDefaultExpression,
RawReplyDefaultExpression,
FastifyContractRouteInterface<ReplyType, BodyType, ParamsType, QueryType, HeadersType>,
// biome-ignore lint/suspicious/noExplicitAny: it's ok
any,
// biome-ignore lint/suspicious/noExplicitAny: it's ok
any,
// biome-ignore lint/suspicious/noExplicitAny: it's ok
any,
// biome-ignore lint/suspicious/noExplicitAny: it's ok
any
>
The problem is with FastifyContractRouteInterface, Opinionated machine is expecting RouteGenericInterface there.
For metadata, it is not important as we are not allowing to modify the types defined there
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?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)
packages/app/fastify-api-contracts/src/types.ts (1)
94-108:⚠️ Potential issue | 🔴 Critical
onRequestAbortis not a valid per-route hook in Fastify'sRouteOptions—remove it from the Pick list.
onRequestAbortcan only be registered viafastify.addHook()at the instance or plugin level, not as a per-route hook. The official Fastify documentation lists the valid per-route lifecycle hooks as:onRequest,preParsing,preValidation,preHandler,preSerialization,onSend,onResponse,onTimeout, andonError. Line 107 should be removed from the type definition.🤖 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 94 - 108, The Pick type that narrows RouteOptions incorrectly includes the non-route hook "onRequestAbort"; update the type in types.ts to remove 'onRequestAbort' from the Pick list so it only includes valid per-route hooks (e.g., 'onRequest','preParsing','preValidation','preHandler','preSerialization','onSend','onResponse','onTimeout','onError'), ensuring the exported type that references RouteOptions no longer lists 'onRequestAbort'.
🤖 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 `@packages/app/fastify-api-contracts/src/types.ts`:
- Around line 94-108: The Pick type that narrows RouteOptions incorrectly
includes the non-route hook "onRequestAbort"; update the type in types.ts to
remove 'onRequestAbort' from the Pick list so it only includes valid per-route
hooks (e.g.,
'onRequest','preParsing','preValidation','preHandler','preSerialization','onSend','onResponse','onTimeout','onError'),
ensuring the exported type that references RouteOptions no longer lists
'onRequestAbort'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: lokalise/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dd0e1dc-397f-429a-ad35-b6917e0f5654
📒 Files selected for processing (1)
packages/app/fastify-api-contracts/src/types.ts
2 participants
Changes
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.