Default input typing for routes to use void not unknown#1136
Default input typing for routes to use void not unknown#1136treehill05 wants to merge 4 commits into
Conversation
Signed-off-by: Henry Kwon <henry@lyra.so>
Signed-off-by: Henry Kwon <henry@lyra.so>
Signed-off-by: Henry Kwon <henry@lyra.so>
dosubot
Bot
added
the
size:L
|
@treehill05 is attempting to deploy a commit to the unnoq-team Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughBuilder singletons (oc and os) now have their input schema type narrowed from Schema<unknown, unknown> to Schema<void, void>, establishing void as the default input type. Comprehensive type-level and runtime tests verify void input inference and behavior. Playground routes demonstrate practical void input usage patterns. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes apply a consistent pattern across multiple files (type narrowing and comprehensive test coverage), but require verification of type inference correctness across four test files and playground integration logic. Possibly related issues
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 @treehill05, 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 introduces a significant refinement to the default input typing for routes within the framework, changing it from Highlights
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement, changing the default input type for routes from unknown to void. This enhances type safety and provides a better developer experience by making the absence of an input explicit. The change is well-supported by a comprehensive suite of new runtime and type-level tests, which is excellent. I've identified a couple of minor areas for improvement in the test code and a playground component to enhance clarity and remove redundancy.
| it('should accept no input when input() is not specified', async () => { | ||
| const procedure = os | ||
| .output(z.string()) | ||
| .handler(() => 'result') | ||
|
|
||
| // Should work without providing input | ||
| const result = await procedure['~orpc'].handler({ | ||
| context: {}, | ||
| input: undefined, | ||
| rawInput: undefined, | ||
| }) | ||
|
|
||
| expect(result).toBe('result') | ||
| }) | ||
|
|
||
| it('should accept undefined as input when input() is not specified', async () => { | ||
| const procedure = os | ||
| .output(z.string()) | ||
| .handler(() => 'result') | ||
|
|
||
| // Should work with undefined input | ||
| const result = await procedure['~orpc'].handler({ | ||
| context: {}, | ||
| input: undefined, | ||
| rawInput: undefined, | ||
| }) | ||
|
|
||
| expect(result).toBe('result') | ||
| }) |
There was a problem hiding this comment.
These two test cases, 'should accept no input when input() is not specified' and 'should accept undefined as input when input() is not specified', are functionally identical. Both tests use input: undefined in the handler call, effectively testing the same scenario. To improve conciseness and remove redundancy, they can be merged into a single test.
| it('should accept no input when input() is not specified', async () => { | |
| const procedure = os | |
| .output(z.string()) | |
| .handler(() => 'result') | |
| // Should work without providing input | |
| const result = await procedure['~orpc'].handler({ | |
| context: {}, | |
| input: undefined, | |
| rawInput: undefined, | |
| }) | |
| expect(result).toBe('result') | |
| }) | |
| it('should accept undefined as input when input() is not specified', async () => { | |
| const procedure = os | |
| .output(z.string()) | |
| .handler(() => 'result') | |
| // Should work with undefined input | |
| const result = await procedure['~orpc'].handler({ | |
| context: {}, | |
| input: undefined, | |
| rawInput: undefined, | |
| }) | |
| expect(result).toBe('result') | |
| }) | |
| it('should accept undefined as input when input() is not specified', async () => { | |
| const procedure = os | |
| .output(z.string()) | |
| .handler(() => 'result') | |
| // Should work with undefined input | |
| const result = await procedure['~orpc'].handler({ | |
| context: {}, | |
| input: undefined, | |
| rawInput: undefined, | |
| }) | |
| expect(result).toBe('result') | |
| }) |
| const { mutate: testMutate } = useMutation(orpc.ping.run.mutationOptions()) | ||
| const { mutate: testVoidMutate } = useMutation(orpc.ping.runVoid.mutationOptions()) | ||
|
|
||
| useCallback(() => { | ||
| testMutate() // this will throw an error if z.void() is not default of input schema | ||
| testMutate(undefined) // this will not throw an error | ||
| testVoidMutate() // this will not throw an error | ||
| }, [testMutate, testVoidMutate]) |
There was a problem hiding this comment.
This useCallback and the associated useMutation hooks appear to be for compile-time type checking and have no effect at runtime. This type of check is valuable but belongs in dedicated type-test files (.test-d.ts) rather than in a component file, where it can be confusing and is effectively dead code. The new .test-d.ts files in this PR already provide excellent coverage for these type-level behaviors.
Removing this block would also allow for the removal of the useCallback import on line 5.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
playgrounds/next/src/app/orpc-mutation.tsx (1)
5-5: Unused import.The
useCallbackimport is only used for an unused callback (lines 27-31). Consider removing this import if the callback is not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/contract/src/builder.ts(1 hunks)packages/contract/src/default-void-input.test-d.ts(1 hunks)packages/contract/src/default-void-input.test.ts(1 hunks)packages/server/src/builder.ts(1 hunks)packages/server/src/default-void-input.test-d.ts(1 hunks)packages/server/src/default-void-input.test.ts(1 hunks)playgrounds/next/src/app/orpc-mutation.tsx(2 hunks)playgrounds/next/src/routers/index.ts(2 hunks)playgrounds/next/src/routers/ping.ts(1 hunks)playgrounds/next/src/schemas/ping.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/server/src/default-void-input.test-d.ts (1)
packages/server/src/builder.ts (2)
os(336-352)middleware(146-150)
packages/server/src/builder.ts (1)
packages/contract/src/schema.ts (1)
Schema(5-5)
playgrounds/next/src/routers/index.ts (1)
playgrounds/next/src/routers/ping.ts (2)
ping(4-4)pingVoid(5-5)
packages/contract/src/default-void-input.test-d.ts (1)
packages/contract/src/builder.ts (1)
oc(189-198)
packages/contract/src/default-void-input.test.ts (2)
packages/contract/src/builder.ts (1)
oc(189-198)packages/contract/src/procedure.ts (1)
isContractProcedure(51-66)
packages/contract/src/builder.ts (1)
packages/contract/src/schema.ts (1)
Schema(5-5)
packages/server/src/default-void-input.test.ts (1)
packages/server/src/builder.ts (2)
os(336-352)middleware(146-150)
playgrounds/next/src/app/orpc-mutation.tsx (1)
playgrounds/next/src/lib/orpc.ts (1)
orpc(32-32)
playgrounds/next/src/routers/ping.ts (1)
playgrounds/next/src/schemas/ping.ts (2)
PingSchema(3-3)PingVoidSchema(4-4)
🔇 Additional comments (9)
playgrounds/next/src/schemas/ping.ts (1)
1-4: Clean schema definitions.The schemas are well-defined with descriptive messages. The explicit use of
z.void()inPingVoidSchemaprovides clarity even thoughvoidis now the default input type.playgrounds/next/src/routers/index.ts (1)
2-2: LGTM!The new
pingrouter follows the established pattern and cleanly integrates the ping endpoints into the router structure.Also applies to: 21-25
playgrounds/next/src/routers/ping.ts (1)
1-5: Clean implementation demonstrating void input defaults.The two endpoints effectively demonstrate the PR's core functionality:
pingrelies on the new void input default (no explicit input schema)pingVoidexplicitly declares void input for comparisonBoth handlers are simple and appropriate for playground/demo purposes.
packages/server/src/default-void-input.test-d.ts (1)
1-86: Excellent type-level test coverage for void input behavior.The test suite comprehensively validates the default void input type at the type level, covering:
- Inference when
input()is omitted- Equivalence between implicit and explicit
z.void()- Override capability with explicit schemas
- Compatibility with middleware and router patterns
packages/server/src/builder.ts (1)
336-352: LGTM - Type signature correctly updated to default void input.The change from
Schema<unknown, unknown>toSchema<void, void>for the input schema type parameter establishes void as the default, which better represents "no input" semantics. The change is type-only, leaving runtime behavior intact, and is thoroughly validated by the comprehensive test suite.packages/contract/src/builder.ts (1)
189-198: LGTM - Contract builder type signature updated consistently.This change mirrors the server builder update, establishing
Schema<void, void>as the default input type for the contract builder. The consistency across packages ensures uniform behavior and is validated by comprehensive contract-specific tests.packages/contract/src/default-void-input.test-d.ts (1)
1-85: Comprehensive type-level validation for contract void input.The test suite thoroughly validates void input behavior at the contract level, covering all key scenarios including metadata, routing, and error mapping. The use of optional
inputSchema?in type extraction is correct for contract procedures.packages/contract/src/default-void-input.test.ts (1)
1-76: Thorough runtime validation of contract void input behavior.The test suite validates that contracts function correctly at runtime with void input defaults, covering procedure validity, schema properties, and integration with metadata, routing, and error mapping. Complements the type-level tests effectively.
packages/server/src/default-void-input.test.ts (1)
1-119: Excellent runtime validation of server void input behavior.The test suite thoroughly validates that procedures execute correctly with void input defaults, covering:
- Handler invocation without input parameters
- Compatibility with
undefinedinput values- Explicit void vs. implicit void equivalence
- Middleware chaining preservation
- Mixed router scenarios (no-input and with-input procedures)
Signed-off-by: Henry Kwon <henry@lyra.so>
|
Reverted playground commit. |
|
Thanks @treehill05 for the effort! I believe this would be a breaking change, so we can't accept a PR like this at the moment. Plus, your approach also doesn't seem fully safe, since it still allows the client to send any input and the server won't validate or strip that input. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
More templates
@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-publisher
@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-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: |
|
Thanks for your feedback, I agree on your point regarding security. Closing this PR for a safer approach. |
2 participants
Implementation of #1135
packages/server/src/builder.ts(line 339)packages/contract/src/builder.ts(line 190)Testing
✅ All existing tests pass
✅ Added new tests covering runtime and type-level behavior
✅ Verified in playground environments (Next.js)
Summary by CodeRabbit
New Features
Tests