feat(nest, contract): prefer import populateContractRouterPaths from @orpc/contract
#1312
Conversation
…@orpc/contract Fixes #1311
📝 WalkthroughWalkthroughThe PR relocates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ 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 |
Summary of ChangesHello @unnoq, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the populateContractRouterPaths utility, moving it from the @orpc/nest package to @orpc/contract for better modularity. The changes include moving the function implementation, its types, and tests to the contract package, and updating documentation and re-exports in the nest package to ensure backward compatibility with a deprecation notice. The implementation of the new utility is sound, but I've provided a couple of suggestions in packages/contract/src/router-utils.ts to improve type safety and robustness.
| export function populateContractRouterPaths<T extends AnyContractRouter>(router: T, options: PopulateContractRouterPathsOptions = {}): PopulatedContractRouterPaths<T> { | ||
| const path = toArray(options.path) | ||
|
|
||
| if (isContractProcedure(router)) { | ||
| if (router['~orpc'].route.path === undefined) { | ||
| return new ContractProcedure({ | ||
| ...router['~orpc'], | ||
| route: { | ||
| ...router['~orpc'].route, | ||
| path: toHttpPath(path), | ||
| }, | ||
| }) as any | ||
| } | ||
|
|
||
| return router as any | ||
| } | ||
|
|
||
| const populated: Record<string, any> = {} | ||
|
|
||
| for (const key in router) { | ||
| populated[key] = populateContractRouterPaths(router[key]!, { ...options, path: [...path, key] }) | ||
| } | ||
|
|
||
| return populated as any | ||
| } |
There was a problem hiding this comment.
The function populateContractRouterPaths uses as any in three places (lines 125, 128, 137), which undermines TypeScript's type safety. While complex recursive types can make strict typing difficult, it's worth exploring alternatives to any to prevent potential runtime bugs. For example, on line 128, return router; might work without a cast. In other places, a more specific cast like as PopulatedContractRouterPaths<T> would be safer than any.
| for (const key in router) { | ||
| populated[key] = populateContractRouterPaths(router[key]!, { ...options, path: [...path, key] }) | ||
| } |
There was a problem hiding this comment.
The for...in loop iterates over all enumerable properties of an object, including those on its prototype chain. To ensure you're only processing properties of the router object itself, it's a good practice to include a hasOwnProperty check.
| for (const key in router) { | |
| populated[key] = populateContractRouterPaths(router[key]!, { ...options, path: [...path, key] }) | |
| } | |
| for (const key in router) { | |
| if (Object.prototype.hasOwnProperty.call(router, key)) { | |
| populated[key] = populateContractRouterPaths(router[key]!, { ...options, path: [...path, key] }) | |
| } | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/client
@orpc/contract
@orpc/experimental-durable-iterator
@orpc/hey-api
@orpc/interop
@orpc/json-schema
@orpc/nest
@orpc/openapi
@orpc/openapi-client
@orpc/otel
@orpc/experimental-pino
@orpc/experimental-publisher
@orpc/experimental-publisher-durable-object
@orpc/experimental-ratelimit
@orpc/react
@orpc/react-query
@orpc/experimental-react-swr
@orpc/server
@orpc/shared
@orpc/solid-query
@orpc/standard-server
@orpc/standard-server-aws-lambda
@orpc/standard-server-fastify
@orpc/standard-server-fetch
@orpc/standard-server-node
@orpc/standard-server-peer
@orpc/svelte-query
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/vue-colada
@orpc/vue-query
@orpc/zod
commit: |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nest/src/utils.test.ts (1)
3-10: Duplicate test case on lines 4 and 5.Both lines test the same input
'/ping'with the same expected output. Consider removing one or replacing it with a different test case for better coverage.🔎 Proposed fix
it('toNestPattern', () => { expect(toNestPattern('/ping')).toBe('/ping') - expect(toNestPattern('/ping')).toBe('/ping') expect(toNestPattern('/{id}')).toBe('/:id') expect(toNestPattern('/{id}/{+path}')).toBe('/:id/*path') expect(toNestPattern('/{id}/name{name}')).toBe('/:id/name{name}') })packages/contract/src/router-utils.test.ts (1)
77-101: Good test coverage for the new utility.The tests appropriately verify path preservation for procedures with explicit paths and auto-population for those without. Consider adding a test case for the
options.pathparameter to verify custom prefix functionality:// Test with custom path prefix const populatedWithPrefix = populateContractRouterPaths(contract, { path: ['api', 'v1'] }) expect(populatedWithPrefix.ping['~orpc'].route.path).toBe('/api/v1/ping')
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/content/docs/openapi/integrations/implement-contract-in-nest.mdpackages/contract/src/router-utils.test-d.tspackages/contract/src/router-utils.test.tspackages/contract/src/router-utils.tspackages/nest/src/implement.tspackages/nest/src/index.tspackages/nest/src/utils.test-d.tspackages/nest/src/utils.test.tspackages/nest/src/utils.ts
💤 Files with no reviewable changes (1)
- packages/nest/src/utils.test-d.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/contract/src/router-utils.ts (5)
packages/nest/src/index.ts (3)
PopulatedContractRouterPaths(25-25)PopulateContractRouterPathsOptions(21-21)populateContractRouterPaths(15-15)packages/contract/src/router.ts (1)
AnyContractRouter(17-17)packages/shared/src/array.ts (1)
toArray(1-3)packages/contract/src/procedure.ts (1)
isContractProcedure(51-66)packages/client/src/adapters/standard/utils.ts (1)
toHttpPath(6-8)
packages/contract/src/router-utils.test-d.ts (3)
packages/contract/src/router-utils.ts (1)
PopulatedContractRouterPaths(95-100)packages/contract/src/builder.ts (2)
router(198-200)oc(203-212)packages/contract/src/procedure.ts (1)
ContractProcedure(25-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: publish-commit
- GitHub Check: lint
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
packages/nest/src/utils.ts (1)
1-8: LGTM!The utility file is appropriately simplified. The
toNestPatternfunction is retained for NestJS-specific path conversion, while thepopulateContractRouterPathsutility has been correctly relocated to@orpc/contractto avoid bundling NestJS dependencies in frontend code.packages/nest/src/implement.ts (1)
48-53: LGTM!The error message correctly directs users to the new import location for
populateContractRouterPaths, aligning with the PR's objective to prefer imports from@orpc/contract.apps/content/docs/openapi/integrations/implement-contract-in-nest.md (1)
67-67: LGTM!The documentation correctly demonstrates importing
populateContractRouterPathsfrom@orpc/contractalongsideoc, which aligns with the PR's objective and provides clear guidance to users.packages/nest/src/index.ts (1)
11-26: LGTM!Excellent approach to maintaining backward compatibility. The deprecated re-exports allow existing consumers to continue working while the deprecation notices clearly guide them to migrate to the new import path from
@orpc/contract.packages/contract/src/router-utils.test-d.ts (1)
50-68: LGTM!Good type-level test coverage for
PopulatedContractRouterPaths. The tests verify that the type correctly preserves the procedure's type parameters (input/output schemas, error map, meta) through the transformation.packages/contract/src/router-utils.ts (2)
95-138: LGTM!The implementation is well-structured and follows the established patterns in this file (e.g.,
enhanceContractRouter,minifyContractRouter). The recursive approach correctly handles nested routers, preserves existing paths when present, and only populates missing paths. The type definition properly preserves procedure type parameters through the transformation.The JSDoc comments provide clear documentation and link to the relevant NestJS integration docs.
5-6: Dependency is correctly declared.The
@orpc/clientdependency is properly declared inpackages/contract/package.jsonas"@orpc/client": "workspace:*", so there are no resolution issues.
Deploying orpc with
|
| Latest commit: |
ff37597
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f1943be5.orpc-1qh.pages.dev |
| Branch Preview URL: | https://unnoq-issue1311.orpc-1qh.pages.dev |
Fixes #1311
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.