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

Custom error message of missing object entries changed in v1.0.0-beta.13 #1034

Open
fabian-hiller opened this issue Jan 24, 2025 · 14 comments
Open
Assignees
Labels
fix A smaller enhancement or bug fix priority This has priority question Further information is requested
Milestone

Comments

@fabian-hiller
Copy link
Owner

The error message returned for missing object entries has changed in v1.0.0-beta.13. Previously, the custom error message of the nested field was used. Now it uses the custom error message of the objectschema. With this issue we want to investigate if we should use the custom message of the nested field instead. Here is a playground and here is a code example:

import * as v from 'valibot';

const Schema = v.object({
  field: v.string('My custom error'), // This error message if "ignored"
});

const result = v.safeParse(Schema, {});
@fabian-hiller fabian-hiller self-assigned this Jan 24, 2025
@fabian-hiller fabian-hiller added question Further information is requested fix A smaller enhancement or bug fix labels Jan 24, 2025
@fabian-hiller fabian-hiller added this to the v1 milestone Jan 24, 2025
@fabian-hiller fabian-hiller added the priority This has priority label Jan 24, 2025
@fabian-hiller
Copy link
Owner Author

I have investigated this issue. Theoretically, we can easily support the old behavior and return an issue of the nested field instead of an object issue for required fields that are missing. But there are some drawbacks.

The new and current behavior is objectively more correct. There is a difference between a missing property and a present but undefined property. A missing property is something that the object schema should detect, because only the object schema has this information and knows its structure. Therefore, it makes sense that an object issue is returned and not the nested schema issue.

This becomes especially clear when using any or unknown for object keys:

import * as v from 'valibot';

const Schema = v.object({ key1: v.any(), key2: v.unknown() });

// ERROR: Type '{}' is missing the following properties from type '{ key1: any; key2: unknown; }': key1, key2
const input: v.InferInput<typeof Schema> = {};

In this case, our schema should return an issue for both properties if they are missing to match the behavior of TypeScript. Our old implementation would ignore this case and treat an empty object as a valid input for the given schema, which is wrong according to the given TypeScript error message. The tricky part is that we can't return an any or unknown issue because both schemas pass any input. They do not know the concept of an issue. Therefore, they would produce a wired error message such as "Invalid input: Expected any but received undefined" and break the type safety of Valibot. And treating missing any and unknown properties differently than other schemas would be inconsistent.

These concerns made me think that the "problem" of this issue might not be our new internal implementation, but the user-defined schema. I may be wrong, but could it be that, for example, you want to validate that the input passed is not an empty string whether the object property is provided or not? If so, you could define that by changing your schema:

import * as v from 'valibot';

const Schema = v.object({
  field: v.optional(v.pipe(v.string(), v.nonEmpty('My custom error')), ''),
});

Now the schema defaults to an empty string for missing properties and returns an issue if this does not match any further validation rules. I know that this is a lot more code to write, and that the old solution was much simpler. That's why I'm interested in hearing your feedback and investigating this issue further.

@fabian-hiller fabian-hiller pinned this issue Jan 25, 2025
@fabian-hiller fabian-hiller changed the title Error message of missing object entries changed in v1.0.0-beta.13 Custom error message of missing object entries changed in v1.0.0-beta.13 Jan 25, 2025
@EltonLobo07
Copy link
Contributor

I think the new and current implementation should be kept. We should figure out how to let users customize the error message for each missing key in the object. In the last code block of your last message, the users won't be able to customize the "missing key" messages, as any custom message inside an entry's schema would indicate "the input has the key and validation failed" instead of "the input did not contain the key". Maybe there are cases where the author of the schema wants to provide a different message for the "missing key" and "validation failed" cases easily. What if we add a way to customize each "missing key" error message like shown in the example below?

import * as v from 'valibot';

const Schema = v.object({
  key1: [
    v.pipe(
      v.string('`key1` should be a string'),
      v.nonEmpty('`key1` cannot be empty'),
    ),
    '`key1` is required',
  ],
  key2: [
    v.pipe(
      v.string('`key2` should be a string'),
      v.nonEmpty('`key2` cannot be empty'),
    ),
    '`key2` is required',
  ],
  // If `key3` is missing, it will use object's error message
  key3: v.number('`key3` should be a number')
});

@fabian-hiller
Copy link
Owner Author

