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

[Bug?]: Weird validation issues when using nested checkboxes #310

Open
2 of 4 tasks
andreafdaf opened this issue Jul 27, 2023 · 5 comments
Open
2 of 4 tasks

[Bug?]: Weird validation issues when using nested checkboxes #310

andreafdaf opened this issue Jul 27, 2023 · 5 comments
Labels
bug Something isn't working

Comments

@andreafdaf
Copy link

andreafdaf commented Jul 27, 2023

Which packages are impacted?

  • remix-validated-form
  • @remix-validated-form/with-zod
  • @remix-validated-form/with-yup
  • zod-form-data

What version of these packages are you using?

  • remix-validated-form: 5.0.2
  • @remix-validated-form/with-zod: 2.0.6

Please provide a link to a minimal reproduction of the issue.

https://stackblitz.com/edit/remix-run-remix-zthg3e

Steps to Reproduce the Bug or Issue

All fields shown are required, try to submit the form without filling any of them and look at the error messages.

From my understanding of how it should work the result is totally unexpected. Each field should show the error message defined in the schema but:

  • deeply.nested.checkbox: shows "Required" instead of "Deep checkbox is required"
  • nested.checkbox: shows "Required" instead of "Nested checkbox is required" (the sibling text field works correctly)
  • another.checkbox: concurs to validation (non flagging it prevents the submission) but no errors are shown

I think it is related to the fact that unchecked checkboxes don't show up in FormData and the issue cascades from that.

Expected behavior

I expected to see the error messages defined in the Zod schema alongside each errored field.

Screenshots or Videos

image

image

Platform

  • OS: any
  • Browser: any
  • Version: any

Additional context

This issue also affects radios.

@andreafdaf andreafdaf added the bug Something isn't working label Jul 27, 2023
@airjp73
Copy link
Owner

airjp73 commented Jul 27, 2023

You're right that this is because the checkboxes are not included in the FormData at all. In the case of nested.checkbox and deeply.nested.checkbox, using min(1) will never work and you should instead use string({ required_error: "This is required" }). IMO, zfd.checkbox is a little more intuitive to work with for checkboxes, if you're using those helpers.

For the lone another.checkbox, that still doesn't work because the whole object isn't being included in the form data at all. For that one, I wonder if a new zfd.nested helper could help fix the issue.

Here's a stackblitz demoing this: https://stackblitz.com/edit/remix-run-remix-tvvemm?file=app%2Froutes%2F_index.tsx

@andreafdaf
Copy link
Author

Hi @airjp73, first of all thanks for this beautiful project and also for such a quick reply.

As for your first paragraph I got it, although I think it could be something to write about in the docs.

As for the second paragraph, well, we'll add hidden fields alongside nested checkboxes to make things work without having to change schemas or doing extra work.

I think the helper would be a good addition to zfd but IMHO there is some underlying that might be solved directly in the library and would help in many other scenarios as well.
The thing is: the errors returned by @remix-validated-form/with-zod when using nested values stop at the outermost nested value that fails validation, and this is pretty straight forward and expected when validating an object.
When validating a form we cannot control the nesting of the objects directly but rely simply on using flat keys ('some.flat.key') and a helper in the library unfolds the keys into an object, but what we really care about when validating a form are the leaves that usually map to a field of some sort (in this case 'key').

Again IMHO, there is this gap, or mismatch, in the validation logic where FormData treat keys as string, eventually representing a flattened object property, but this library aims at producing a JSON object and expects validation to be built as such. For example, let's analyze the lone another.checkbox:

// this fails to properly validate the form, but succeeds when validating the parsed output of a valid submission
const schema = z.object({
  another: z.object({
    checkbox: z.string({ required_error: 'Another checkbox is required' }),
  }),
})
const validator = withZod(schema);
// this succeeds when validating the form, but fails when validating the parsed JSON output of the validation itself
const schema = z.object({
  'another.checkbox': z.string({ required_error: 'Another checkbox is required' }),
})
const validator = withZod(schema);

What do you think? Is this a valid point?

@airjp73
Copy link
Owner

airjp73 commented Jul 27, 2023

I think you've pretty accurately describe the challenge here, but I definitely think this is something that has to be solved by the schema itself. This issue is as much to do with the limitations of zod as with RVF. As much as I love zod, it treats validating forms is an afterthought and not the main focus the library.

For example: I redid the reproduction using Yup instead (where validating forms is the main focus of the library), and everything works as expected without workarounds: https://stackblitz.com/edit/remix-run-remix-ntsm8s?file=app%2Froutes%2F_index.tsx

So I think a helper in zfd is as much as we can do to really address this situation. It would probably look like this:

const validator = withZod(
  z.object({
    // nested would be a thin wrapper around z.preprocess
    another: zfd.nested({
      checkbox: z.string() // or zfd.checkbox() if you want
    })
  })
);

Since validators need to work on any FormData object with no direct knowledge of the actual makeup of the form, we can't, for example, automatically create an object where you have nested fields in your form. But defining the schema using zfd.nested absolutely could do that.


On another note, if you don't want to use an extra helper, I would probably recommend removing the last layer of nesting instead of adding hidden fields. If the checkbox is the only field in the object, it doesn't seem necessary.

@andreafdaf
Copy link
Author

Thanks again for your reply, I think your points are totally legit and it changed my view of the problem to match yours.

The zfd.nested() helper really looks like the best solution. Can we close this? Should we change this issue to become a FR?

@airjp73
Copy link
Owner

airjp73 commented Aug 11, 2023

Sorry for the delayed response. I'm not super strict about needing to change this to a feature request. Would you have time to help with a PR to add in the helper?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants