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

Support ZodIntersection in useFormInputProps, ZodPipeline in getInputProps #36

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

solomonhawk
Copy link

@solomonhawk solomonhawk commented May 2, 2023

Fixes #34
Fixes #35

NOTE: This bumps the peer dep on zod to 3.20.0 for ZodIntersection support.

  • Adds a step to useFormInputProps that handles the case where the passed-in ZodType is something other than a ZodObject (e.g. ZodIntersection/ZodEffects)
  • Adds clauses to getInputProps to handle both ZodEffects (recurse into inner schema) and ZodPipeline (infer input type from the pipeline's out type)
  • Removes generics from calls to getParams in example app - these were yielding type errors and didn't seem to be necessary (type is inferred from schema)
  • (maybe) Fixes a bug with ZodOptional - see 8121879

@kiliman I'm not 100% confident in these changes so a close eye would be appreciated. Per my use case and the tests it seems to address the need. I would welcome additional test cases to consider.

I don't have a good example use case for when a field is itself a ZodIntersection e.g.:

z.object({
  field: z.intersection(z.object({ a: z.string() }), z.object({ b: z.number() }))
})

I cleaned up a few unused variables in the process of making these changes.

I thought about support for z.union([...schemas]) and found it difficult to determine what the correct behavior should be, given that unions model OR types, which could lead to confusing situations where an input's props would need to be e.g. text or number.


In case anyone is curious, I found intersections to be crucial when modeling schemas with refinements on the base type (.superRefine calls that need multiple fields in order to layer on additional cross-field validation constraints). Without them, a failed parse on any of the schema fields prevents the refinement from even running which means you only get some of the errors.

More detailed code sample

Not great, dates constraint violation isn't caught when base schema isn't successfully parsed

const schema = z.object({
  name: z.string(), // required string
  startAt: z.date(),
  endAt: z.date(),
}).superRefine(({ startAt, endAt }, ctx) => {
  if (startAt >= endAt) {
    ctx.addIssue({
      code: z.ZodIssueCode.custom,
      message: 'Start date must be before end date',
    })
  }
})

// schema with invalid dates AND missing name
schema.parse({
  name: undefined,
  startAt: new Date('2021-01-01'),
  endAt: new Date('2020-01-01')
})

// 😔 errors do not include date validation from refinement
// ZodError: [
//   {
//     "code": "invalid_type",
//     "expected": "string",
//     "received": "undefined",
//     "path": [
//       "name"
//     ],
//     "message": "Required"
//   }
// ]

// ----------------------

// schema with valid name and invalid dates, refinement validation runs as expected
schema.parse({
  name: "Barry Bluejeans",
  startAt: new Date('2021-01-01'),
  endAt: new Date('2020-01-01')
})

// ZodError: [
//   {
//     "code": "custom",
//     "message": "Start date must be before end date",
//     "path": []
//   }
// ]

Better, intersection allows refinements to produce errors even when other portions of the schema are invalid

const baseSchema = z.object({
  name: z.string(), // required string
})

const datesSchema = z.object({
  startAt: z.date(),
  endAt: z.date(),
}).superRefine(({ startAt, endAt }, ctx) => {
  if (startAt >= endAt) {
    ctx.addIssue({
      code: z.ZodIssueCode.custom,
      message: 'Start date must be before end date',
    })
  }
})

const schema = z.intersection(baseSchema, datesSchema)

schema.parse({
  startAt: new Date('2021-01-01'),
  endAt: new Date('2020-01-01')
})

// 🎉 both errors are reported, refinement ran even though the baseSchema was invalid
// ZodError: [
//   {
//     "code": "invalid_type",
//     "expected": "string",
//     "received": "undefined",
//     "path": [
//       "name"
//     ],
//     "message": "Required"
//   },
//   {
//     "code": "custom",
//     "message": "Start date must be before end date",
//     "path": []
//   }
// ]

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@* or ignore all packages with @SocketSecurity ignore-all

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

⬆️ Updated Package Version Diff Added Capability Access +/- Transitive Count Publisher
zod@3.21.4 3.11.6...3.21.4 None +0/-0 colinmcd94

- the inferred shape of ParamsSchema is not a valid ZodType
- the generic on `getParams` is already correctly inferred from the schema
}

let inputProps: InputPropType = {
name,
type,
}
if (!def.isOptional()) inputProps.required = true
if (!(def.isOptional() || optional)) inputProps.required = true
Copy link
Author

Choose a reason for hiding this comment

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

The "optionality" of a type seemed to get lost when introspecting its inner type (which I think explains the discrepancy in the tests where we're asserting on the generated props for the optional d field from the base test schema but the assertion includes required: true).

@@ -284,7 +284,7 @@ describe('test useFormInputProps', () => {
name: 'c',
required: true,
})
expect(inputProps('d')).toEqual({ type: 'text', name: 'd', required: true })
expect(inputProps('d')).toEqual({ type: 'text', name: 'd' })
Copy link
Author

Choose a reason for hiding this comment

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

d is defined as z.string().optional(), so it should not be required.

@lildesert
Copy link

Hey @solomonhawk,
Any news on this PR? Did you find another workaround?
I'm also using z.coerce.date in one of my schema and it's not working currently.
Thanks!

@solomonhawk
Copy link
Author

@lildesert I don't have any updates here - the PR is offered up for @kiliman to consider. I don't know if they are actively working on remix-params-helper although it looks like a few updates were released at the end of last year.

@kiliman
Copy link
Owner

kiliman commented Feb 28, 2024

I'm sorry for not getting back to you sooner. I haven't used this package much lately. I have my own fork essentially with a different API.

I really should get back to this and clean it up. Also, if anyone would like to maintain this going forward, that would be great!

@solomonhawk
Copy link
Author

solomonhawk commented Feb 28, 2024

I'm sorry for not getting back to you sooner. I haven't used this package much lately. I have my own fork essentially with a different API.

I really should get back to this and clean it up. Also, if anyone would like to maintain this going forward, that would be great!

It's all good. I know how it goes :) I don't have time to contribute more unfortunately.

I do think it's a worthwhile pursuit to have a way to derive input props from a schema definition (the main thing I used this package for) as it reduces duplication and helps prevent a divergence between the validation rules and the form impl. It's a little disappointing that building on top of zod isn't simpler. That may be unavoidable given how closely it maps to TS, inheriting much of the complexity of the type system itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants