-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: prefer Standard Schema for input/output type inference #6888
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Since it is the new and standard thing, we should use Standard Schema to extract input and output types whenever the schema implements it. Fixes trpc#6887
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@dzhu is attempting to deploy a commit to the trpc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughReorders type-level parser inference to prefer Standard Schema-like parsers first, then ParserWithInputOutput, then ParserWithoutInput; adds a zod-branded Changes
Sequence Diagram(s)sequenceDiagram
participant Call as inferParser<TParser>
participant Types as Parser union
Call->>Types: check TParser type
alt ParserStandardSchemaEsque<TIn,TOut>
Types-->>Call: { in: TIn, out: TOut }
else ParserWithInputOutput<TIn,TOut>
Types-->>Call: { in: TIn, out: TOut }
else ParserWithoutInput<T>
Types-->>Call: { in: T, out: T }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
⏰ 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). (17)
🔇 Additional comments (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
www/docs/server/validators.md (3)
8-8: Tighten phrasing and hyphenationSmall clarity/style tweak and hyphenation:
- “first class support” → “first-class support”
- Consider splitting the long sentence for readability.
- tRPC procedures may define validation logic for their input and/or output, and validators are also used to infer the types of inputs and outputs (using the [Standard Schema](https://standardschema.dev) interface if available, or custom interfaces for supported validators if not). We have first class support for many popular validators, and you can [integrate validators](#contributing-your-own-validator-library) which we don't directly support. + tRPC procedures may define validation logic for their input and/or output. Validators are also used to infer the types of inputs and outputs (using the [Standard Schema](https://standardschema.dev) interface if available, or custom interfaces for supported validators if not). We have first-class support for many popular validators, and you can [integrate validators](#contributing-your-own-validator-library) which we don't directly support.
172-172: Specify the version: “Standard Schema v1”Since the code targets v1, make it explicit to reduce ambiguity.
- including any library conforming to [Standard Schema](https://standardschema.dev). + including any library conforming to [Standard Schema v1](https://standardschema.dev).
498-498: Tone/style: avoid “Feel free to” and clarifyConsider a more direct tone and minor rewording:
- Conforming to [Standard Schema](https://standardschema.dev) is recommended, but in some cases we may accept a PR to add a new supported interface. Feel free to open an issue for discussion. + Conforming to [Standard Schema](https://standardschema.dev) is recommended. In some cases, we may accept a PR to add a new supported interface. You can open an issue to discuss your use case.packages/tests/server/validators.test.ts (1)
938-942: Nice addition: direct branded scalar route coveredThis validates Standard Schema-based inference for scalar inputs. Consider also asserting router-level types for
topto mirror thenumchecks:type RouterInput = inferRouterInputs<typeof router>; type RouterOutput = inferRouterOutputs<typeof router>; type AccountIdInput = RouterInput['num']['accountId']; // ^? type AccountIdOutput = RouterOutput['num']['accountId']; // ^? expectTypeOf<AccountIdInput>().toEqualTypeOf<Types['input']>(); expectTypeOf<AccountIdOutput>().toEqualTypeOf<Types['output']>(); + + // Additional coverage for the scalar route: + type TopInput = RouterInput['top']; + // ^? + type TopOutput = RouterOutput['top']; + // ^? + expectTypeOf<TopInput>().toEqualTypeOf<Types['input']>(); + expectTypeOf<TopOutput>().toEqualTypeOf<Types['output']>();This strengthens the guarantee that the public API reflects the Standard Schema types for both object-wrapped and direct scalar forms.
packages/server/src/unstable-core-do-not-import/parser.ts (1)
65-81: Prefer Standard Schema ininferParser: correct precedenceGood change. This ensures dual-interface parsers (e.g., Zod v4) infer via Standard Schema first, avoiding stale or incorrect internals.
Consider adding a brief doc comment to lock in the intent and reduce future regressions:
export type inferParser<TParser extends Parser> = + /** + * Precedence: + * 1) Standard Schema v1 (canonical input/output) + * 2) Parsers exposing explicit input/output internals + * 3) Parsers with single in/out type (custom fn, myzod, scale, superstruct, yup) + */ TParser extends ParserStandardSchemaEsque<infer $TIn, infer $TOut> ? { in: $TIn; out: $TOut } : TParser extends ParserWithInputOutput<infer $TIn, infer $TOut> ? { in: $TIn; out: $TOut } : TParser extends ParserWithoutInput<infer $InOut> ? { in: $InOut; out: $InOut } : never;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/server/src/unstable-core-do-not-import/parser.ts(1 hunks)packages/tests/server/validators.test.ts(1 hunks)www/docs/server/validators.md(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/tests/server/validators.test.ts (1)
packages/server/src/vendor/standard-schema-v1/spec.ts (1)
Types(62-67)
🪛 LanguageTool
www/docs/server/validators.md
[style] ~498-~498: Consider using a less common alternative to make your writing sound more unique and professional.
Context: ... a PR to add a new supported interface. Feel free to open an issue for discussion. You can c...
(FEEL_FREE_TO_STYLE_ME)
⏰ 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). (3)
- GitHub Check: Lint and auto-fix
- GitHub Check: test
- GitHub Check: Analyze (typescript)
|
|
||
| export type inferParser<TParser extends Parser> = | ||
| TParser extends ParserWithInputOutput<infer $TIn, infer $TOut> | ||
| TParser extends ParserStandardSchemaEsque<infer $TIn, infer $TOut> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! This is a great idea and the docs changes look good
In this case couldn't we change the precedence of ParserWithInputOutput to keep the conditional type simple?
export type ParserWithInputOutput<TInput, TParsedInput> =
| ParserStandardSchemaEsque<TInput, TParsedInput>
| ParserZodEsque<TInput, TParsedInput>
| ParserValibotEsque<TInput, TParsedInput>
| ParserArkTypeEsque<TInput, TParsedInput>;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried that, but it didn't work. My understanding, as a relative newcomer to TypeScript, is that there isn't actually precedence there; rather, the final type is the union of the outcomes for each individual branch.
For example, if you try to compile the following code, you should see an error that indicates that Get<{ a: number; b: string }> evaluates to string | number.
interface A<T> {
a: T;
}
type B<T> = {
b: T;
};
type U<T> = A<T> | B<T>;
type Get<X> = X extends U<infer $T> ? $T : never;
const x: Get<{ a: number; b: string }> = {};I think the same thing applies in the Zod case, but it's hidden because the branded type is a subtype of the base type, so the union just equals the base type. (If you brand string with "brand", the branded type is string & z.BRAND<"brand">.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I believe you're correct, thanks for clarifying!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it a bit, got rid of the ParserStandardSchemaEsque-type and just placed StandardSchemaV1 instead ofi t first
Edit: nvm, undid that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/parser.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)
packages/server/src/vendor/standard-schema-v1/spec.ts (1)
StandardSchemaV1(10-13)
🔇 Additional comments (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)
51-55: No external usage of ParserWithInputOutput detected—removal of Standard Schema is safeI’ve searched across the repo and found that
ParserWithInputOutputis only defined and used in packages/server/src/unstable-core-do-not-import/parser.ts (it isn’t imported or referenced elsewhere). RemovingStandardSchemaV1from that union won’t impact downstream code or public surfaces.
This reverts commit 155347f.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)
67-70: Including StandardSchemaV1 in Parser unblocks the first branch of inferParserBy adding StandardSchemaV1 to the Parser union, the Standard Schema branch in inferParser becomes reachable and preferred. This also addresses the prior “unreachable branch” concern raised earlier.
🧹 Nitpick comments (3)
packages/server/src/unstable-core-do-not-import/parser.ts (3)
72-88: Optional: remove the generic constraint to decouple inference from the Parser unioninferParser now works because Parser includes StandardSchemaV1. To make this robust against future changes to Parser (and to allow inference on any Standard Schema-implementing value even if it isn’t typed as Parser), consider dropping the extends Parser constraint. This keeps the precedence logic self-contained and avoids reintroducing the “unreachable branch” class of issues if Parser is refactored later.
Proposed minimal change:
-export type inferParser<TParser extends Parser> = +export type inferParser<TParser> = TParser extends StandardSchemaV1<infer $TIn, infer $TOut> ? { in: $TIn; out: $TOut; } : TParser extends ParserWithInputOutput<infer $TIn, infer $TOut> ? { in: $TIn; out: $TOut; } : TParser extends ParserWithoutInput<infer $InOut> ? { in: $InOut; out: $InOut; } : never;
49-51: Nit: capitalize and hyphenate “Standard Schema” in the deprecation noteFor consistency with the docs and naming elsewhere, prefer “non-Standard Schema”.
/** - * @deprecated we can drop support for non standard schema parsers in the next major version + * @deprecated We can drop support for non-Standard Schema parsers in the next major version */
59-61: Nit: same capitalization/hyphenation fix here/** - * @deprecated we can drop support for non standard schema parsers in the next major version + * @deprecated We can drop support for non-Standard Schema parsers in the next major version */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/parser.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)
packages/server/src/vendor/standard-schema-v1/spec.ts (1)
StandardSchemaV1(10-13)
⏰ 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: build
- GitHub Check: test
- GitHub Check: Lint and auto-fix
- GitHub Check: Analyze (typescript)
🔇 Additional comments (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)
62-66: Removing StandardSchema from ParserWithInputOutput is correct to enforce precedenceGood call excluding ParserStandardSchemaEsque from ParserWithInputOutput. This ensures Standard Schema inference happens via the dedicated first branch in inferParser, not conflated with other “with input/output” parsers.
This reverts commit 4b0db76.
@trpc/client
@trpc/next
@trpc/react-query
@trpc/server
@trpc/tanstack-react-query
@trpc/upgrade
commit: |
|
This pull request has been locked because we are very unlikely to see comments on closed issues. If you think, this PR is still necessary, create a new one with the same branch. Thank you. |
🎯 Changes
Since it is the new and standard thing, we should use Standard Schema to extract input and output types whenever the schema implements it.
Fixes #6887
✅ Checklist
toEqualTypeOfin a resolver definition like the other Zod branded type test, which doesn't seem to actually causepnpm testto signal failure if the equality is not satisfied, though it does show up as an error in a TypeScript-aware editor. I'm not sure if a failure there is actually being caught in any automatically visible way.)Summary by CodeRabbit