-
Couldn't load subscription status.
- Fork 1.3k
feat: Add support for zod@4 schemas #1666
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add support for zod@4 schemas #1666
Conversation
c482776 to
8f56474
Compare
tests/helpers/zod.test.ts
Outdated
| }); | ||
|
|
||
| it('throws error on optional fields', () => { | ||
| (version === 'v4' ? it.skip : it)('throws error on optional fields', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we transform the schema, we actually add missing fields in required property, as we do in python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I don't think this is quite right, in Python it's fine because the property being omitted or explicitly set to null results in the same type, None, but in TS it's different.
so if we add properties that are .optional() to the required array, then the API will send them back as null, which breaks the type promise because it'd be typed as property?: string instead of property?: string | null or property: string | null.
the equivalent behaviour here for python would be to only add properties to required when they're both .optional() and .nullable() which is why we throw the current error.
| describe.each([ | ||
| { version: 'v3', z: zv3 as any }, | ||
| { version: 'v4', z: zv4 as any }, | ||
| ])('zodResponseFormat (Zod $version)', ({ version, z }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 this is a cool way to test things
tests/helpers/zod.test.ts
Outdated
| }); | ||
|
|
||
| it('throws error on optional fields', () => { | ||
| (version === 'v4' ? it.skip : it)('throws error on optional fields', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I don't think this is quite right, in Python it's fine because the property being omitted or explicitly set to null results in the same type, None, but in TS it's different.
so if we add properties that are .optional() to the required array, then the API will send them back as null, which breaks the type promise because it'd be typed as property?: string instead of property?: string | null or property: string | null.
the equivalent behaviour here for python would be to only add properties to required when they're both .optional() and .nullable() which is why we throw the current error.
1a733c6 to
86f85b0
Compare
|
Hi, whats blocking this? |
520c8a9 to
3595555
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 quick comments.
Thanks so much for this @karpetrosyan !
| import { OpenAI } from 'openai'; | ||
| import { zodResponsesFunction } from 'openai/helpers/zod'; | ||
| import { z } from 'zod/v3'; | ||
| import { z } from 'zod/v4'; // Also works for 'zod/v3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karpetrosyan any chance you could run all these examples with v3 on this PR just to make sure things are back-compatible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thats a good callout, I think I asked to switch them to v4 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked all of them locally, all worked with v3!
| function zodV4ToJsonSchema(schema: z4.ZodType): Record<string, unknown> { | ||
| return toStrictJsonSchema( | ||
| z4.toJSONSchema(schema, { | ||
| target: 'draft-7', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious how we picked this value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've lost the context here a little bit, but looking at it again, it seems like we shouldn't set the concrete target. I'll remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think I did that because the current vendored version that parses Zod schema to JSON Schema is using draft-07 by default
| return ensureStrictJsonSchema(schema, [], schema); | ||
| } | ||
|
|
||
| function isNullable(schema: JSONSchemaDefinition): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should check schema.nullable as well right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, maybe? it's not part of the json schema spec tho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait what, is that openapi only?
| import { OpenAI } from 'openai'; | ||
| import { zodResponsesFunction } from 'openai/helpers/zod'; | ||
| import { z } from 'zod/v3'; | ||
| import { z } from 'zod/v4'; // Also works for 'zod/v3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thats a good callout, I think I asked to switch them to v4 🤦
Co-authored-by: Robert Craigie <robert@craigie.dev>
src/lib/transform.ts
Outdated
| // Handle arrays | ||
| const items = jsonSchema.items; | ||
| if (isDict(items)) { | ||
| // @ts-ignore(2345) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi that you should basically never use ts-ignore, instead you should use ts-expect-error so that you're notified if the ignore comment is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! FYI I pushed a couple small commits improving the internal types and with some small cleanup
Co-authored-by: Robert Craigie <robert@craigie.dev>
Changes being requested
Additional context & links