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

Conditionally requiring object fields #5

Closed
Kamyil opened this issue Jul 25, 2023 · 27 comments
Closed

Conditionally requiring object fields #5

Kamyil opened this issue Jul 25, 2023 · 27 comments
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@Kamyil
Copy link

Kamyil commented Jul 25, 2023

Hi! Thank you very much for this library. I do very appreciate the focus on performance and very high composability of rules! It's super awesome! Also congratulations of a first release!
I came here with this issue, because to this day I'm using Zod, since it's doing fine for me so far, but my biggest nitpick there is that there is no easy way to declare conditional requiring of the object properties. I wonder if there is a possibility to do that here

What do I mean specifically?

I mean that you can mark an object property as required (I'm assuming that's by default here), but only when given condition is met

How it could look like

f.e.

import { parse, object, string, requiredWhen } from 'valibot';

function getFormValidationSchema(dataToValidate) {
  const formSchema = object({
    character: {
      type: string(),
    },
    shurikensAmount: number([
      requiredWhen(() => dataToValidate.character.type === 'ninja'),
      minRange(1), 
      maxRange(3)
    ]),
  });
  return formSchema;
}

const formValidationSchema = getFormValidationSchema(data);
parse(formValidationSchema)

With data like this

type CharacterType = 'ninja' | 'warrior' | 'wizard';

const data = {
  character: {
     type: 'ninja' as CharacterType
  },
  shurikensAmount: null
};

parse() should throw an error, because shurikensAmount is null and it's required for character.type === 'ninja'
but with data like this

type CharacterType = 'ninja' | 'warrior' | 'wizard';

const data = {
  character: {
     type: 'wizard' as CharacterType
  },
  shurikensAmount: null
};

should be fine, since wizards are not using shurikens

Conclusion

I'm not sure if this is a good idea or not. I'm only giving an idea for a solution for my problem where I struggled with trying to achieve this in Zod. I had to use refine() in this case, but it was more of a workaround rather than proper solution.
link: colinhacks/zod#938 (comment)
Since this library focus on high composability of rules, I wonder if it there is a possibility to do that here.
If this is not a good idea, I will appreciate proper explanation
Have a good day!

@fabian-hiller fabian-hiller self-assigned this Jul 25, 2023
@fabian-hiller fabian-hiller added the enhancement New feature or request label Jul 25, 2023
@fabian-hiller
Copy link
Owner

Thank you for creating this issue. Currently this is possible with our pipeline feature. Below is an example:

import { object, number, enumType, custom, nullable } from 'valibot';

const YourSchema = object(
  {
    character: object({
      type: enumType(['ninja', 'warrior', 'wizard']),
    }),
    shurikensAmount: nullable(number()),
  },
  [
    custom(
      (input) =>
        input.character.type !== 'ninja' ||
        typeof input.shurikensAmount === 'number',
      '"shurikensAmount" is required if character type is "ninja"'
    ),
  ]
);

If you need this pipeline validation more than once, you can offload it to a separate function, which makes the schema cleaner.

import { object, number, enumType, nullable } from 'valibot';
import { checkShurikensAmount } from '~/validations';

const YourSchema = object(
  {
    character: object({
      type: enumType(['ninja', 'warrior', 'wizard']),
    }),
    shurikensAmount: nullable(number()),
  },
  [checkShurikensAmount()]
);

As soon as I have some more time, I will investigate if a special API can make this even easier and type-safe.

@Kamyil
Copy link
Author

Kamyil commented Jul 26, 2023

Very cool 👍 thank you fabian!

@fabian-hiller fabian-hiller added the priority This has priority label Jul 29, 2023
@cannap
Copy link

cannap commented Aug 1, 2023

Hi,
first thanks for this awesome lib!

how can i do this on the field startsAt i want to be ensure the field startsAt is always lower as the field endsAt isBefore is just from date-fns

export const createPollSchema = object(
  {
    question: string([minLength(3, 'Minimum 3 characters')]),
    information: optional(string()),
    id: optional(number()),
    startsAt: coerce(date(), (input) => new Date(input as any)),
    endsAt: coerce(date(), (input) => new Date(input as any)),
    captcha: useDefault(boolean(), false),
    pollOptions: array(pollOptions),
    isClosed: useDefault(boolean(), false)
  },
  [
    custom(
      (input) => {
        console.log(input);
        return isBefore(input.startsAt, input.endsAt);
      },
     'startsAt is higher as endsAt'
    )
  ]
);

thanks

@fabian-hiller
Copy link
Owner

@cannap thank you! If the if-condition is the right way around, your code looks good. I have tested the following code and it works.

import { coerce, custom, date, object, parse } from 'valibot';

const forceDate = (input: any) => new Date(input);

export const Schema = object(
  {
    startsAt: coerce(date(), forceDate),
    endsAt: coerce(date(), forceDate),
  },
  [
    custom(
      (input) => input.startsAt < input.endsAt,
      'The date of "endsAt" and must be after "startsAt".'
    ),
  ]
);