It seems like we have 3 options:

  1. Sticking with the new and current behavior and not changing anything
  2. Return the nested schema's custom error message if available for missing properties (old behavior)
  3. Extend the API (Elton's example) and allow a custom error message for each missing property

Options 2 and 3 make things more complicated and increase the bundle size. I would probably wait for more feedback before changing the behavior or API.

@teddy-dubal
Copy link

Hi,
I use valibot with a vue js, a key can be missing , but required , that why optional directive is'nt appropriate , a custom message is more suitable

@fabian-hiller
Copy link
Owner Author

Thank you for your feedback! Can you provide more details and maybe some example code?

@huseeiin
Copy link

i prefer the old behavior, i think most agree with me

@Bilboramix
Copy link

I like the new behavior, i also think this is the most correct to have.
So, in order to customize the message i like the examples you gave in the discord thread however there's some drawbacks to both.

The first would require a capitalize function call to display in frontend app (Maybe we could have an utility function like flatten for this ?) :

const ObjectSchema = v.strictObject(
  {
    name: v.string(),
    email: v.string(),
  },
  (issue) => (issue.path ? `${issue.path[0].key} is required` : 'Unknown error')
);

And this one require us to keep in sync this object and the schema (But it may be build in to ensure type safety ?):

import * as v from "valibot";

function message(messages: Record<string, string>) {
  return (issue: v.BaseIssue<unknown>): string => {
    return messages[(issue.path?.[0].key as string) ?? ''] ?? issue.message;
  };
}

const ObjectSchema = v.strictObject(
  {
    name: v.string(),
    email: v.string(),
  },
  message({
    name: 'Name is required',
    email: 'Email is required',
  })
);

That said, the old behavior have the advantage to keep it simple for the author so it makes difficult to me to choose between option 1 or 2...

@fabian-hiller
Copy link
Owner Author

I will look again at ways to get the best of both worlds. I have an idea in mind. I will get back to all of you here as soon as possible.

@fabian-hiller
Copy link
Owner Author

I am still investigating this issue. Please provide a 👍 or 👎 if the following workaround is an option for you. I only ask because I am currently investigating several changes to the current behavior, but they all have other drawbacks.

A lot of cases where you might want to set a specific custom message for a missing object property is when working with forms. The reason for this is that you may not have control over how untouched but required form fields are handled. Some form libraries don't initialize them, resulting in a missing object property when representing the form state as an object.

A workaround could be to set the form field as optional in your Valibot schema, but provide an initial value, for example an empty string, as the default value to be used as input to the schema whenever the actual object property is missing or undefined. Now you can use a validation action like nonEmpty to return the same custom error message regardless of whether the property is missing, undefined, or an empty string. Feel free to test the following code example in this playground.

import * as v from 'valibot';

const Schema = v.object({
  email: v.pipe(
    v.optional(v.string(), ''), // <- Empty string as default value
    v.nonEmpty('Please enter your email.'),
    v.email('The email is badly formatted.'),
  ),
});

@tats-u
Copy link
Contributor

tats-u commented Feb 3, 2025

Zod provides 2 kinds of messages.

  • required_error
  • invalid_type_error
z.object({
  name: z.string({ required_error: "Please let me know your name.", invalid_type_error: "The name should be string." }),
  birth_year: z.number().optional()
})

The suggested way will be able to cover the former, but how can or should we easily customize the latter in Valibot?

https://zod-playground.vercel.app?appdata=N4IgzgxgFgpgtgQxALhALwHQHsBGArGCAFwApgAdAOwAJrKE4ZlrMwiAnAS0oHMzr2MAI4BXToIAmAfRjt2Wds3IgACgBsYCMDGoai1RtQDWlLAHdqATywj2dBjAzKANNW4A3BGs7SilgA4wMnIKSiAAKrD2hmBQNmoS1Dg6bFy8TiDUAL4AlM5UtDjiRFBSlpqKLBiUInDJ7CQ52P5EnFj0ao1UuSDOIJ5qIjBgKADaIMBZvRP0jMwAjFMAun3usmBtlCggAMwYAEwALBjzIFlAA

@fabian-hiller
Copy link
Owner Author

fabian-hiller commented Feb 4, 2025

Here is a Valibot schema with a similar behavior. You can try it out in this playground.

I know this code example is a bit verbose, but we have to keep in mind that Zod does not distinguish between missing and existing but undefined object entries, which can lead to bugs in edge cases. This is exactly what we "fixed" in v1.0.0-beta.13, which is the reason for this discussion. 😐

import * as v from 'valibot';

const Schema = v.object({
  name: v.pipe(
    v.optional(v.string('The name should be string.'), () => undefined),
    v.string('Please let me know your name.'),
  ),
  birth_year: v.optional(v.number()),
});

Also keep in mind that Valibot allows you to read the actual input when generating an error message.

Without the new and more accurate validation behavior, this would behave almost exactly like your Zod example.

import * as v from 'valibot';

const Schema = v.object({
  name: v.string(({ input }) =>
    input ? 'The name should be string.' : 'Please let me know your name.'
  ),
});

@rmuchall
Copy link

rmuchall commented Feb 5, 2025

> import * as v from 'valibot';
> 
> const Schema = v.object({
>   email: v.pipe(
>     v.optional(v.string(), ''), // <- Empty string as default value
>     v.nonEmpty('Please enter your email.'),
>     v.email('The email is badly formatted.'),
>   ),
> });

This works nicely but I find the use of v.optional to not be very readable/clear. It indicates to the reader that this property is now optional. I realize it may not fit correctly with the rest of the API design but a better option for readability would be v.required("Email is required"), or v.nonUndefined("Email is required").

@fabian-hiller
Copy link
Owner Author

Is your idea that required or nonUndefined will convert a missing object property to undefined and pass it to its nested schema?

@rmuchall
Copy link

rmuchall commented Feb 5, 2025

Is your idea that required or nonUndefined will convert a missing object property to undefined and pass it to its nested schema?

No, my comment was solely about the readability of the code rather than the implementation. I don't know if it's possible to resolve this issue while still maintaining readability but thought I'd mention it.

If I'm a developer who is new to valibot then v.optional(v.string(), ''), // <- Empty string as default value makes it look like the property is optional rather than the actual effect of setting the default value to an empty string. It works but it's not very readable.

EDIT: Having read https://valibot.dev/guides/mental-model/ I see this is likely not possible.
Perhaps a more readable solution would be to add another validation that would check for undefined, e.g.
v.required("Email is required") or v.notUndefined("Email is required")
if input is undefined then display error "x is required".

Thanks for the great library by the way, it's awesome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A smaller enhancement or bug fix priority This has priority question Further information is requested
Projects
None yet
Development

No branches or pull requests

7 participants