-
Notifications
You must be signed in to change notification settings - Fork 98
Add Standard Schema validation support for configuration schema #2122
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
base: master
Are you sure you want to change the base?
Conversation
|
I like the ideia of supporting zod specially now with a lot of AI-related stuff using it. Can you elaborate a bit on why we can't use zod prior to this PR? I thought that we're able to use zod alreay in |
|
You're absolutely right — technically, Zod can be used with The main goals of this PR are:
|
c266d8d to
22298fe
Compare
package.json
Outdated
| "typescript": "5.8.3", | ||
| "typescript-eslint": "8.38.0" | ||
| "typescript-eslint": "8.38.0", | ||
| "zod": "^4.0.5" |
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.
Adding zod like this is going to break others as libraries have been slow to adopt zod v4. See https://zod.dev/library-authors#how-to-support-zod-4
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 still think "zod": "^3.25.0 || ^4.0.0" should be added as a peer dependency, so that npm can manage this. Also, please consider `"joi": "^17.13.0 || ^18.0.0" added as a peer dep., granted that you have tested 'joi v18' and we don't have node 12 compatibility to maintain for this library.
lib/validators/validator.factory.ts
Outdated
| @@ -0,0 +1,67 @@ | |||
| import { type Schema as JoiSchema } from 'joi'; | |||
| import { type ZodType } from 'zod'; | |||
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.
Please use zod/v3 zod/v4/core, respectively. Using zod this way will cause both zod v3 and zod v4 mini to break when using this library.
tests/src/app.module.ts
Outdated
| @@ -1,5 +1,6 @@ | |||
| import { DynamicModule, Inject, Module, Optional } from '@nestjs/common'; | |||
| import Joi from 'joi'; | |||
| import { z } from 'zod'; | |||
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.
Tests should be done across v3, v4, and v4 mini, as they are all supported versions of Zod.
lib/validators/zod.validator.ts
Outdated
| @@ -0,0 +1,39 @@ | |||
| import type { ZodType } from 'zod'; | |||
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.
Ditto.
lib/validators/validator.factory.ts
Outdated
| 'parse' in schema && | ||
| typeof schema.parse === 'function' | ||
| ) { | ||
| return this.createZodValidator(schema as ZodType); |
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.
Ditto
| * @param schema Joi schema | ||
| * @returns JoiValidator instance | ||
| */ | ||
| static createJoiValidator(schema: JoiSchema): Validator { |
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.
why not put these factory methods in validators themselves?
lib/validators/validator.factory.ts
Outdated
| if ( | ||
| schema && | ||
| typeof schema === 'object' && | ||
| 'validate' in schema && | ||
| typeof schema.validate === 'function' | ||
| ) { | ||
| return this.createJoiValidator(schema as JoiSchema); | ||
| } | ||
|
|
||
| // Check if it's a Zod schema | ||
| if ( | ||
| schema && | ||
| typeof schema === 'object' && | ||
| 'parse' in schema && | ||
| typeof schema.parse === 'function' | ||
| ) { | ||
| return this.createZodValidator(schema as ZodType); |
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.
Instead of forcing this check on the user each time, why not use some dependency injection on the factory itself? It's highly unlikely that there would be two different types of validation schemas in the same class or even module. You could use this logic to validate against once instead of each time the createValidator is called.
|
Thanks for the suggestion!
|
nstandif
left a comment
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.
Maybe we could do a StandardSchemaV1 validator instead of just zod? I don't see anything that is implementation specific to zod that we'd need to support. WDYT?
lib/types/zod.type.ts
Outdated
| @@ -0,0 +1,7 @@ | |||
| import type { ZodType as ZodTypeV3 } from 'zod/v3'; | |||
| import type { ZodType as ZodTypeV4 } from 'zod/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.
Should use $ZodType from zod/v4/core.
All Zod Mini schemas extend the z.ZodMiniType base class, which in turn extends z.core.$ZodType from zod/v4/core. While this class implements far fewer methods than ZodType in zod, some particularly useful methods remain.
package.json
Outdated
| "typescript": "5.8.3", | ||
| "typescript-eslint": "8.38.0" | ||
| "typescript-eslint": "8.38.0", | ||
| "zod": "^4.0.5" |
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 still think "zod": "^3.25.0 || ^4.0.0" should be added as a peer dependency, so that npm can manage this. Also, please consider `"joi": "^17.13.0 || ^18.0.0" added as a peer dep., granted that you have tested 'joi v18' and we don't have node 12 compatibility to maintain for this library.
Good point — I agree, there doesn’t seem to be anything strictly tied to Zod here. Supporting StandardSchemaV1 sounds like a better approach. |
I see your point, but since these dependencies are optional, I think it’s clearer and simpler to keep them out of peerDependencies. This way we avoid forcing consumers to install packages they may not need. |
@nstandif DONE! |
nstandif
left a comment
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.
LGTM
|
Once you get this merged in, please make a PR to nestjs/docs.nestjs.com and standard-schema/standard-schema so that others know that standard schema is supported! |
|
Got it — I’m waiting for the merge. |
|
I hope so |
… Zod v3 and v4 - Added a new type definition for ZodType that supports both Zod v3 and v4. - Refactored validator imports to use the new ZodType definition. - Updated AppModule to include methods for Zod v3, v4, and v4 mini schema validation. - Enhanced e2e tests to validate environment variables using the new Zod schema methods.
8ede4e9 to
214f44b
Compare
|
joi also supports Standard Schema. hapijs/joi#3080 |
Yes, but it is only supported as of v18.0.0+. Removing the joi dependency would be breaking. |
|
Yes, just pointing it out here — I don’t want to introduce major changes either, but thought it might be helpful to mention. Thanks! |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number:
#1164
What is the new behavior?
Does this PR introduce a breaking change?
Other information