parse(Schema, { startsAt: 0, endsAt: 1000 }); // { startsAt: Date; endsAt: Date }
parse(Schema, { startsAt: 1000, endsAt: 0 }); // Throws error

@cannap
Copy link

cannap commented Aug 1, 2023

yes but how can i add this function to the "startsAt" like: i just have access to the current value and not the whole object inside

startsAt: coerce(**custom(.....)** ,date(), forceDate),

//Pseude code 
 [
    custom(
     'startsAt', //would present the field where the custom validation is for or something 
      (input) => input.startsAt < input.endsAt,
      'The date of "endsAt" and must be after "startsAt".'
    ),
  ]

because i work with veevalidation/valibot
https://github.com/logaretm/vee-validate/tree/main/packages/valibot
but i think there is no way to figure out which field is the custom function is for

//German
mein englisch suckt
Ich brauche einen Weg um der custom funktion zu sagen bei welchem Feld der Fehler raus kommen soll

@fabian-hiller
Copy link
Owner

I think that this is currently only possible through detours. However, I am aware of the problem and I will try to find a solution in the long run.

For example, I could try to provide the entire input in a second parameter at custom, so that the validation can be done directly at startsAt. However, at this point, the validation is not yet complete, so the input could be incorrect and I would have to type it as unknown.

The second option would be to add to custom the ability to specify which field is affected by an error, so that the error can be output in the correct place.

Until then, you can write your own validation function that throws a ValiError with path information to startsAt. Below is an example.

import { coerce, date, object, parse, ValiError } from 'valibot';

const forceDate = (input: any) => new Date(input);

export const Schema = object(
  {
    startsAt: coerce(date(), forceDate),
    endsAt: coerce(date(), forceDate),
  },
  [
    (input) => {
      if (input.startsAt > input.endsAt) {
        throw new ValiError([
          {
            reason: 'date',
            validation: 'custom',
            origin: 'value',
            message: 'Invalid date',
            input: input.startsAt,
            path: [
              {
                schema: 'object',
                input: input,
                key: 'startsAt',
                value: input.startsAt,
              },
            ],
          },
        ]);
      }
      return input;
    },
  ]
);

parse(Schema, { startsAt: 0, endsAt: 1000 }); // { startsAt: Date; endsAt: Date }
parse(Schema, { startsAt: 1000, endsAt: 0 }); // Throws error

@cannap
Copy link

cannap commented Aug 1, 2023

thanks! will try it
it works i forgot to reverse the true to false !isBefore

@demarchenac
Copy link

demarchenac commented Sep 1, 2023

Hi, maybe this is unrelated but when I'm adding a custom step to the PipeLine of an object using react-hook-form with the valibotResolver from @hookform/resolvers/valibot, whenever I try to conditionally require a form field the view frozes.

Currently I've something like this:

const someStringList = ['a', 'b', 'c'];

export const vehicleUsageSchema = object(
  {
    controlField: withDefault(boolean(), false),
    controlledField: nullish(string([custom((input) => someStringList.includes(input))]))
  },
  [
    custom((input) => {
      console.log(input);
      return input.controlField? typeof input.controlledField === 'string' : true;
    }, 'Some error message')
  ]
);

@fabian-hiller
Copy link
Owner

You can use includes validation instead of custom with .includes.

Thanks for the tip. Do you have time to investigate the problem or check with React Hook Form directly? My guess is that the problem is not with Valibot.

@demarchenac
Copy link

Hey, @fabian-hiller thanks for the suggestion maybe I didn't see the includes while reading the methods. Also, I was reading through the issues and PR's for @hookform/resolvers, but didn't find anything related. Could this be related to the fact that they're using valibot v0.12 and not valibot v0.13, I'm asking since I saw that v0.13 introduces a breaking change right?

@fabian-hiller
Copy link
Owner

Actually, this should not be the problem. However, more people have the problem: #76 (comment)

@demarchenac
Copy link

I'll try researching a bit into this issue. Also, I don't think the includes pipeline method would be a helpful replacement since my custom checks if input from the pipeline is included within the values of a given list of strings

@fabian-hiller
Copy link
Owner

You are right. Thank you! Can you share your schema that causes problems with React Hook Forms?

@demarchenac
Copy link

demarchenac commented Sep 4, 2023

Hi, @fabian-hiller I've created a StackBlitz repro for this bug:
https://stackblitz.com/edit/stackblitz-starters-z8ytsk?file=src%2FApp.tsx

It's a form with 4 fields, of which 3 depend on the Required Input field.

You can notice that every time the Required Input triggers an onChange and any of the other fields are undefined, the web view freezes.

@fabian-hiller
Copy link
Owner

I have looked into it and can't figure out the problem. Can you create an issue at React Hook Form Resolvers? Feel free to link me. https://github.com/react-hook-form/resolvers

@demarchenac
Copy link

So I went ahead and read:

I think that this is currently only possible through detours. However, I am aware of the problem and I will try to find a solution in the long run.

For example, I could try to provide the entire input in a second parameter at custom, so that the validation can be done directly at startsAt. However, at this point, the validation is not yet complete, so the input could be incorrect and I would have to type it as unknown.

The second option would be to add to custom the ability to specify which field is affected by an error, so that the error can be output in the correct place.

Until then, you can write your own validation function that throws a ValiError with path information to startsAt. Below is an example.

import { coerce, date, object, parse, ValiError } from 'valibot';

const forceDate = (input: any) => new Date(input);

export const Schema = object(
  {
    startsAt: coerce(date(), forceDate),
    endsAt: coerce(date(), forceDate),
  },
  [
    (input) => {
      if (input.startsAt > input.endsAt) {
        throw new ValiError([
          {
            reason: 'date',
            validation: 'custom',
            origin: 'value',
            message: 'Invalid date',
            input: input.startsAt,
            path: [
              {
                schema: 'object',
                input: input,
                key: 'startsAt',
                value: input.startsAt,
              },
            ],
          },
        ]);
      }
      return input;
    },
  ]
);

parse(Schema, { startsAt: 0, endsAt: 1000 }); // { startsAt: Date; endsAt: Date }
parse(Schema, { startsAt: 1000, endsAt: 0 }); // Throws error

And started tinkering with it. Turns out that replacing the return input statment with return { output: input } statement did the trick.

So, I got my schema working as I wanted. It seems that react-hook-form or @hookform/resolvers/valibot doesn't like the idea of having a custom pipeline method on an object schema, this would require further research.

To conclude, I've got a working example in this StackBlitz. The TL;DR would be that I just removed the custom pipeline method and added my own custom pipeline method.

P:S. @fabian-hiller, I wanted to know if you knew of a way in which I could improve this schema by making it easier to read.

Thanks in advance!

@demarchenac
Copy link

demarchenac commented Sep 7, 2023

Hey, @fabian-hiller I was thinking of defining a custom pipe method like this:

import { type Pipe, ValiError } from "valibot";

type WhenArguments<TValues> = {
  dependsOn: keyof TValues;
  field: keyof TValues;
  constraint: (input: TValues) => boolean;
};

type PipeMethod = <TValues>(
  args: WhenArguments<TValues>,
  message?: string
) => Pipe<TValues>[number];

export const when: PipeMethod =
  ({ dependsOn, field, constraint }, message) =>
  (input) => {
    if (
      input[dependsOn] &&
      typeof input[field] === "string" &&
      constraint(input)
    ) {
      return { output: input };
    }

    let genericMessage = "";

    if (!message) {
      const dependsOnPretty = (dependsOn as string).replace(
        /([a-z])([A-Z])/g,
        "$1 $2"
      );

      const dependsOnLabel =
        dependsOnPretty.charAt(0).toUpperCase() +
        dependsOnPretty.substring(1, dependsOnPretty.length);

      genericMessage = `This field is required when the ${dependsOnLabel} field is provided`;
    }

    throw new ValiError([
      {
        reason: "string",
        validation: "custom",
        origin: "value",
        message: message ?? genericMessage,
        input: input[field],
        path: [
          {
            schema: "object",
            input: input as Record<string, unknown>,
            key: field as string,
            value: input[field],
          },
        ],
      },
    ]);
  };

I wanted to get your feedback on this or to know if could improve it a little bit, thanks in advance!

@demarchenac
Copy link

Here's an example on StackBlitz with the prior idea ☝🏻

@fabian-hiller
Copy link
Owner

@demarchenac the internal implementation has changed with v0.13.0. Please read the release notes: https://github.com/fabian-hiller/valibot/releases/tag/v0.13.0

@demarchenac
Copy link

demarchenac commented Sep 7, 2023

@demarchenac the internal implementation has changed with v0.13.0. Please read the release notes: https://github.com/fabian-hiller/valibot/releases/tag/v0.13.0

Oh ok, so I should use the issue syntax instead of throwing a ValiError error right?

@demarchenac
Copy link

@fabian-hiller ok I understand now. So, the issue syntax doesn't have a path key to specify the error path, thus I ended up throwing a ValiError. I'm new to this library so it took me a while to understand what you were referring to 😅

@fabian-hiller
Copy link
Owner

Exactly. I will add the ability to specify the path soon. Thanks for your code examples regarding this.

@fabian-hiller
Copy link
Owner

v0.15.0 is now available.

@demarchenac
Copy link

I'll try it out when re-writing my fieldRequiredWhen implementation, thanks for getting this through!

@fabian-hiller
Copy link
Owner

@demarchenac feel free to let me know what you think about this.

@demarchenac
Copy link

@fabian-hiller Yeah I was just reading that issue!

@fabian-hiller
Copy link
Owner

I think this issue will be fixed in the next version (v0.31.0). If you think I am wrong or have a question, please ping me here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority This has priority
Projects
None yet
Development

No branches or pull requests

4 participants