airjp73 / rvf

Easy form validation and state management for React and Remix
https://rvf-js.io
MIT License
839 stars 66 forks source link

Add more accurate types to Zod helpers #346

Closed fnimick closed 11 months ago

fnimick commented 11 months ago

@airjp73 I did some poking and wasn't sure how to set up the monorepo environment for testing, but I'm 99% sure the type changes in the first commit won't cause any problems. This is simply a readability and type-safety change to remove the need for any casts in the helper functions which use z.preprocess.

For example, in the current setup, doing the following does not produce a type error:

export const text: InputType<ZodString> = (schema = z.number()) =>
  preprocessWithoutUnknown(preprocessIfValid(stripEmpty), schema) as any;

though it should as z.number() does not generate a ZodString input.

By default, z.preprocess broadens the input type of the resulting ZodEffect from the input type of the provided schema to unknown. This can be valuable, but in the cases of these helpers, the input types should remain the same.

By defining and using a version of preprocess which does not perform this broadening, we can avoid the need for the as any casts in all of the helper functions.

I have adapted some of the preprocessors to use in my own projects (sveltekit instead of remix) and ended up needing to refine these types in order to accurately and safely use them with additional preprocessing steps. Figured I'd contribute these fixes back!


The second commit is substantially more risky, but possibly valuable. This modifies InputType to perform type checking on the provided schema, by asserting an intersection between the default schema from an InputType and the provided schema. Previously, e.g. text(z.number()) did not provide a type error, despite the fact that string is not assignable to number. Now, this will throw an error.

I did some basic tests to confirm that e.g. all modifications that allow string to be assigned to the input generic parameter of a ZodType are legal arguments to text(), e.g. text(z.string().optional()) works as expected. Similarly, z.number().nullable() is a valid imput to numeric(). However, I'm not enough of a Typescript expert to state this unequivocally - so please give me feedback on this approach!

EDIT: please note that there are now type errors in the tests, which are testing e.g. how text(z.number()) and numeric(z.string()) behave. I believe these should be type errors, since it doesn't really make sense to me to have fields that coerce certain types of strings also accept non-string schemas, or fields that coerce to number accept non-number schemas.

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
remix-validated-form ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2023 0:24am