Skip to content

Conversation

@fabian-hiller
Copy link
Contributor

@fabian-hiller fabian-hiller commented Oct 4, 2024

🎯 Changes

An union of schema library authors (Zod, Valibot, ArkType, ...) collaborating on a standard interface for schema libraries called Standard Schema. This simplifies implementation and prevents vendor lock-in. So far Valibot v1 supports this spec and Zod will follow soon. This PR adds support for all schema libraries that follow the v1 of this spec in the future.

In the long run, the explicit implementation of Valibot and Zod specific code can be removed from tRPC.

✅ Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

Summary by CodeRabbit

  • Dependencies

    • Updated valibot package to version 1.0.0-beta.14 across multiple packages
    • Introduced support for parallel versioning of valibot
  • Validation

    • Simplified input and output validation syntax
    • Removed v.parser() wrapper in validation methods
    • Added support for new schema validation approach
  • Error Handling

    • Introduced StandardSchemaV1Error for more structured error reporting
    • Enhanced error formatting capabilities
  • Documentation

    • Updated quickstart and server validation documentation to reflect new validation approach

@fabian-hiller fabian-hiller requested review from a team as code owners October 4, 2024 17:18
@vercel
Copy link

vercel bot commented Oct 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
www ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 25, 2025 11:43pm

@vercel
Copy link

vercel bot commented Oct 4, 2024

@fabian-hiller is attempting to deploy a commit to the trpc Team on Vercel.

A member of the Team first needs to authorize it.

const result = await parser['~validate']({ value });
if (result.issues) {
console.error('Schema issues:', result.issues);
throw new Error(result.issues[0].message);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there not be a StandardError or something? Having a generic error here makes it impossible to do errorFormatter like this one? https://trpc.io/docs/server/error-formatting#adding-custom-formatting

Can we attach all the issues on Error.cause or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added our SchemaError utility class. Does this look okay to you? Or should we add SchemaError directly to the source code and not import it from @standard-schema/utils?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea no deps and mports. Just a way to revive the full error with all the issues instead of just the first one

Copy link
Member

@KATT KATT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that y'all are converging at a standard way of doing things

We don't want third party dependencies in trpc though

Comment on lines 153 to 155
"dependencies": {
"@standard-schema/utils": "^0.1.0"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want dependencies.

Suggested change
"dependencies": {
"@standard-schema/utils": "^0.1.0"
}

Comment on lines 127 to 129
if (result.issues) {
throw new SchemaError(result.issues);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If result.reason was an Error-type, we could've just rethrown here

But for now I think we gotta do something like

if (result.issues) {
  throw new Error("Validation error", { cause: result.issues })
}

For us, it would be a bit simpler if there was an actual Error here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @juliusmarminge already pointed out, it'd be nice if it was something like

if (result.error) {
  throw result.error
}

tuples work well too

const [err, result] =  await parser['~validate'](value);

if (err) {
  throw err;
}
return result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think forcing schema libraries to use a specific error class to be compatible with Standard Schema is not a good idea, as it would unnecessarily compromise bundle size, performance (creating Error instances is expensive), and flexibility.

'~types'?: {
input: TInput;
output: TParsedInput;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have some different wip-branches where we do infer errors too, does ~types also have an Error-subclass?

In the case of zod, we could infer that pretty easily in #6027 where we played with this, see L12 here:

export interface ParserZodEsque<TInput, TParsedInput, TError> {
_input: TInput;
_output: TParsedInput;
safeParseAsync: (input: unknown) => Promise<
| {
success: true;
data: TParsedInput;
}
| {
success: false;
error: TError;
}
>;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of inferring the error type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think e.g. a form submission - it'd be nice to infer the right path when rendering the errors

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the error class for this?

Copy link
Member

@juliusmarminge juliusmarminge Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cause there's no way to get the type of an error cause, so if you'd to throw new Error("", { cause: issues }) you'd need a cast like (err.cause as SchemaIssues<TSchema>).fieldErrors.email for example. Or how would you infer here?

Would be nice if something strictly typed came out of the validate method directly as a built-in we could re-throw

Copy link
Member

@juliusmarminge juliusmarminge Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the example! I understand it much better now. I think this is also possible with Standard Schema. We know the input and output type, so we know all possible paths that can occur, and can transform them into an object of dot paths. I can provide example code.

Copy link
Contributor Author

@fabian-hiller fabian-hiller Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is basically to infer the input type from the schema. Standard Schema provides an InferInput util type for this. Then we could use a modified version of ts-essentials' Paths type, which converts the input type into a union of all possible paths. Finally, we pass this type to Record (or a custom mapped type) to construct the FieldErrors type.

type FieldErrors = Record<Paths<InferInput<TSchema>>, string[]>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an object might not be the best idea as errors could occur on e.g both foo and foo.bar simultaneously - an array with issues and paths is probably best for a generic lib - you can supply functions to turn it into an object

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using an object might not be the best idea as errors could occur on e.g both foo and foo.bar simultaneously

You don't think this is a problem, or are you aware of an edge case? That's why I wrote that the Paths type needs to be modified. I can provide a raw implementation if you are interested.

an array with issues and paths is probably best for a generic lib

I completely agree. That's exactly what the Standard Schema does.

you can supply functions to turn it into an object

I have thought about providing such a function with @standard-schema/utils, but such an implementation requires a lot of thought to make it work for everyone. Some libraries are probably only interested in nested messages, because they assume (or know) that the root type is always valid. Others are also interested in errors related to the root schema. There may also be edge cases where a path cannot be converted to a dot path. Then there are libraries that need the value to be a string and others that need it to be a string[].

We are very interested in and grateful for feedback from libraries like tRPC to make the spec as perfect as possible.

@fabian-hiller
Copy link
Contributor Author

Does this PR look good to you, or should I change or modify anything?

@KATT KATT requested review from KATT and juliusmarminge October 9, 2024 20:30
@juliusmarminge
Copy link
Member

juliusmarminge commented Oct 9, 2024

how done is the spec? I notice it's sstill in beeta? Are you not open to returning some tuple-like thing from ~validate() so libs can rethrow the "proper thing"?

@fabian-hiller
Copy link
Contributor Author

I have been working with Colin and others on the interface and we think the spec is already in good shape, but at the end of the day we are in beta and Standard Schema only works if we can get everyone on board. The idea of the beta is to get feedback like yours and we are more than happy to discuss things and improve the spec together. I am also available for a call to figure things out.

KATT
KATT previously approved these changes Oct 14, 2024
@fabian-hiller
Copy link
Contributor Author

We will probably release a new beta version of the spec this week. I keep you posted.

@fabian-hiller
Copy link
Contributor Author

I updated the Standard Schema version and improved the error handling.

};
}

if ('~standard' in parser) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be sensible to move this higher up. All of the existing validation libraries won't actually reach here.

Now that standard-schema exists, the onus should be on validation libraries to implement it, and trpc's docs should slowly move towards "just use any standard-schema lib". At that point, most users will expect the parser['~standard'].validate(...) method to be used over parser.parse(value) or whatever. Hopefully there being any differences would be very rare, but still I think better to prioritise standard-schema.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be a breaking change though. since for example a validation failure on a zod schema would not throw ZodError when using ~standard.parse

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I put it at the end. Once Standard Schema is more established, tRPC may move it to the top when they release a new version.

@colinhacks
Copy link
Contributor

colinhacks commented Jan 22, 2025

Summarizing some discussion points for clarity/posterity, and because we'll be pointing to this PR as a model for other integrators 👍

1. Error subclass

We definitely don't want to require spec-compliant libraries to instantiate Errors for performance reasons. It's a huge hit to performance in the "validation failure" case and we don't want to impose a performance tax like that on everyone consuming spec-compliant validators.

2. Integration complexity

For us, it would be a bit simpler if there was an actual Error here

The hard part here is creating a spec that library authors will actually implement. It's expected that integrators like tRPC will need to do some massaging to fit the result of ~standard.validate() into their own systems. No way to please everyone so the spec is just trying to be the lowest common denominator.

3. Error formatting / inferrable errors

Given a compliant schema, you have an inferred type and you have the issue metadata (including paths for each issue). So you have all the ingredients you need to replicate Zod's current ZodError#format() and ZodError#flatten() logic if you like. Better yet, it would work for all spec-compliant schemas, not just Zod.

Here's what that logic looks like in Zod 3: https://github.com/colinhacks/zod/blob/f7ad26147ba291cb3fb257545972a8e00e767470/src/ZodError.ts#L221C3-L266C7

(To answer your other question @juliusmarminge, Zod 4 is pulling this logic out into separate formatError and flattenError functions for bundle size reasons but the functionality will still exist.)

As Fabian mentioned a flatten() utility could conceivably be implemented in the @standard-schema/utils package at some point. But this is entirely optional.

4. Dependencies

The spec is 100% types-only. In tRPC's case I recommend copying the text of the spec directly into your codebase. This is what I did with Zod.

You could just as easily as it as a dependency, and since it is a types-only package this would incur zero runtime overhead. But it would still exist in your package.json#dependencies and for zero-dependency projects like Zod and tRPC that would be unacceptable. Hence, copying the file into your project. We guarantee no breaking changes.

4. Zod 4 questions

if they migrate to StandardSchema will they lose that? That'd be a big loss

I am not sure what Zod's result.issues will look like when they adopt this standard interface

This is worth clarifying too: Zod 3.24 already implements the spec, and there were no changes made to ZodError or any part of Zod's API.

All of the functionality you're alluding to still exists. It's easy for something like Zod to implement this spec without changing it's own functionality. The behavior/signatures of Zod's first party .parse* methods can be made completely independent of the ~standard.validate() method. Does that making sense?

You can see Zod's implementation of the spec here: https://github.com/colinhacks/zod/blob/f7ad26147ba291cb3fb257545972a8e00e767470/src/types.ts#L287-L294

@csvn
Copy link

csvn commented Jan 23, 2025

Thanks for the update and great summary!

The PR looks pretty complete, what is there left to update here now? Is it to remove @standard-schema/spec as a devDependency?

Would be lovely to get this PR merged when it's ready, as working with valibot@latest (1.0.0-beta.14) has some issues with inferring Input/Output correctly due to the schema._types to schema['~types'] changes.

Not trying to put pressure, just curious. The changes here seems to make it much nicer to handle validation/parsing as well (without wrapping in the valibot.parse(schema) method), so just looking forward to them being in tRPC v11!

@fabian-hiller
Copy link
Contributor Author

.. as working with valibot@latest (1.0.0-beta.14) has some issues with inferring Input/Output correctly due to the schema._types to schema['~types'] changes.

Valibot v1 supports Standard Schema. There should be no bugs. And tRPC should still support our old implementation. But I can look into it. Maybe I am wrong. Feel free to give me feedback. Maybe I missed something.

Not trying to put pressure, just curious. The changes here seems to make it much nicer to handle validation/parsing as well (without wrapping in the valibot.parse(schema) method), so just looking forward to them being in tRPC v11!

Exactly 😎

@csvn
Copy link

csvn commented Jan 24, 2025

Valibot v1 supports Standard Schema. There should be no bugs. And tRPC should still support our old implementation. But I can look into it. Maybe I am wrong. Feel free to give me feedback. Maybe I missed something.

@fabian-hiller I don't want to hijack this PR, I described my issue in the section below.
It's a great idea for a shared API for validators, great initiative and thanks for all the hard work! 😍

Click to show

Issue

The issue I get is with [email protected] + @trpc/[email protected]. It seems that the client is using the "Output" type on the client instead of the "Input" type for queries/mutations.

// On the server
const querySchema = v.object({
  q: v.optional(v.string(), '')
});

export const appRouter = router({
  list: publicProcedure.input(v.parser(v.object({
    q: v.optional(v.string(), '')
  })))
});

export type QueryInput = v.InferInput<typeof querySchema>;
             // ^ { q?: string | undefined; }
export type QueryOutput = v.InferOutput<typeof querySchema>;
             // ^ { q: string; }
// On the client
trpc.list.query({});
             // ^ Property 'q' is missing in type '{}' but required in type '{ q: string; }'.  ts(2345)

// The type here should be the same as `QueryInput`, but it is the same as `QueryOutput`

The only issue I've had is when using optional() with a default value, which makes the Input/Output types differ. transform() may also cause similar issues.

Likely cause

I haven't looked too close at the issue (don't know how tRPC manages to infer the output type), but it looks to me like the ParserValibotEsque type is no longer matching the schema for the current Valibot latest version

image

For now, I got it working correctly by wrapping v.parser with a type adjustment, which makes tRPC able to correctly infer the Input type again:

publicProcedure
    .input(compat(v.parser(querySchema)))

function compat<T extends v.Parser<any, any>>(parser: T): T & {
  schema: {
    _types?: {
      input: v.InferInput<T['schema']>;
      output: v.InferOutput<T['schema']>;
    };
  }
} {
  return parser;
}

@fabian-hiller
Copy link
Contributor Author

fabian-hiller commented Jan 25, 2025

Yes, what you say may be true. This should be fixed once Standard Schema is supported. It will also allow you to remove the parser function. In the long run, we should probably remove the old Valibot specific code.

Copy link
Member

@KATT KATT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to see this PR get up to date and merged in general

If there's changes to the standard schema, I guess you can just add versioning too

The only tweak I'd want would be to vendor rather than importing the standard schema

@@ -1,3 +1,5 @@
import type { v1 } from '@standard-schema/spec';
Copy link
Member

@KATT KATT Jan 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please vendor this type instead of adding a dep

Suggested change
import type { v1 } from '@standard-schema/spec';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. I created a standardSchema.ts file. Feel free to rename it or to move the code around.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 25, 2025

Walkthrough

This pull request introduces significant updates to the Valibot library integration across multiple packages. The changes primarily involve updating the Valibot dependency from version 0.42.0 to 1.0.0-beta.14, modifying input and output validation approaches, and introducing a new StandardSchemaV1 error handling mechanism. The modifications span package configurations, test files, documentation, and core server components, ensuring compatibility with the new Valibot version while providing enhanced schema validation capabilities.

Changes

File Change Summary
packages/server/package.json Updated valibot dependency to version 1.0.0-beta.14
packages/server/src/unstable-core-do-not-import/parser.ts Added ParserStandardSchemaEsque type and updated parser handling
packages/tests/package.json Replaced single valibot dependency with valibot0 and valibot1
packages/tests/server/outputParser.test.ts Updated tests to support both Valibot v0 and v1
packages/tests/server/validators.test.ts Modified tests for Valibot v0 and added tests for v1
www/docs/main/quickstart.mdx Simplified input parser syntax
www/docs/server/validators.md Removed v.parser() wrapper from input/output validation
www/package.json Updated Valibot dependency to version 1.0.0-beta.14
packages/server/src/@trpc/server/index.ts Added StandardSchemaV1Error export
packages/server/src/unstable-core-do-not-import.ts Added exports for standard schema error and spec
packages/server/src/vendor/standard-schema-v1/ Added new error and specification implementations

Sequence Diagram

sequenceDiagram
    participant Client
    participant Validator
    participant StandardSchema
    participant ErrorHandler

    Client->>Validator: Send input data
    Validator->>StandardSchema: Validate input
    alt Validation Successful
        StandardSchema-->>Validator: Return validated data
        Validator-->>Client: Process request
    else Validation Failed
        StandardSchema->>ErrorHandler: Generate StandardSchemaV1Error
        ErrorHandler-->>Client: Return detailed error issues
    end
Loading

Poem

🐰 Valibot's dance, a version's leap,
From zero-point-four-two we creep
To beta's brave new validation song
Where schemas validate, precise and strong!
Hop, hop, hurray for type-safe delight! 🎉

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
packages/server/src/unstable-core-do-not-import/standardSchema.ts (3)

1-3: Consider removing or justifying ESLint rule disables.
If namespaces are crucial to the design, that's fine, but it's increasingly common to avoid TS namespaces in favor of modules. Removing these eslint-disable directives might align with standard TypeScript best practices unless there’s a strong rationale to keep them.


5-7: Confirm the usage of '~standard' as a property key.
Using a tilde in the property name might create friction with certain tooling. Unless specifically required, consider a more conventional property name (e.g., standardProps or _standard).


75-90: Include more details in the Error.message.
Currently, only issues[0]?.message is used as the error message. If there are multiple validation failures, the user is unaware of the rest. Consider concatenating or summarizing all of the issue messages for better error visibility.

constructor(issues: ReadonlyArray<StandardSchemaV1.Issue>) {
-  super(issues[0]?.message);
+  super(
+    issues.map((issue, idx) => `[${idx}] ${issue.message}`).join('\n'),
+  );
   this.name = 'SchemaError';
   this.issues = issues;
}
packages/server/src/unstable-core-do-not-import/parser.ts (1)

1-1: Use consistent import style.
Consider also importing SchemaError as a named import to emphasize usage over type conversion.

packages/tests/server/outputParser.test.ts (1)

5-6: Separate “valibot0” vs. “valibot1” imports at scale.
As more versions are introduced, consider grouping these imports clearly or placing them in different test files for better maintainability.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6be0eb0 and 2424cb4.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (9)
  • packages/server/package.json (1 hunks)
  • packages/server/src/unstable-core-do-not-import/parser.ts (4 hunks)
  • packages/server/src/unstable-core-do-not-import/standardSchema.ts (1 hunks)
  • packages/tests/package.json (1 hunks)
  • packages/tests/server/outputParser.test.ts (5 hunks)
  • packages/tests/server/validators.test.ts (5 hunks)
  • www/docs/main/quickstart.mdx (2 hunks)
  • www/docs/server/validators.md (1 hunks)
  • www/package.json (1 hunks)
🔇 Additional comments (18)
packages/server/src/unstable-core-do-not-import/standardSchema.ts (1)

10-73: Interface definitions look coherent.
All exported types and interfaces are well-documented, which helps maintain clarity. These types provide a clear contract for validation results and error reporting.

packages/server/src/unstable-core-do-not-import/parser.ts (3)

23-26: Good introduction of ParserStandardSchemaEsque.
This new type effectively expands parser capabilities for standard schemas.


58-59: Nicely extended union type.
Including ParserStandardSchemaEsque in the ParserWithInputOutput union ensures wide compatibility without breaking existing parser logic.


121-130: Validate partial vs. full success scenario.
Throwing SchemaError on any issues is logical for standard usage, but consider if partial validation success is ever relevant and if so, handle it accordingly.

Would you like to check references in the codebase to ensure partial success is not needed?

packages/tests/server/outputParser.test.ts (6)

108-114: valibot v0 test suite is consistent.
Assert that this test covers edge cases such as null, undefined, or arrays if noteworthy. Otherwise, the coverage looks good.


135-144: Asynchronous test coverage is robust.
Performing both valid and invalid queries ensures that async validations are handled properly.


170-180: Transform test is well-structured.
Verifies that the transform pipeline correctly modifies the output.


207-232: valibot v1 tests.
Great addition to confirm the updated API under $v1. The changes appear to align with the new library structure.


234-267: Async test for valibot v1 is well-covered.
You might consider testing performance if these validations become more complex, but at this scale, it's fine.


269-302: Transformation logic for valibot v1 validated successfully.
This is a strong example of how transformations can be chained in the new version.

packages/tests/server/validators.test.ts (2)

10-11: LGTM! Good separation of valibot versions.

The split imports for valibot0 and valibot1 provide a clean way to test both versions simultaneously.


235-255: Comprehensive test coverage for valibot v1.

The new test cases for valibot v1 maintain parity with v0 tests, covering:

  • Basic validation
  • Async validation
  • Transform operations

Also applies to: 257-285, 287-324

packages/tests/package.json (1)

52-53: Verify stability of beta dependency.

Using [email protected] in tests could potentially lead to instability. Consider:

  1. Pinning to a specific beta version instead of using ^
  2. Adding a comment explaining the dual-version strategy
www/package.json (1)

103-103: LGTM! Appropriate version pinning.

The exact version pin to 1.0.0-beta.14 in devDependencies is appropriate for the documentation website.

packages/server/package.json (1)

144-144: Consider removing valibot dependency.

Based on previous feedback ("We don't want dependencies"), consider:

  1. Moving valibot to peerDependencies if it's optional
  2. Removing it entirely if it's only needed for tests

If the dependency is necessary, the exact version pin to 1.0.0-beta.14 should be explained in the PR description.

www/docs/server/validators.md (1)

410-411: LGTM! The Valibot example has been updated correctly.

The changes correctly reflect the new Valibot v1 syntax by removing the v.parser() wrapper, which aligns with the Standard Schema support. The example now matches the simplified validation approach used by other schema libraries.

www/docs/main/quickstart.mdx (2)

291-291: LGTM! Clear and accurate documentation for Valibot schema usage.

The explanation of Valibot schema usage is concise and matches the implementation examples.


310-310: LGTM! Consistent with the new Valibot v1 syntax.

The example correctly demonstrates the simplified validation syntax using v.string() without the parser wrapper, maintaining consistency with other validation libraries shown in the guide.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 (1)
packages/server/src/unstable-core-do-not-import/parser.ts (1)

121-130: Consider enhancing error handling and type safety.

The implementation is solid, but could benefit from these improvements:

   if ('~standard' in parser) {
     // StandardSchemaEsque
     return async (value) => {
+      // Type narrowing for better type safety
+      const standardParser = parser['~standard'] as StandardSchemaV1<unknown, TType>;
-      const result = await parser['~standard'].validate(value);
+      const result = await standardParser.validate(value);
       if (result.issues) {
-        throw new SchemaError(result.issues);
+        throw new SchemaError(result.issues, 'Schema validation failed');
       }
       return result.value;
     };
   }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2424cb4 and 6f4a566.

📒 Files selected for processing (2)
  • packages/server/package.json (1 hunks)
  • packages/server/src/unstable-core-do-not-import/parser.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/server/package.json
🔇 Additional comments (3)
packages/server/src/unstable-core-do-not-import/parser.ts (3)

1-1: LGTM! Clean implementation of Standard Schema type.

The import statement and type definition are well-structured and follow the existing naming conventions.

Also applies to: 23-26


58-59: LGTM! Placement of Standard Schema parser is appropriate.

The placement of ParserStandardSchemaEsque at the end of the union aligns with the discussed strategy of treating Standard Schema as a fallback until it becomes more established.


121-130: Implementation aligns well with discussed requirements.

The code successfully implements the agreed-upon approach from the PR discussions:

  • Uses SchemaError for consistent error handling
  • Maintains proper placement in the parser chain
  • Preserves type inference capabilities

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 25, 2025

Open in Stackblitz

npm i https://pkg.pr.new/trpc/trpc/@trpc/client@6079
npm i https://pkg.pr.new/trpc/trpc/@trpc/next@6079
npm i https://pkg.pr.new/trpc/trpc/@trpc/server@6079
npm i https://pkg.pr.new/trpc/trpc/@trpc/react-query@6079

commit: 6f57134

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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)
packages/server/src/vendor/standard-schema-v1/spec.ts (2)

1-8: Consider specifying a stable reference to the spec in the doc comment.
Pointing to a particular commit or release tag of the standard-schema repository can help ensure long-term consistency in case the main branch changes unexpectedly.


40-46: Consider including an optional error code field.
Providing an optional error code or category could help consumers handle different failure scenarios programmatically.

packages/server/src/@trpc/server/index.ts (1)

40-40: Consider documenting the experimental status of StandardSchemaV1Error.

The export looks good, but since it's from the unstable core module, consider adding a JSDoc comment to indicate its experimental status and potential for breaking changes.

+/**
+ * @experimental This feature is part of the Standard Schema v1 support and may have breaking changes.
+ */
 StandardSchemaV1Error,
packages/tests/server/errorFormatting.test.ts (1)

127-174: Consider adding more test cases for error handling edge cases.

The test case effectively validates the basic error formatting functionality. However, consider adding tests for:

  1. Multiple validation errors
  2. Nested object validation errors
  3. Array validation errors

Here's an example of additional test cases:

test('custom error formatter with standard schema v1 (valibot) - multiple errors', async () => {
  const t = initTRPC.create({
    errorFormatter: opts => ({
      ...opts.shape,
      data: {
        ...opts.shape.data,
        standardSchemaV1Error:
          opts.error.cause instanceof StandardSchemaV1Error
            ? { issues: opts.error.cause.issues }
            : null,
      },
    }),
  });

  const input = v1.object({
    age: v1.number(),
    items: v1.array(v1.string()),
    nested: v1.object({
      value: v1.number(),
    }),
  });

  const router = t.router({
    test: t.procedure.input(input).query(({ input }) => input),
  });

  const ctx = routerToServerAndClientNew(router);

  const err = await waitError(
    ctx.client.test.query({
      age: '25',
      items: ['valid', 123],
      nested: { value: 'invalid' },
    } as any),
    TRPCClientError,
  );

  assert(err.data?.standardSchemaV1Error);
  expect(err.data.standardSchemaV1Error.issues).toHaveLength(3);
});
packages/tests/server/validators.test.ts (1)

Line range hint 139-160: Consider adding explicit version compatibility comments.

The test case effectively validates valibot v0 functionality. Consider adding a comment to clarify the version compatibility requirements.

+/**
+ * @requires valibot ^0.42.0
+ */
 test('valibot v0', async () => {
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6f4a566 and 6f57134.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • packages/server/src/@trpc/server/index.ts (1 hunks)
  • packages/server/src/unstable-core-do-not-import.ts (1 hunks)
  • packages/server/src/unstable-core-do-not-import/parser.ts (4 hunks)
  • packages/server/src/vendor/standard-schema-v1/error.ts (1 hunks)
  • packages/server/src/vendor/standard-schema-v1/spec.ts (1 hunks)
  • packages/tests/server/errorFormatting.test.ts (2 hunks)
  • packages/tests/server/validators.test.ts (6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (19)
  • GitHub Check: E2E-tests (express-server)
  • GitHub Check: e2e-legacy-node (next-prisma-todomvc, 20.x)
  • GitHub Check: E2E-tests (express-minimal)
  • GitHub Check: e2e-legacy-node (next-prisma-todomvc, 18.x)
  • GitHub Check: E2E-tests (cloudflare-workers)
  • GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 20.x)
  • GitHub Check: E2E-tests (.test/ssg)
  • GitHub Check: e2e-legacy-node (next-prisma-websockets-starter, 18.x)
  • GitHub Check: E2E-tests (.test/internal-types-export)
  • GitHub Check: e2e-legacy-node (next-prisma-starter, 20.x)
  • GitHub Check: Release using pkg.pr.new
  • GitHub Check: E2E-tests (.test/diagnostics-big-router)
  • GitHub Check: Test a monorepo using built declaration files
  • GitHub Check: e2e-legacy-node (next-prisma-starter, 18.x)
  • GitHub Check: E2E-tests (.experimental/next-app-dir)
  • GitHub Check: E2E-tests (Bun) (bun, ubuntu-latest)
  • GitHub Check: E2E-tests (Deno) (deno-deploy)
  • GitHub Check: test
  • GitHub Check: Lint and auto-fix
🔇 Additional comments (15)
packages/server/src/vendor/standard-schema-v1/spec.ts (5)

9-14: Interface naming structure looks good.
Defining the ~standard property under StandardSchemaV1 aligns with the external spec. No immediate concerns here.


15-29: Props interface is well-organized.
Each property is clearly documented, and the usage of generics for Input and Output is consistent with TypeScript best practices.


30-39: Result union is clear and explicit.
Splitting validation outcomes into SuccessResult and FailureResult makes the API easy to consume.


47-60: Issue and PathSegment interfaces align well with typical validation problem reports.
No concerns regarding the structure or naming—these interfaces are intuitive.


61-79: Type inference utilities are clearly defined.
Ensuring InferInput and InferOutput revolve around NonNullable helps reliably capture the declared types. Nicely done.

packages/server/src/vendor/standard-schema-v1/error.ts (2)

1-2: Note about potential undefined message.
If issues is empty, super(issues[0]?.message) will pass undefined to Error. You might consider a fallback message.


5-19: StandardSchemaV1Error effectively captures multiple issues.
Storing the array of issues is beneficial. Setting the error name to 'SchemaError' is clear—no further changes needed.

packages/server/src/unstable-core-do-not-import.ts (1)

43-44: Additional exports maintain consistent re-export patterns.
Exposing both error and spec aligns with the existing structure. Just keep in mind this file warns consumers not to import directly.

packages/tests/server/errorFormatting.test.ts (1)

3-4: LGTM! Clean import organization.

The imports are well-organized, grouping related imports together (trpc-related and validation libraries).

Also applies to: 9-10

packages/server/src/unstable-core-do-not-import/parser.ts (3)

1-2: LGTM! Clean import organization.

The imports are well-organized, importing from the vendored Standard Schema v1 implementation.


24-27: LGTM! Clear type definition.

The type definition for ParserStandardSchemaEsque is clear and follows the pattern of other parser types.


122-131: Consider moving the Standard Schema check higher in the priority list.

Based on past review comments, once Standard Schema becomes more established, this check should be moved higher in the priority list. For now, keeping it at the end is appropriate to maintain backward compatibility.

Let's verify the impact of the Standard Schema validation order:

✅ Verification successful

Validation order change is safe to implement when needed

The current implementation uses mutually exclusive checks for each parser type, making the order safe to change without affecting functionality. The suggestion to move Standard Schema validation higher in priority is valid and can be implemented when Standard Schema becomes more established.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential conflicts with other validation methods.

# Test: Search for validation libraries that might implement both Standard Schema and their own validation.
rg -A 5 $'~standard\' in' .

Length of output: 625


Script:

#!/bin/bash
# Get the complete validation chain from parser.ts
echo "=== Parser.ts content ==="
cat ./packages/server/src/unstable-core-do-not-import/parser.ts

echo -e "\n=== Documentation about Standard Schema ==="
# Search for documentation about Standard Schema validation
rg -i "standard.*schema" -A 5 -B 5 "README.md" || true

Length of output: 3784


Script:

#!/bin/bash
# Search for tests and examples using StandardSchema
echo "=== Tests using StandardSchema ==="
rg -l "StandardSchema" ./packages/server/test/ || true

echo -e "\n=== Usage examples of StandardSchema ==="
rg -l "StandardSchema" ./packages/server/examples/ || true

Length of output: 419

packages/tests/server/validators.test.ts (3)

3-4: LGTM! Clean import organization.

The imports are well-organized, clearly distinguishing between valibot v0 and v1.

Also applies to: 9-10


234-302: LGTM! Comprehensive error type testing.

The test cases for valibot v1 are thorough, including validation of the StandardSchemaV1Error structure and its issues.


304-371: LGTM! Thorough transformation testing.

The test cases effectively validate async validation and transformation capabilities of valibot v1.

@KATT KATT merged commit a604132 into trpc:next Jan 25, 2025
40 of 43 checks passed
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants