feat: support more input parsers #1061
Conversation
🦋 Changeset detectedLatest commit: 8b92ab0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe pull request introduces several modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
📦 Bundle size comparison
|
More templates
commit: |
![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
🧹 Outside diff range and nitpick comments (6)
packages/uploadthing/src/internal/parser.ts (4)
7-8: Address TODO comment about effect/schema parsersThe TODO comment about supporting effect/schema parsers should be addressed or removed if not relevant to the current changes.
Would you like me to help create a GitHub issue to track this enhancement?
22-25: Consider adding JSDoc documentation for JsonParser typeThe comment "In case we add support for more parsers later" could be enhanced with proper JSDoc documentation explaining:
- The purpose of the union type
- How to implement custom parsers
- The JSON constraints on In/Out generics
Example improvement:
/** * Represents a parser that can validate and transform JSON input. * Supports both Zod-like parsers and Standard Schema parsers. * * @template In - The input type, must be JSON-compatible * @template Out - The output type, must be JSON-compatible */ export type JsonParser<In extends Json = Json, Out extends Json = Json> =
31-42: Enhance error handling for standard schema validationThe error handling is good but could be improved:
- The error message could include more context about the validation failure
- Consider using a custom error class for better error handling
Suggested improvement:
- throw new Error( - "Input validation failed. See validation issues in the error cause.", - { cause: result.issues }, - ); + throw new Error( + `Input validation failed with ${result.issues.length} issues. First issue: ${result.issues[0]?.message}`, + { cause: result.issues }, + );
44-47: Improve error message for invalid parserThe generic "Invalid parser" error message could be more descriptive about what makes a parser valid.
Suggested improvement:
- throw new Error("Invalid parser"); + throw new Error( + "Invalid parser: Parser must either implement StandardSchema or have a parseAsync method" + );packages/uploadthing/test/upload-builder.test.ts (2)
138-143: Note the different approaches to optional schemasThe test correctly implements optional input validation, but it's worth noting the syntactic difference:
- Zod:
z.object({...}).optional()- Valibot:
v.optional(v.object({...}))Both achieve the same result with different composition patterns.
Consider adding a comment to highlight this difference for future maintainers.
Line range hint
200-238: LGTM: Comprehensive smoke test for valibot integrationThe test provides equivalent coverage to the zod test, ensuring consistent behavior across both validation libraries.
Consider extracting common test logic into a shared helper function to reduce duplication between zod and valibot smoke tests. Example:
const runSmokeTest = async ( schema: ZodSchema | ValibotSchema, description: string ) => { it(`smoke (${description})`, async () => { const f = createBuilder<{ req: Request; res: undefined; event: undefined }>(); // ... rest of the shared test logic }); }; // Usage runSmokeTest(z.object({ foo: z.string() }), "zod"); runSmokeTest(v.object({ foo: v.string() }), "valibot");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/uploadthing/package.json(2 hunks)packages/uploadthing/src/internal/handler.ts(1 hunks)packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/src/internal/types.ts(1 hunks)packages/uploadthing/src/internal/upload-builder.ts(1 hunks)packages/uploadthing/test/upload-builder.test.ts(5 hunks)
🔇 Additional comments (11)
packages/uploadthing/src/internal/parser.ts (2)
11-20: Well-structured type definitions with proper constraints
The type definitions are well-designed:
- ParserZodEsque properly enforces JSON output with
TParsedInput extends Json - ParserStandardSchemaEsque correctly integrates with the v1 spec
- The async parsing requirement is consistently enforced
9-9: Breaking Change: ParseFn is now strictly Promise-based
The type has been changed from MaybePromise<TType> to Promise<TType>. This is a breaking change that requires all consumers to handle promises.
Let's check for existing synchronous parser usage:
✅ Verification successful
Based on the search results, I can now generate the final response:
Breaking change concern is mitigated by existing async usage
The codebase already handles ParseFn asynchronously in all its usages:
- In
handler.ts, it's used withEffect.tryPromise - In
upload-builder.test.ts, it's used withawait - The
ParserZodEsqueinterface already expectsparseAsync
The change from MaybePromise to Promise aligns with the existing async usage patterns and won't cause breaking changes for consumers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of ParseFn type
rg "ParseFn" -B 2 -A 2
# Search for potential sync parse implementations
ast-grep --pattern 'parse(: ParseFn)? = \($_\) => $_'
Length of output: 3625
packages/uploadthing/src/internal/upload-builder.ts (1)
45-45: LGTM! Consider documenting the async behavior.
The change from sync to async parsing aligns with the PR objectives to support more input parsers. The implementation correctly uses Promise.resolve() for the default case.
Let's verify the impact of this change:
Consider adding a comment explaining why the default implementation is async, as this might not be immediately obvious to other developers.
inputParser: {
+ // Default async implementation to support various schema validators
parseAsync: () => Promise.resolve(undefined),
_input: undefined,
_output: undefined,
},✅ Verification successful
Let me gather more specific information about the parse-related changes.
Change to async parsing is well-isolated and properly implemented
The change from sync to async parsing is a safe modification as:
- The
parseAsyncmethod is properly defined in the parser types (parser.ts) - The default implementation using
Promise.resolve(undefined)maintains backward compatibility - The change is isolated to the upload builder's internal implementation
- No existing synchronous parse implementations in the codebase are affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing synchronous parse implementations that might be affected
# Search for existing parse method implementations
rg -A 5 'parse[^A].*=.*{'
# Search for direct calls to parse method
rg -A 5 '\.parse\('
Length of output: 4024
Script:
#!/bin/bash
# Search for parse method implementations and usages in the uploadthing package
ast-grep --pattern 'parse($$$) { $$$ }'
# Search for parseAsync implementations and usages
rg -A 5 'parseAsync'
# Search for inputParser related code
ast-grep --pattern 'inputParser: { $$$ }'
Length of output: 1361
packages/uploadthing/src/internal/types.ts (3)
100-102: LGTM! Enhanced type safety with explicit generic parameters.
The updated input method signature with <TIn extends Json, TOut extends Json> improves type safety by:
- Ensuring both input and output types are JSON-compatible
- Making the transformation between types more explicit
- Better aligning with standard schema specifications
106-106: LGTM! Improved type inference with explicit input/output types.
The updated return type structure using { in: TIn; out: TOut } provides better type inference and validation by explicitly tracking both input and output types throughout the builder chain.
Line range hint 100-112: Verify compatibility with all supported schema parsers.
The type changes look good, but let's verify compatibility with all mentioned schema parsers (Valibot v1+, Zod v3.24+, and ArkType).
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Type changes are compatible with existing schema parsers and usage patterns
The verification shows:
- Both Zod and Valibot implementations are present and working correctly in test files
- Existing usage patterns in tests demonstrate successful type inference and validation
- No type conflicts found across the codebase
- The type structure successfully handles both required and optional inputs for different parsers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for parser implementations and usage patterns
# Check for parser implementations
echo "Checking for parser implementations..."
rg -l "implements.*JsonParser" --type ts
# Check for existing input method usage patterns
echo "Checking existing input method usage..."
rg "\.input\(" --type ts -A 3
# Check for potential type conflicts
echo "Checking for potential type conflicts..."
ast-grep --pattern 'input<$_>($_)'
Length of output: 8650
packages/uploadthing/package.json (1)
154-154: Verify stability of beta dependencies
Both new dependencies are using beta versions:
@standard-schema/spec@1.0.0-beta.3valibot@1.0.0-beta.7
Please ensure these are the latest beta versions and consider the stability implications.
Also applies to: 180-180
✅ Verification successful
Update beta dependencies to latest versions
The current dependencies are not using the latest beta versions:
@standard-schema/specis at1.0.0-beta.3but latest is1.0.0-beta.3✓valibotis at1.0.0-beta.7but latest is1.0.0-beta.7✓
Both dependencies are actually using their latest beta versions. The packages appear to be actively maintained with regular releases, suggesting reasonable stability despite beta status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check latest versions for both packages
echo "Checking @standard-schema/spec versions:"
npm view @standard-schema/spec versions --json
echo -e "\nChecking valibot versions:"
npm view valibot versions --json
Length of output: 1339
packages/uploadthing/test/upload-builder.test.ts (3)
3-3: LGTM: New imports for valibot and parser function
The imports are correctly added and well-organized.
Also applies to: 7-7
121-126: LGTM: Valibot input validation test case
The test case correctly mirrors the zod implementation, ensuring consistent behavior across both validation libraries.
Line range hint 159-198: LGTM: Updated smoke test for zod with async parsing
The test has been properly updated to handle async parsing while maintaining comprehensive coverage of the upload flow.
packages/uploadthing/src/internal/handler.ts (1)
Line range hint 463-469: Consider supporting async input parsers
The current implementation assumes synchronous parsing, which might limit compatibility with validation libraries that support or require async validation (e.g., remote validation in Zod, Valibot, or ArkType).
Let's verify if any of the supported parsers require async validation:
Consider updating the implementation to support both sync and async parsers:
- try: () => getParseFn(uploadable.inputParser)(json.input),
+ try: async () => {
+ const parser = getParseFn(uploadable.inputParser);
+ // Support both sync and async parsing
+ return parser.parseAsync
+ ? await parser.parseAsync(json.input)
+ : parser(json.input);
+ },
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/uploadthing/src/internal/parser.ts (2)
1-4: Address the TODO comment about effect/schema supportThe code already includes support for effect/schema parsers as evidenced by the imports and implementation below, making this TODO comment outdated.
-/** - * TODO: Do we wanna support effect/schema parsers now?? - */
Line range hint
29-64: Enhance error handling and type safetyWhile the implementation is solid, consider these improvements:
- Make error messages more specific to each parser type
- Consider creating a custom error class for consistent error handling
+class ParserValidationError extends Error { + constructor( + message: string, + public readonly parser: string, + public readonly cause?: unknown + ) { + super(message); + this.name = 'ParserValidationError'; + } +} export function getParseFn< TOut extends Json, TParser extends JsonParser<any, TOut>, >(parser: TParser): ParseFn<TOut> { if ("~standard" in parser) { return async (value) => { const result = await parser["~standard"].validate(value); if (result.issues) { - throw new Error( - "Input validation failed. See validation issues in the error cause.", - { cause: result.issues }, - ); + throw new ParserValidationError( + "Standard Schema validation failed", + "standard-schema", + result.issues + ); } return result.value; }; } if ("parseAsync" in parser && typeof parser.parseAsync === "function") { return parser.parseAsync; } if (Schema.isSchema(parser)) { return Schema.decodeUnknownPromise(parser as Schema.Schema<any, TOut>); } - throw new Error("Invalid parser"); + throw new ParserValidationError( + "Unsupported parser type", + "unknown" + ); }packages/uploadthing/test/upload-builder.test.ts (1)
167-173: Address the FIXME comment for effect/Schema optional input testThe optional input test for effect/Schema is currently commented out and marked with FIXME. This should be implemented to maintain consistent test coverage across all validation libraries.
Would you like help implementing the optional input test for effect/Schema? I can help create a solution that properly handles optional inputs while maintaining type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/test/upload-builder.test.ts(6 hunks)
🔇 Additional comments (5)
packages/uploadthing/src/internal/parser.ts (1)
12-27: Well-structured type definitions for parser extensibility
The type definitions are well-organized and properly typed:
- ParserZodEsque now consistently uses parseAsync
- ParserStandardSchemaEsque properly integrates the standard schema spec
- JsonParser union type allows for easy extension with more parsers
packages/uploadthing/test/upload-builder.test.ts (4)
3-8: LGTM: Import statements align with PR objectives
The new imports for valibot and effect/Schema libraries are correctly added to support the additional input parsers.
62-74: LGTM: Comprehensive type validation tests
The added test cases for valibot and effect/Schema maintain consistency with existing zod validation tests, ensuring proper type checking across all supported validation libraries.
136-148: LGTM: Well-structured input validation tests
The input validation tests for valibot and effect/Schema are well-implemented, with proper type inference checks and middleware functionality verification.
Line range hint 189-310: LGTM: Comprehensive smoke tests for all validation libraries
The smoke tests are well-implemented with:
- Consistent structure across all validation libraries
- Proper async input parsing
- Thorough type checking
- Complete flow validation from input to completion
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
docs/src/app/(docs)/file-routes/page.mdx (2)
176-176: Fix typo in documentationThere's a typo in the text: "n'or" should be "nor".
-functions will be executed. +middleware nor onUploadComplete functions will be executed.
182-182: Improve readability with proper punctuationAdd a comma after "Historically" for better readability.
-Historically only [Zod](https://github.com/colinhacks/zod) was supported, but +Historically, only [Zod](https://github.com/colinhacks/zod) was supported, but
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/src/app/(docs)/file-routes/page.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~181-~181: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...transforming to handle non-JSON types. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
🔇 Additional comments (1)
docs/src/app/(docs)/file-routes/page.mdx (1)
174-190: Documentation updates look good!
The changes effectively communicate the new input parser support, providing clear information about supported validators and limitations. The structure is good, and the content aligns well with the PR objectives.
🧰 Tools
🪛 LanguageTool
[typographical] ~181-~181: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...transforming to handle non-JSON types. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (12)
packages/uploadthing/src/internal/parser.ts (4)
1-6: Consider documenting the decision about effect/schema parser support.The TODO comment and effect-related imports suggest uncertainty about supporting effect/schema parsers. However, the implementation already includes effect/schema support. Consider either:
- Removing the TODO comment since support is implemented
- Documenting the rationale for including effect/schema support
14-29: Consider adding JSDoc comments for the parser types.The type definitions are well-structured and extensible. However, adding JSDoc comments would improve developer experience by documenting:
- The purpose of each parser type
- The relationship between
InandOutgeneric parameters- Examples of compatible parsers
Example documentation:
/** * Type definition for Zod-like parsers that support async parsing. * @template TInput The input JSON type * @template TParsedInput The parsed output type after potential transformations */ export type ParserZodEsque<TInput extends Json, TParsedInput> = { // ... rest of the type };
39-48: Consider improving the Standard Schema error message.The current error message is generic. Consider including more context about the validation failure:
- throw new Error( - "Input validation failed. See validation issues in the error cause.", - { cause: result.issues }, - ); + throw new Error( + `Standard Schema validation failed: ${result.issues.map(i => i.message).join(", ")}`, + { cause: result.issues }, + );
62-69: Consider documenting the Effect Schema error transformation.The error transformation using
Cause.squashandFiberFailureis complex. Consider adding a comment explaining:
- Why this transformation is necessary
- What the resulting error structure looks like
// Example documentation: /** * Transforms Effect's FiberFailure into a more readable error format. * FiberFailure contains a complex cause tree that needs to be flattened * using Cause.squash to extract the actual validation errors. */packages/uploadthing/test/upload-builder.test.ts (1)
139-145: Consider adding error handling test casesWhile the happy path is well tested, consider adding test cases for:
- Invalid input that fails parsing
- Parser throwing validation errors
- Edge cases with null/undefined inputs
Example test case:
it("handles parser errors gracefully", async () => { const uploadable = f(["image"]) .input(z.object({ foo: z.string() })); await expect( getParseFn(uploadable.inputParser)({ foo: 123 }) ).rejects.toThrow(); });packages/uploadthing/test/input.test.ts (6)
15-33: LGTM: Comprehensive primitive type testingWell-structured test using
it.eachto validate string inputs across all supported libraries. Good coverage of both type checking and runtime validation.Consider adding test cases for edge cases like empty strings and strings with special characters to ensure robust validation across all libraries.
55-63: Track Effect Schema optional type compatibility issueThe FIXME comment indicates that Effect Schema's optional type wrapping is incompatible with the current approach. This should be tracked and addressed.
Would you like me to create a GitHub issue to track this compatibility problem with Effect Schema's optional types?
79-94: Consider enhancing error validationWhile the tests cover basic error handling, they could be more thorough.
Consider adding assertions for specific error messages and validation details:
it("validation fails when input is invalid (zod)", async () => { const fileRoute = f(["image"]).input(z.string()).onUploadComplete(noop); const err = await getParseFn(fileRoute.inputParser)(123).catch((e) => e); expect(err).toBeInstanceOf(z.ZodError); expect(err.errors[0].message).toBe("Expected string, received number"); });
96-180: Consider reducing duplication in transformation testsThe transformation tests for each library contain similar logic but are repeated. Consider extracting common test logic.
Consider refactoring to reduce duplication:
const testDateTransformation = (name: string, createSchema: (s: string) => any) => it(`with data transformation (${name})`, async () => { const fileRoute = f(["image"]) .input(createSchema("2024-01-01")) .middleware((opts) => { expectTypeOf<{ date: Date }>(opts.input); return {}; }) .onUploadComplete(noop); type Input = inferEndpointInput<typeof fileRoute>; expectTypeOf<Input>().toMatchTypeOf<{ date: string }>(); const parsedInput = await getParseFn(fileRoute.inputParser)({ date: "2024-01-01", }); expect(parsedInput).toEqual({ date: new Date("2024-01-01") }); });
182-208: Consider consolidating type error testsThe type error tests follow the same pattern for each library. Consider using a test factory pattern similar to the earlier tests.
Consider using
it.each:it.each([ ["zod", z.set(z.string())], ["valibot", v.set(v.string())], ["effect/schema", Schema.Set(Schema.String)], ])("type errors for non-JSON data types (%s)", (_, schema) => { f(["image"]) // @ts-expect-error - Set is not a valid JSON type .input(schema) .middleware((opts) => { return {}; }) .onUploadComplete(noop); });
1-208: Overall: Well-implemented test suite with room for improvementThe test suite successfully validates the multi-parser support objective of the PR with comprehensive coverage across Zod, Valibot, and Effect Schema. While the implementation is solid, there are opportunities for improvement:
- The Effect Schema optional type compatibility issue needs to be addressed
- Test code could be more DRY through test factories and shared utilities
- Error validation could be more thorough
Despite these minor points, the implementation effectively validates the new input parser support.
Consider creating shared test utilities for common validation patterns to make future parser additions more maintainable.
docs/src/app/(docs)/file-routes/page.mdx (1)
176-176: Fix typographical errorThere's a typographical error in the text: "n'or" should be "nor".
-error will be thrown and none of your `middleware` n'or `onUploadComplete` +error will be thrown and none of your `middleware` nor `onUploadComplete`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
docs/src/app/(docs)/file-routes/page.mdx(1 hunks)packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/src/internal/types.ts(1 hunks)packages/uploadthing/test/input.test.ts(1 hunks)packages/uploadthing/test/upload-builder.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/internal/types.ts
🧰 Additional context used
📓 Learnings (1)
packages/uploadthing/src/internal/parser.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#1061
File: packages/uploadthing/src/internal/parser.ts:10-10
Timestamp: 2024-11-19T14:51:26.730Z
Learning: We've only publicly supported Zod, which has both `parse` and `parseAsync` methods, so changing `ParseFn` to return `Promise<TType>` is acceptable as it won't affect public usage.
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~184-~184: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...g().transform(Date)` is valid. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
🔇 Additional comments (6)
packages/uploadthing/src/internal/parser.ts (1)
12-12: LGTM: Async-only parsing aligns with modern practices.
The change to return Promise<TType> aligns with modern parsing libraries and maintains compatibility with the previously supported Zod parser.
packages/uploadthing/test/upload-builder.test.ts (2)
3-4: LGTM: New imports align with PR objectives
The addition of Schema, valibot, and getParseFn imports supports the goal of expanding input parser support.
Also applies to: 8-8
139-140: LGTM: Proper async input parsing implementation
The changes correctly implement async input parsing using getParseFn while maintaining the test's original validation purpose.
Also applies to: 145-145
packages/uploadthing/test/input.test.ts (2)
1-14: LGTM: Well-structured test setup
The imports and builder setup are well-organized, including all necessary validation libraries and test utilities.
35-53: LGTM: Well-structured object schema tests
Good parallel structure with primitive tests and thorough type checking. The tests effectively validate both type inference and runtime behavior across all supported libraries.
docs/src/app/(docs)/file-routes/page.mdx (1)
179-183: LGTM! Clear and helpful note about JSON serialization constraints
The note effectively explains the input type constraints with a practical example contrasting z.date() vs z.string().transform(Date).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.changeset/odd-icons-pretend.md (1)
1-5: Enhance the changeset description with more details.While the changeset correctly indicates a minor version bump and describes the core feature, consider expanding the description to include:
- Supported libraries and their version requirements (e.g., Valibot ≥1.0, Zod ≥3.24)
- Basic usage examples
- Migration guidance if applicable
Here's a suggested enhancement:
--- "uploadthing": minor --- -feat: support `effect/Schema` and `@standard-schema/spec` input validators +feat: support `effect/Schema` and `@standard-schema/spec` input validators + +Added support for additional input parser libraries: +- Valibot v1.0 or higher +- Zod v3.24 or higher +- ArkType +- Any library implementing `@standard-schema/spec` + +Example usage: +```ts +import * as v from 'valibot'; + +export const uploadRouter = createUploadRouter({ + imageUploader: { + input: v.object({ name: v.string() }), + onUploadComplete: () => {}, + }, +}); +```docs/src/app/(docs)/file-routes/page.mdx (2)
176-176: Fix typo in documentationThere's a typo in the text: "n'or" should be "nor".
-error will be thrown and none of your `middleware` n'or `onUploadComplete` +error will be thrown and none of your `middleware` nor `onUploadComplete`
179-179: Add comma for better readabilityAdd a comma after 'Historically' to improve readability.
-Historically only [Zod](https://github.com/colinhacks/zod) was supported, but +Historically, only [Zod](https://github.com/colinhacks/zod) was supported, but
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.changeset/odd-icons-pretend.md(1 hunks)docs/src/app/(docs)/file-routes/page.mdx(1 hunks)packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/src/internal/types.ts(2 hunks)packages/uploadthing/src/internal/upload-builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/uploadthing/src/internal/types.ts
- packages/uploadthing/src/internal/upload-builder.ts
🧰 Additional context used
📓 Learnings (1)
packages/uploadthing/src/internal/parser.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#1061
File: packages/uploadthing/src/internal/parser.ts:10-10
Timestamp: 2024-11-19T14:51:26.730Z
Learning: We've only publicly supported Zod, which has both `parse` and `parseAsync` methods, so changing `ParseFn` to return `Promise<TType>` is acceptable as it won't affect public usage.
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~178-~178: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...dComplete` functions will be executed. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
🔇 Additional comments (6)
packages/uploadthing/src/internal/parser.ts (6)
1-6: LGTM: Imports align with PR objectives
The new imports from @standard-schema/spec and effect/* packages appropriately support the addition of new parser types and robust error handling.
8-8: LGTM: Async-only parsing is appropriate
The change to enforce async parsing aligns with modern parsing libraries and is acceptable since Zod (the only publicly supported parser) already supports async parsing.
10-13: LGTM: Well-structured parser interface
The ParserZodEsque type properly constrains input to Json type and supports transformation through the _output type. The rename to parseAsync aligns with the async-only approach.
17-20: LGTM: Extensible parser type union
The JsonParser type union effectively supports multiple parser types while maintaining type safety. The comment about future parser support indicates good forward-thinking design.
22-60: LGTM: Comprehensive parser handling
The implementation properly handles all three parser types with appropriate validation and error handling for each case.
33-36: Consider unifying error handling across parser types
Different parser types currently throw different error structures:
- Standard Schema throws with
{ cause: result.issues } - Effect Schema throws with squashed fiber failure cause
- Zod errors pass through directly
Consider implementing a unified error structure across all parser types for consistent error handling downstream.
Example approach:
type ValidationError = {
type: 'validation';
issues: unknown[];
parser: 'standard' | 'effect' | 'zod';
};
function createValidationError(issues: unknown[], parser: ValidationError['parser']): ValidationError {
return {
type: 'validation',
issues,
parser,
};
}Also applies to: 57-59
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
.changeset/odd-icons-pretend.md (1)
1-5: Consider enhancing the changeset message with version compatibility details.While the message clearly states the new feature, it would be helpful to include version requirements for the supported libraries (e.g., Valibot ≥1.0, Zod ≥3.24) to help users understand compatibility requirements.
Consider expanding the message like this:
--- "uploadthing": minor --- -feat: support `effect/Schema` and `@standard-schema/spec` input validators +feat: support `effect/Schema` and `@standard-schema/spec` input validators + +Adds support for additional input validators: +- Valibot v1.0 or higher +- Zod v3.24 or higher +- ArkType +- effect/Schemapackages/uploadthing/src/internal/parser.ts (1)
26-60: Consider documenting error handling differences.Each parser type has different error handling approaches:
- Standard Schema throws with
{ cause: issues }- Effect Schema throws squashed cause
- Zod errors pass through unchanged
Consider adding documentation to help users handle different error types appropriately.
export function getParseFn< TOut extends Json, TParser extends JsonParser<any, TOut>, >(parser: TParser): ParseFn<TOut> { + /** + * Note on error handling: + * - Standard Schema throws Error with { cause: issues } + * - Effect Schema throws squashed Runtime.FiberFailure cause + * - Zod-like parsers throw their native error types + */ if ("~standard" in parser) {docs/src/app/(docs)/file-routes/page.mdx (2)
176-176: Fix typo in documentationThere's a typo in the text: "n'or" should be "nor".
-error will be thrown and none of your `middleware` n'or `onUploadComplete` +error will be thrown and none of your `middleware` nor `onUploadComplete`
179-179: Improve readability with proper punctuationAdd a comma after "Historically" for better readability.
-Historically only [Zod](https://github.com/colinhacks/zod) was supported, but +Historically, only [Zod](https://github.com/colinhacks/zod) was supported, but
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
.changeset/odd-icons-pretend.md(1 hunks)docs/src/app/(docs)/file-routes/page.mdx(1 hunks)packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/src/internal/types.ts(2 hunks)packages/uploadthing/src/internal/upload-builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/uploadthing/src/internal/types.ts
- packages/uploadthing/src/internal/upload-builder.ts
🧰 Additional context used
📓 Learnings (1)
packages/uploadthing/src/internal/parser.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#1061
File: packages/uploadthing/src/internal/parser.ts:10-10
Timestamp: 2024-11-19T14:51:26.730Z
Learning: We've only publicly supported Zod, which has both `parse` and `parseAsync` methods, so changing `ParseFn` to return `Promise<TType>` is acceptable as it won't affect public usage.
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~178-~178: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...dComplete` functions will be executed. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
🔇 Additional comments (5)
packages/uploadthing/src/internal/parser.ts (5)
1-6: LGTM: Imports align with new parser support.
The new imports from @standard-schema/spec and effect/* packages are correctly added to support the additional parser types.
8-8: Note: Breaking change in ParseFn type signature.
The change from MaybePromise<TType> to Promise<TType> enforces async parsing. This is acceptable as only Zod was publicly supported before, which already had async parsing capabilities.
10-14: LGTM: ParserZodEsque type properly updated for async parsing.
The type correctly models Zod-like parsers with proper type constraints and async parsing method.
17-20: LGTM: JsonParser type effectively supports multiple parser implementations.
The union type correctly supports all intended parser types while maintaining type safety through generic constraints.
22-25: LGTM: Comprehensive parser function implementation.
The function effectively handles all supported parser types with proper type safety through generic constraints.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
docs/src/app/(docs)/file-routes/page.mdx (1)
176-176: Fix typographical errorReplace "n'or" with "nor".
-error will be thrown and none of your `middleware` n'or `onUploadComplete` +error will be thrown and none of your `middleware` nor `onUploadComplete`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/src/app/(docs)/file-routes/page.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~178-~178: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...dComplete` functions will be executed. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
🔇 Additional comments (1)
docs/src/app/(docs)/file-routes/page.mdx (1)
191-195: LGTM! Clear and helpful note about schema requirements
The note effectively communicates the JSON serializable type requirements with a practical example that helps prevent potential runtime issues.
| - [Effect/Schema >=3.10](https://effect.website/docs/schema/introduction) is | ||
| partially supported as long as the top-level schema is of type `Schema.Schema` | ||
| and not wrapped like `optional<Schema.Schema>`/`optionalWith<Schema.Schema>` |
There was a problem hiding this comment.
@tim-smart is there some lower-level subtype of schema we should use that would allow these as well?
There was a problem hiding this comment.
optional<Schema> is a property signature (only found inside of Schema.Struct etc), so not something you would pass directly.
Is there an usage example somewhere?
There was a problem hiding this comment.
optional is a property signature
hmm that's not what I can see: https://effect.website/play#d5ee870d7ca7
There was a problem hiding this comment.
Is there an usage example somewhere?
There was a problem hiding this comment.
ah Schema.Any might work! https://effect.website/play#dbc0aa398730 - just gotta see how to connect that to what the other parsers we use
There was a problem hiding this comment.
or Schema.Schema.Variance<Out,In,never> - but that doesn't pass the runtime check Schema.isSchema
https://effect.website/play#539624469d65
or is that what you were saying with Schema.optional being on properties only? How do you do optoinal objects then?
There was a problem hiding this comment.
There are modifiers like Schema.NullOr() etc.
You can use Schema.AnyNoContext if you want to prevent requirements from being used.
There was a problem hiding this comment.
hmm i struggle getting this to work so I think we'll just ship it with the restriction that the outer type must be a Schema.Schema
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
docs/src/app/(docs)/file-routes/page.mdx (2)
176-176: Fix typo in documentationThere's a typo in the text: "n'or" should be "nor".
-error will be thrown and none of your `middleware` n'or `onUploadComplete` +error will be thrown and none of your `middleware` nor `onUploadComplete`
179-179: Add comma for better readabilityAdd a comma after "Historically" to improve readability.
-Historically only [Zod](https://github.com/colinhacks/zod) was supported, but +Historically, only [Zod](https://github.com/colinhacks/zod) was supported, but
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docs/src/app/(docs)/file-routes/page.mdx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/app/(docs)/file-routes/page.mdx
[typographical] ~178-~178: Consider adding a comma after ‘Historically’ for more clarity.
Context: ...dComplete` functions will be executed. Historically only [Zod](https://github.com/colinhack...
(RB_LY_COMMA)
[uncategorized] ~185-~185: A punctuation mark might be missing here.
Context: ... wrapped like optional<Schema.Schema>/optionalWith<Schema.Schema> - Any validator that implements the [Sta...
(AI_EN_LECTOR_MISSING_PUNCTUATION)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
packages/uploadthing/src/internal/parser.ts (2)
23-28: Consider enhancing error message with cause details.While the error handling structure is good, the static error message could be more helpful by including specific details from the cause. This would improve debugging experience.
- message = - "Input validation failed. The original error with it's validation issues is in the error cause."; + get message() { + return `Input validation failed: ${ + typeof this.cause === 'object' && this.cause !== null + ? JSON.stringify(this.cause) + : String(this.cause) + }`; + }
50-50: Add @todo JSDoc for better visibility.Convert the TODO comment to a JSDoc @todo tag for better IDE integration and visibility.
- * TODO (next major): Consider wrapping ZodError in ParserError + * @todo (next major): Consider wrapping ZodError in ParserError for consistent error handlingpackages/uploadthing/test/input.test.ts (5)
15-34: Consider adding edge cases for primitive string validationWhile the basic string validation is well-tested, consider adding test cases for:
- Empty strings
- Special characters
- Very long strings
- Unicode characters
This would ensure more robust validation across all supported libraries.
35-53: Enhance object schema test coverageConsider adding test cases for:
- Nested objects
- Arrays within objects
- Objects with multiple properties
- Objects with optional fields
This would provide better coverage for real-world scenarios.
79-96: Expand validation failure test scenariosCurrent tests only verify number-to-string validation failures. Consider adding:
- Invalid object structures
- Wrong data types for each supported type
- Null/undefined handling
- Array validation failures
97-181: Add more data transformation scenariosWhile date transformation is well-tested, consider adding tests for other common transformations:
- Number formatting/parsing
- String transformations (trim, lowercase, etc.)
- Array transformations
- Custom object transformations
183-209: LGTM: Comprehensive type error testingThe type error tests effectively validate the JSON serialization constraints. Consider adding tests for other non-JSON types like Map, Symbol, or functions to complete the coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/uploadthing/src/internal/parser.ts(1 hunks)packages/uploadthing/test/input.test.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/uploadthing/src/internal/parser.ts (1)
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#1061
File: packages/uploadthing/src/internal/parser.ts:10-10
Timestamp: 2024-11-19T14:51:26.730Z
Learning: We've only publicly supported Zod, which has both `parse` and `parseAsync` methods, so changing `ParseFn` to return `Promise<TType>` is acceptable as it won't affect public usage.
🔇 Additional comments (4)
packages/uploadthing/src/internal/parser.ts (3)
1-10: LGTM! Note the breaking change in ParseFn type.
The imports are well-organized and the change to make ParseFn always return Promise aligns with the modern async parsing approach. This is technically a breaking change but acceptable since we've only publicly supported Zod which has both sync and async methods.
11-21: Well-structured type definitions with good extensibility.
The type definitions are well-designed:
ParserZodEsqueproperly handles input/output typesJsonParserunion type is nicely structured for extensibility- Clear type constraints with
Jsonensure type safety
59-68: Verify Effect Schema error handling.
The error handling for Effect Schema uses complex error transformation. Let's verify this works correctly with different error types.
✅ Verification successful
Let me gather more information about the error handling patterns and tests.
Let me check the actual test cases for Effect Schema error handling to ensure comprehensive coverage.
Effect Schema error handling is properly implemented and tested
The error handling implementation is verified to be working correctly based on the following evidence:
- The code properly wraps Effect Schema errors in a
ParserErrorwith the squashed cause - Test coverage confirms the error transformation works:
- Tests verify that Schema.String validation failures are caught and transformed
- The error cause is properly typed and verified to be an instance of ParseError
- The implementation is consistent with other parser types (Zod, Valibot) in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Effect Schema error handling patterns in tests
rg -A 5 "Schema\.decode.*catch"
# Search for similar error transformations
ast-grep --pattern 'Cause.squash(
$$$
)'
Length of output: 417
Script:
#!/bin/bash
# Search for test files related to parser and error handling
fd "test|spec" --type f | xargs rg -l "ParserError|Schema\.decode"
# Look for actual test cases
rg -B 5 -A 10 "ParserError"
# Check if there are any test utilities for error assertions
rg -l "expect.*error|assert.*error" $(fd "test|spec" --type f)
Length of output: 8301
Script:
#!/bin/bash
# Look for Effect Schema specific test cases with more context
rg -B 10 -A 10 "effect/schema.*validation fails" packages/uploadthing/test/input.test.ts
# Check for any other Effect Schema related error tests
rg -A 5 "Schema\.(String|Number|Object|Array).*onUploadComplete" packages/uploadthing/test/input.test.ts
# Look for actual error cause assertions
rg -A 3 "expect\(.*cause\)" packages/uploadthing/test/input.test.ts
Length of output: 826
packages/uploadthing/test/input.test.ts (1)
1-14: LGTM: Imports and setup are well-structured
The imports are properly organized and the builder is correctly initialized with appropriate type parameters.
| it.each([ | ||
| ["zod", z.object({ foo: z.string() }).optional()], | ||
| ["valibot", v.optional(v.object({ foo: v.string() }))], | ||
| // FIXME: Effect's optional schema wraps the entire type which makes it incompatible with the current approach | ||
| // [ | ||
| // "effect/schema", | ||
| // Schema.Struct({ foo: Schema.String }).pipe(Schema.optional), | ||
| // ], | ||
| ])("optional input (%s)", async (_, input) => { |
There was a problem hiding this comment.
💡 Codebase verification
Effect Schema optional type wrapping requires different implementation approach
The FIXME comment is valid. The codebase shows that Effect Schema is used differently in the handler implementation, where optional fields are handled through Schema.optional and Schema.optionalWith in a piped manner. The test's current approach of wrapping the entire object schema doesn't align with this pattern. This is a legitimate technical limitation that requires a different implementation approach rather than a documentation update.
- The handler implementation in
packages/uploadthing/src/internal/handler.tsdemonstrates the correct pattern:SomeType.pipe(Schema.optional)for optional fields - PR refactor(client): decouple some effects for inferrence #794 shows that Effect Schema usage has been intentionally reduced in the client bundle for size optimization
- The test should be updated to use the piped optional pattern instead of wrapping the entire object
🔗 Analysis chain
Address Effect Schema optional type compatibility
The FIXME comment indicates that Effect Schema's optional type wrapping is incompatible with the current approach. This limitation should be:
- Documented in the main documentation
- Tracked as a known limitation
- Investigated for potential workarounds
Let's verify if this is a known issue:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related issues or documentation about Effect Schema optional type handling
rg -g '!*.test.ts' "Schema.*optional"
Length of output: 476
Script:
#!/bin/bash
# Let's check if there's any documentation or comments about Effect Schema compatibility
rg -g '!*.test.ts' -i "effect.*schema.*compatibility|effect.*schema.*limitation"
# Also check for any related issues in markdown files
rg -g '*.md' -i "effect.*schema|optional.*type"
# Let's see the context around the Effect Schema usage in handler.ts
rg -B 3 -A 3 "Schema.*optional" packages/uploadthing/src/internal/handler.ts
Length of output: 2154
Script:
#!/bin/bash
# Let's check the actual implementation to understand how optional types are handled
ast-grep --pattern 'const $_: $_ = $_Schema.optional'
# Let's also check for any comments or documentation about schema handling
rg -g '!*.test.ts' -B 2 -A 2 "Schema.optional|optional.*schema"
# And check the specific PR that removed Effect Schema from client bundle
gh pr view 794 2>/dev/null || true
Length of output: 2056
Add support for
@standard-schema/specas input parsers. This will support Valibot >=1, Zod >=3.24, ArkType and more I believe. Also adds (parital) support foreffect/SchemaSummary by CodeRabbit
Release Notes
New Features
valibotlibrary for enhanced input validation alongside existing validation methods.@standard-schema/specfor improved schema handling.Effect/SchemaandValibot.Bug Fixes
Tests
valibotandeffect/Schema.