-
Notifications
You must be signed in to change notification settings - Fork 1
feat: switch from valibot to zod #318
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
Conversation
a90bcaa
to
fa47956
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.
Pull Request Overview
This PR replaces Valibot with Zod for form validation across multiple components.
- Swap all
valibotResolver
and Valibot schema imports tozodResolver
and Zod primitives - Refactor every schema definition to use Zod (
z.object
,z.string
,z.enum
, etc.) - Update type aliases to use Zod’s
infer
,input
, andoutput
utilities
Reviewed Changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/components/Company/StateTaxes/StateTaxesForm/StateTaxesForm.tsx | Switched from Valibot to Zod for dynamic form schema creation and resolver |
src/components/Company/StateTaxes/StateTaxesForm/Form.tsx | Converted static form schema and types to Zod |
src/components/Company/PaySchedule/usePaySchedule.ts | Refactored PaySchedule schema to Zod and updated types |
src/components/Company/PaySchedule/PaySchedule.tsx | Updated useForm resolver to zodResolver |
src/components/Company/Locations/LocationForm/LocationForm.tsx | Switched resolver import from Valibot to zodResolver |
src/components/Company/Locations/LocationForm/Form.tsx | Converted location schema and types to Zod |
src/components/Company/FederalTaxes/useFederalTaxes.ts | Refactored federal tax schema to Zod and updated types |
src/components/Company/FederalTaxes/FederalTaxes.tsx | Updated form resolver and payload type assertions for Zod schemas |
src/components/Company/BankAccount/BankAccountVerify/Form.tsx | Converted bank verification schema to Zod |
src/components/Company/BankAccount/BankAccountVerify/BankAccountVerify.tsx | Switched form resolver to zodResolver |
src/components/Company/BankAccount/BankAccountForm/Form.tsx | Refactored bank account form schema to Zod |
src/components/Company/BankAccount/BankAccountForm/BankAccountForm.tsx | Updated resolver to zodResolver |
src/components/Company/AssignSignatory/InviteSignatory/InviteSignatoryForm.tsx | Converted invite-signatory schema to Zod |
src/components/Company/AssignSignatory/InviteSignatory/InviteSignatory.tsx | Updated resolver to zodResolver |
src/components/Company/AssignSignatory/CreateSignatory/Schema.ts | Refactored signatory-creation schema to Zod |
src/components/Company/AssignSignatory/CreateSignatory/CreateSignatoryForm.tsx | Updated type inference using Zod |
src/components/Company/AssignSignatory/CreateSignatory/CreateSignatory.tsx | Switched resolver to zodResolver |
src/components/Company/AssignSignatory/AssignSignatory.tsx | Refactored selection schema and resolver to Zod |
src/components/Common/SignatureForm/SignatureForm.tsx | Converted signature form schema and resolver to Zod |
package.json | Replaced valibot dependency with zod |
Comments suppressed due to low confidence (3)
src/components/Company/AssignSignatory/InviteSignatory/InviteSignatoryForm.tsx:9
- [nitpick] Consider renaming
emailMismatchError
toEMAIL_MISMATCH_ERROR
to signal it's a constant and align with naming conventions.
const emailMismatchError = 'email_mismatch'
src/components/Company/StateTaxes/StateTaxesForm/StateTaxesForm.tsx:44
- Add unit tests for the dynamic schema generation in
StateTaxesForm
to verifyz.preprocess
correctly handles edge cases like empty or invalid numeric inputs.
const { dynamicSchema, defaultValues } = useMemo(() => {
src/components/Company/AssignSignatory/InviteSignatory/InviteSignatoryForm.tsx:1
- The schema uses
nameValidation
but it's not imported; addimport { nameValidation } from '@/helpers/validations'
.
import { z } from 'zod'
src/components/Company/AssignSignatory/CreateSignatory/CreateSignatory.tsx
Show resolved
Hide resolved
fa47956
to
9fd0112
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.
Looks good at a high level! question on the non empty syntax with the min length. Will make sure @dmortal can take a look since he has more familiarity with valibot
signature: v.pipe(v.string(), v.nonEmpty()), | ||
confirmSignature: v.literal(true), | ||
export const SignatureFormSchema = z.object({ | ||
signature: z.string().min(1), |
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 like you already resolved the copilot one above lol, but is there a non empty equivalent function? or is checking a min length of one the sanctioned way of doing 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.
👍🏻
return true | ||
}, | ||
{ | ||
message: 'State WC class code is required when stateWcCovered is true', |
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.
i18n?
@@ -90,12 +90,12 @@ export const Root = ({ employeeId, className }: DeductionsProps) => { | |||
}, [currentDeduction]) | |||
|
|||
const includeDeductionsFormMethods = useForm<IncludeDeductionsPayload>({ | |||
// resolver: valibotResolver(IncludeDeductionsSchema), | |||
// resolver: zodResolver(IncludeDeductionsSchema), |
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 is this commented out?
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.
Not sure, the existing resolve was commented out, but my fine and replace just replaced the word so no real change here.
z.union([ | ||
// Case 2a: enableSsn is true | ||
z.object({ | ||
...PersonalDetailsCommonSchema.shape, |
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 spreading was very particular to valibot - are we sure this is the right way to do this with 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.
Good call out, this works with Zod BUT it looks liek merge and extend are better approaches. I'm reviewing this now and will test it and get another PR up shortly.
This pull request contains changes that allow us to switch from Valibot to Zod due to the peer dependency and integration from our API client.