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

Unexpected error hoisting #391

Closed
aaronadamsCA opened this issue Jan 24, 2024 · 10 comments · Fixed by #733 or #760 · May be fixed by timvandam/conform#1
Closed

Unexpected error hoisting #391

aaronadamsCA opened this issue Jan 24, 2024 · 10 comments · Fixed by #733 or #760 · May be fixed by timvandam/conform#1
Labels
help wanted Extra attention is needed

Comments

@aaronadamsCA
Copy link
Contributor

aaronadamsCA commented Jan 24, 2024

Describe the bug and the expected behavior

If all of the fields in a fieldset are missing, the error is unexpectedly "hoisted" from each field to the fieldset.

Conform version

1.0.0-rc.0

Steps to Reproduce the Bug or Issue

https://stackblitz.com/edit/remix-run-remix-zhkgb3?file=app%2Froutes%2Fworkaround.2.tsx

const schema = z.object({
  flags: z.object({
    one: z.boolean(),
    two: z.boolean(),
  }),
});

export default function Route(): ReactElement {
  const [form, fields] = useForm<z.infer<typeof schema>>({
    onValidate: ({ formData }) => parseWithZod(formData, { schema }),
  });
  const { allErrors } = form;

  return (
    <FormProvider context={form.context}>
      <Form method="post" {...getFormProps(form)}>
        <FlagsFieldset name={fields.flags.name} />
        <button>Submit</button>
        <pre>{JSON.stringify({ allErrors }, null, 2)}</pre>
      </Form>
    </FormProvider>
  );
}

interface FlagsFieldsetProps {
  name: FieldName<{
    one: boolean;
    two: boolean;
  }>;
}

function FlagsFieldset({ name }: FlagsFieldsetProps): ReactElement {
  const [metadata] = useField(name);
  const fields = metadata.getFieldset();

  return (
    <fieldset>
      <input {...getInputProps(fields.one, { type: 'checkbox' })} />
      <input {...getInputProps(fields.two, { type: 'checkbox' })} />
    </fieldset>
  );
}
  1. Submit the form

Expected result:

{
  "allErrors": {
    "flags.one": [
      "Required"
    ],
    "flags.two": [
      "Required"
    ]
  }
}

Actual result:

{
  "allErrors": {
    "flags": [
      "Required"
    ]
  }
}

Errors at the fieldset level are unexpected, and most forms won't know how to render these.

What browsers are you seeing the problem on?

No response

Screenshots or Videos

No response

Additional context

No response

@edmundhung
Copy link
Owner

edmundhung commented Jan 24, 2024

This is a tricky one 😅 As checkbox does not contribute to the FormData unless it is checked, there is no way Conform can construct the flags object. It is basically sending an empty object to zod and zod stop parsing the details of flags as soon as it finds that flags is not defined.

This would not be an issue if you have a text input inside the flags object, so Conform manages to construct an object even nothing is filled, or if you mark the flag itself as optional (No. This won't work with how zod works).

@edmundhung
Copy link
Owner

I wonder if this is something safe to do with automatic type coercion

const schema = z.object({
  flags: z.preprocess(value => {
    if (typeof value !== 'undefined') {
      return value;
    }

    // Set it a default object if it's not available
    return {};
  }, z.object({
    one: z.boolean(),
    two: z.boolean(),
  }),
});

@aaronadamsCA
Copy link
Contributor Author

aaronadamsCA commented Jan 24, 2024

Yeah, this actually came up by accident—I forgot to append .default(false) to my z.boolean() fields, and the form was silently failing—no validation, no visible errors. I almost wasn't going to report it until I realized it may have other more significant effects.

Your suggested solution looks sound to me, I can't imagine any reason it would be dangerous to default every object in a schema to empty (though I'm hardly an expert in this kind of thing!).

Edit: It turns out this issue can occur whether your boolean fields are optional or required.

@edmundhung
Copy link
Owner

I think the value should default to an empty object only if the object schema is required. If the object is optional, we should keep it as is.

@aaronadamsCA
Copy link
Contributor Author

I agree! I do have at least one use case where I might want to explicitly mark an object as .optional(), and it would be nice if that were still an option.

@aaronadamsCA
Copy link
Contributor Author

aaronadamsCA commented Jan 25, 2024

I updated the StackBlitz to show two different workarounds for two different scenarios.

  • If all of your boolean fields are optional, you can just add .default({}) to the object.
  • If any of your boolean fields are required, you can z.preprocess() the fieldset object using the Object() constructor, which will create and return an empty object if the value is null or undefined (unfortunately Zod lacks a z.coerce.object() to simplify this).

@edmundhung edmundhung added the help wanted Extra attention is needed label Feb 2, 2024
@aaronadamsCA
Copy link
Contributor Author

Note this existing feature request for z.coerce.object(): colinhacks/zod#2786

@aaronadamsCA
Copy link
Contributor Author

aaronadamsCA commented Jul 19, 2024

I've come up with my own coerceObject() helper that appears to be working well.

function coerceObject<T extends z.ZodRawShape>(
  shape: T,
  params?: z.RawCreateParams,
) {
  return new z.ZodEffects({
    schema: z.object(shape, params),
    effect: { type: "preprocess", transform: Object },
    typeName: z.ZodFirstPartyTypeKind.ZodEffects,
  });
}

Here it is with an explicit function return type:

function coerceObject<T extends z.ZodRawShape>(
  shape: T,
  params?: z.RawCreateParams,
): z.ZodEffects<
  z.ZodObject<
    T,
    "strip",
    z.ZodTypeAny,
    z.objectOutputType<T, z.ZodTypeAny, "strip">,
    z.objectInputType<T, z.ZodTypeAny, "strip">
  >,
  z.objectOutputType<T, z.ZodTypeAny, "strip">,
  z.objectInputType<T, z.ZodTypeAny, "strip">
> {
  return new z.ZodEffects({
    schema: z.object(shape, params),
    effect: { type: "preprocess", transform: Object },
    typeName: z.ZodFirstPartyTypeKind.ZodEffects,
  });
}

Benefits:

  • Does what it says, by passing input through the Object constructor.
    • For objects this is a no-op, for null and undefined the result is an empty object.
  • Compared to z.preprocess() this preserves the input type.
    • This is important if a schema uses .transform, since useForm<z.input<typeof schema>, z.output<typeof schema>>() will still work for correctly typing defaultValue and onValidate separately.

Limitations:

  • Since this returns a ZodEffects, the schema can no longer be extended or merged.

@edmundhung, I'm happy to contribute this helper to Conform's Zod package along with some tests and docs, as it solves a large pain point while Zod v3 remains frozen (and v4 in progress). Just let me know if you're interested and I'll try to get to it soon.

@edmundhung
Copy link
Owner

Thanks for the offer @aaronadamsCA. I think it would be better to stay consistent with the current approach in which we will coerce the object automatically. (I have plan to make the schema integration less coupled so we might want this in the future, let's see.)

For now, #733 should resolve the issue.

@aaronadamsCA
Copy link
Contributor Author

Agreed, it looks like a good solution. I just hope there are no use cases that depend on an undefined object remaining undefined! But I can't think of any.

(On the topic of "schema integration less coupled", I've often imagined a factory that could return a parseWithZod instance with custom "when you see [this schema], run [this transformer]" schema preprocessing instructions. Even better if I could override built-in coercions, e.g. I'd prefer to convert empty strings to null. I don't know if that's the kind of thing you had in mind, but if so—great!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
2 participants