Skip to content
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

Add support for nested ZodEffects #1382

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sassanh
Copy link

@sassanh sassanh commented Nov 4, 2024

Relevant: #1319

This extends what was done in #1319 so that it works when multiple refinements are added, for example:

const schema = z
  .object({
    username: z.string().min(4),
    password: z.string().min(8),
    confirmPassword: z.string().min(8),
  })
  .refine((form) => form.password === form.confirmPassword, {
    message: "Passwords don't match",
    path: ["confirmPassword"],
  })
  .refine((form) => form.password !== form.username, {
    message: "Password is the same as the username",
    path: ["password"],
  });

@piotrpospiech can you kindly take a look?
Do you think we can have it in version 4? It doesn't look like a new feature.

@github-actions github-actions bot added Area: Bridge Affects some of the bridge packages Bridge: Zod Affects the uniforms-bridge-zod package labels Nov 4, 2024
@piotrpospiech
Copy link
Collaborator

Hi @sassanh, thanks for the contribution! It seems we can add it to the upcoming 4 or 4.x version. I will come back with detailed answer and review next week.

Copy link
Collaborator

@piotrpospiech piotrpospiech left a comment

Choose a reason for hiding this comment

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

Hi @sassanh, great job with this pull request. Thanks for your input and adding new tests for the chained refine and superRefine functions.

Once those minor adjustments are made, we’ll be glad to proceed with the merge.

packages/uniforms-bridge-zod/__tests__/ZodBridge.ts Outdated Show resolved Hide resolved
packages/uniforms-bridge-zod/__tests__/ZodBridge.ts Outdated Show resolved Hide resolved
export default class ZodBridge<T extends ZodRawShape> extends Bridge {
schema: ZodObject<T> | ZodEffects<ZodObject<T>>;
schema: ZodObject<T> | NestedZodEffect<ZodObject<T>>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
schema: ZodObject<T> | NestedZodEffect<ZodObject<T>>;
schema: ZodObject<T> | ZodEffects<ZodSchema>

We decided we don't want to create a special type with depth limit to handle this. ZodEffects<ZodSchema> will allow nested ZodEffects type. It will be easier to understand, maintain and we don't want to hardcode specific depth limit.

The downside is that it ZodSchema allows any zod type. This is something that we need to accept.

Wrong type is still checked using invariant function in the runtime.

@sassanh
Copy link
Author

sassanh commented Nov 15, 2024

Hi @piotrpospiech, thanks for taking the time to review this!

I've made the changes you suggested in the comments. However, regarding your decision to avoid introducing a new type, I wanted to clarify that it does have a trade-off. The error objects returned by bridge.getValidator()(...) will be of type any.

Personally, I use Zod because it perfectly aligns with type hints. If I weren't using Zod, I'd use Yup, which is already supported by many libraries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Bridge Affects some of the bridge packages Bridge: Zod Affects the uniforms-bridge-zod package
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

2 participants