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

[✨Feature Request]: Add similar functionality like yup.when() #145

Closed
xsjcTony opened this issue Sep 5, 2023 · 44 comments
Closed

[✨Feature Request]: Add similar functionality like yup.when() #145

xsjcTony opened this issue Sep 5, 2023 · 44 comments
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@xsjcTony
Copy link

xsjcTony commented Sep 5, 2023

Hey, thanks for the great library.

As a validation library, .when() API is always a great way to do conditional validating.

However, this is not supported in zod (colinhacks/zod#1394) (look at how many 👍 there are), and seems the zod team is 300% 👎 for building this... What a pity😥

As I jumped into it for a while, I totally understand that it's pretty hard to maintain the type-safety if this is implemented, but idk if there's a way to do so. As I said, in the end we are validating libraries, of course type-safety is important, but I do think providing such user-friendly API would be much more important.

Currently, I'm using zod, and I'd like to switch to valibot. When migrating from yup to zod, the lack of .when() almost killed me☠...

I know it's kind of doable via the pipeline in valibot, just like refine() / superRefine() in zod, but it's still a huge pain... Both are more of a workaround instead of a proper solution.

Feel free to say that this is impossible or you are not willing to build that, that's totally fine, as I know the where's the difficulty under the hood, but yeah, what if there's a chance that the dream comes true😂 Thanks💚

If there's any duplicating issues already, please feel free to link them and close this one, as I searched when but nothing appears to be there.

@fabian-hiller fabian-hiller self-assigned this Sep 5, 2023
@fabian-hiller fabian-hiller added duplicate This issue or pull request already exists priority This has priority enhancement New feature or request and removed duplicate This issue or pull request already exists labels Sep 5, 2023
@fabian-hiller
Copy link
Owner

Thank you for creating this issue. I will try to investigate a when method next week. This issue could also be related to #5.

@demarchenac
Copy link

Hi! Having a .when would be awesome! it would simplify the logic to define conditional rules or rules that depend on another field. It would be ✨Awesome✨ to know the position that valibot would take regarding the .when implementation

@ziyak97
Copy link

ziyak97 commented Sep 6, 2023

This enhancement also seems to be exactly what i have been looking for here - #120

@demarchenac
Copy link

Here's an idea I had to add a workaround of .when() as a pipeline method for the object schema, as an example a schema with when would look like:

const schema = object(
  {
    firstName: string([
      minLength(3, "Needs to be at least 3 characters"),
      endsWith("cool", "Needs to end with `cool`"),
    ]),
    lastName: nullish(string()),
  },
  [
    when({
      field: "lastName",
      dependsOn: "firstName",
      constraint: ({ lastName }) => lastName?.length > 0  ?? false,
    }),
  ]
);

Maybe we could re-write this into something similar to:

const schema = object({
  firstName: string([
    minLength(3, "Needs to be at least 3 characters"),
    endsWith("cool", "Needs to end with `cool`"),
  ]),
  lastName: when(string(), {
    dependsOn: "firstName",
    constraint: ({ lastName }) => lastName?.length > 0  ?? false,
  }),
});

@fabian-hiller
Copy link
Owner

@demarchenac thank you for your feedback on this. I will think about it.

@xsjcTony
Copy link
Author

xsjcTony commented Sep 7, 2023

I know it might be hard, but it would be really good if we could append another transform based on the condition just like in yup sometimes we need to omit a specific field based on certain condition, so there won't be a value (because it's still filled in the input element) in the payload.

E.g.

type IDType = 'driver_licence' | 'passport';

const schema: ObjectSchema<IdDetailsFormSchema> = yup.object({
  primary_id_type: yup.string<'driver_licence' | 'passport'>().required(),
  driver_licence_number: yup.string()
    .when('primary_id_type', {
      is: (value: IDType): boolean => value === 'driver_licence',
      then: schema => schema.required(),
      otherwise: schema => schema.transform(() => void 0)
    })
});

This way, even user still has driver's license number input filled in, it won't be included in the final payload because it's been transformed into undefined.

This is just a potential add-on, which is good to have. 💚

@xsjcTony
Copy link
Author

xsjcTony commented Sep 7, 2023

yup.when() basically splits the schema for that specific field into two branches, where all following constraints / transforms are sticking with that branch only.

Based on @demarchenac 's idea, we might add another transform option to perform such task, but I'm not sure if this is a good idea because if it's different on the high-level (i.e. schema does not spilt into different branch), it would be a nightmare to add all available further actions as object properties. e.g.

lastName: when(string(), {
  dependsOn: "firstName",
  constraint: ({ lastName }) => lastName?.length > 0  ?? false,
  transform: // ...
  // ...: ...
}),

@fabian-hiller
Copy link
Owner

I took a closer look at Yup's .when. I understand that Colin does not like the API and has not implemented it in Zod yet. But on the other hand I see the DX advantages of such an API. So I have been thinking about how we can make this API sexy for Valibot. Below are two examples:

// Validate based on a sibling
const Schema1 = object({
  type: enumType(['user', 'admin']),
  token: string([
    when('sibling', (input) => input?.type === 'admin', {
      then: startsWith('admin_'),
      else: startsWith('user_'),
    }),
  ]),
});

// Validate based on a child
const Schema2 = object(
  {
    type: enumType(['user', 'admin']),
    token: string(),
  },
  [
    when('child', (input) => input.type === 'admin', {
      then: custom((input) => input.token.startsWith('admin_')),
      else: custom((input) => input.token.startsWith('user_')),
    }),
  ]
);

Instead of a single validation function, it should be possible to pass a pipeline of functions to then and else. What do you think of this idea? I appreciate any feedback.

@xsjcTony
Copy link
Author

Yeah, this does look good to me, but when using the second case, or not only for the when(), this might happen to all Valibot object pipelines.
The later pipeline is not executed if something fails in the earlier pipeline.

colinhacks/zod#2524 This is an known issue in zod, and even affecting those libs based on zod https://vee-validate.logaretm.com/v4/integrations/zod-schema-validation/

I'm not sure if this is intended behaviour... as both sides have their valid point, maybe consider about this. This is one of the issue when() wants to resolve I think.

@fabian-hiller
Copy link
Owner

fabian-hiller commented Sep 10, 2023

The later pipeline is not executed if something fails in the earlier pipeline.

Are you sure about that? We revised this a few weeks ago.

Edit: Thanks to this comment, I now understand what you mean.

@Karakatiza666
Copy link

Karakatiza666 commented Sep 10, 2023

Disclaimer: just came across this during my research, not very familiar with valibot, only considering it for our project; just wanted to insert my 2 cents.
What also would be great is option to have 'when'-like feature to augment the schema outside of its definition, e.g.:

const schema = augmented(
  object(
    {
      auth: enumType(['pass', 'oauth']),
    }
  ),
  [
    when('child', (obj) => obj.auth === 'pass', {
      then: object({ login: string(), password: string() }),
    }),
    when('child', (obj) => obj.auth === 'oauth', {
      then: object({ bearer: string() }),
    }),
  ]
)
/*
scema :: {
  auth: 'pass' | 'oauth',
  login: 'string' | undefined,
  password: 'string' | undefined,
  bearer: 'string' | undefined
}
*/

Bad object schema for storing app state, useful when dealing with APIs.
I don't think yup can even do this?

@fabian-hiller
Copy link
Owner

fabian-hiller commented Sep 10, 2023

@Karakatiza666 you should use union or variant in this case:

// Normal union
const Schema1 = union([
  object({
    auth: literal('pass'),
    login: string(),
    password: string(),
  }),
  object({
    auth: literal('oauth'),
    bearer: string(),
  }),
]);

// Discriminated union
const Schema2 = variant('auth', [
  object({
    auth: literal('pass'),
    login: string(),
    password: string(),
  }),
  object({
    auth: literal('oauth'),
    bearer: string(),
  }),
]);

@demarchenac
Copy link

demarchenac commented Sep 10, 2023

@fabian-hiller It sounds really helpful for most co-dependent input field validations that I can think of!
But I do agree with @xsjcTony, it seems that the pipeline methods passed on to the object schema only seems to run if all of the schema fields are valid. I don't know if this is a behavior that can be changed though since it seems to be the default behavior. Also, I did see that this is already a known issue.

Lastly, regarding the syntax of your proposal for the when method. It looks awesome ✨
I want to read more about this!
Thanks for your great work!

P.S: I do see myself using the first syntax more often than the second one

@fabian-hiller
Copy link
Owner

Thank you for your feedback. Do we even need the second when('child', ... solution?

Is ist hard wo make both APIs typesafe. My plan is to type input at when('sibling', ... as Record<string, unknown>.

@demarchenac
Copy link

Yeah in all the scenarios I'm currently working on the validations are more like the first example of when('sibling', ...) based.

And I agree with it being typed as Record<string, unknown>, the developer should know what he's doing, or we as developer should be allowed to pass an expected type for it.

@demarchenac
Copy link

demarchenac commented Sep 10, 2023

The most common examples that I've been working on are sibling based validations:

const SomeSchema = object({
    ...fields
}, [
    fieldRequiredWhen(
        {
            field: { key: 'someField', is: (field) => Boolean(field && field.length > SOME_VALUE /* e.g. 6 */) },
            dependsOn: { key: 'sibling' }
        },
        'This field is required'
    ),
    fieldRequiredWhen(
        {
            field: { key: 'otherField' },
            dependsOn: { key: 'otherSibling', is: (otherSibling) => otherSibling.length >= SOME_LENGTH /* e.g. 8 */ }
        },
        'This field is required'
    ),
])

@demarchenac
Copy link

demarchenac commented Sep 10, 2023

Also, I thought of a grouped validation for some fields sharing the required validation when a sibling has some value:

const SomeSchema = object({
    ...fields
}, [
    fieldsRequiredWhen(
        {
            fields: ['dependsOnSibling', 'alsoDependsOnSibling', ...dependantFields, 'lastFieldDependingOnSibling'],
            dependsOn: { key: 'sibling', is: (sibling) => sibling === 'some value' }
        },
        'This field is required'    // or something along those lines.
    )
])

So, I can definitively see the power that the when method allows us to implement really flexible schemas out of the box.

@demarchenac
Copy link

demarchenac commented Sep 10, 2023

The only issue that I currently have with the provided examples above are the runtime issues in which theses validations are only visible to a user if all of the fields validations are successful.

@fabian-hiller
Copy link
Owner

Thank you for your feedback and contribution! I'll give it some thought over the next few days.

@xsjcTony
Copy link
Author

Yeah I totally agree with all the above, and the real difficulty here is to make it type-safe (and I guess that's why zod doesn't want to implement it), so unknown should do the job, where developers should be responsible for what they are doing.

The only issue that I currently have with the provided examples above are the runtime issues in which these validations are only visible to a user if all of the fields validations are successful.

And yeah... this is worrying me. Thanks for the collection.💚

@Demivan
Copy link
Contributor

Demivan commented Sep 11, 2023

I like the idea I proposed in #76 (comment)

Code example
const schema = object({
  name: string(),
  password: string(),
  confirmPassword: string(),
}, [
  {
    type: 'pre', // Run before the validation
    transform: (data: unknown) => data.password === data.confirmPassword, // Input will be unknown
  },

  {
    type: 'post', // Run after validation (default)
    transform: (data: Typed) => data.password === data.confirmPassword, // Input will be typed

    // Allow specifying path and error message instead of throwing validation error
    path: ["confirmPassword"],
    message: "Both password and confirmation must match",
  },

  // Existing pipes are supported. Are the same as 'post' pipes
  custom((data) => data.password === data.confirmPassword),

  {
    // Object schema only
    type: 'partial', // Run after successfull validation of specified fields
    fields: ['password', 'confirmPassword'], // Array elements can be typed
    transform: (data: Pick<Typed, 'password' | 'confirmPassword'>) => data.password === data.confirmPassword, // Input will contain specified fields
  },
])

The only thing Valibot needs is to support are partial (that run then part of the object schema is valid) pipelines and other proposals in this thread (fieldsRequiredWhen and when) could be implemented using them. And all this can be type safe.

What I don't like about non-pipeline solution, is that it is, by definition, cannot be typed. And it requires one field to know the existence of another, that should be the parent schema concern.

@fabian-hiller
Copy link
Owner

I would be interested to hear what others think about @Demivan's idea.

@demarchenac
Copy link

@Demivan By non-pipeline solution are you referring to the one that's not within the pipeline for the object schema?
I'm asking since both examples from @fabian-hiller are within a pipeline, the difference being the type of pipeline since the first one is within an string schema and the second one is within an object schema

@ziyak97
Copy link

ziyak97 commented Sep 12, 2023

i am not a fan of @Demivan idea. i like the original when child solution proposed it's easy to understand.

@xsjcTony
Copy link
Author

xsjcTony commented Sep 13, 2023

So there are a few things on my mind:

  1. The sibling when is easy to understand, but it's very hard (or impossible?) to ensure the type safety there, so unknown will be used and developers need to type guard it, which might be awkward (yup uses any🤣 in this case). ⚠️And this is supposed to be run once the specific field schema is valid (who is using when), not the whole object schema.
  2. Will the parent when going to run even if the object schema validation fails?
  3. The partial idea from above should still be implemented if 2 is negative since this will be a huge issue in form validation.

@Karakatiza666
Copy link

@Karakatiza666 you should use union in this case:

const Schema = union([
  object({
    auth: literal('pass'),
    login: string(),
    password: string(),
  }),
  object({
    auth: literal('oauth'),
    bearer: string(),
  }),
]);

@fabian-hiller There is a semantic problem with this approach: when a derived field fails validation (e.g. bearer minLength isn't satisfied) corresponding union branch fails; now another branch is tested, and field 'auth' with value 'oauth' doesn't match the constraint auth: literal('pass'). In effect, both auth and bearer fail validation (which is confusing in context of user form).

@fabian-hiller
Copy link
Owner

Thanks for the hint. For this we probably need something like discriminatedUnion or switch (related to issue #90).

@unlinking
Copy link

discriminatedUnion is very useful in form validation with different options

@fabian-hiller
Copy link
Owner

Currently, an object's pipeline is not executed if an issue occurs during validation of its entries. This causes problems, especially with forms, because each field must be valid before the object's pipelines can check the relationships between the values of the fields.

I think it makes sense to change this behavior and not run the object's pipeline only when a "type" issue has occurred. For all other issues, which are validated by the pipelines of the entries, we still run the pipeline of the parent object. This would make the behavior similar to refine in Zod. I welcome feedback on this.

@xsjcTony
Copy link
Author

Yeah exactly. This is a huge issue in zod that it makes things so difficult when validating form. Glad to hear we have a plan to run object's pipeline even if field validation failed.

@demarchenac
Copy link

@fabian-hiller This sounds awesome!

@micah-redwood
Copy link

Just wanted to +1 this issue! Being able to define conditional properties would be awesome for schemas that validate forms with conditional logic.

I recently wrote a library for React Hook Forms + Zod (that should be Valibot-compatible as well) where I had to define conditional logic outside of Zod (due to it not supporting this sort of thing), set the corresponding conditional properties in the schema to .optional(), and then manually prune any hidden fields before validation.
rhf-conditional-logic library - Article w/ some more context on the "conditional logic" use case

It would be awesome if Valibot could go a step beyond merely optional properties and actually define conditional properties within the schema. Of course, they'd need to be introspectable so that other libraries could extract the conditional logic (and dependencies) and expose them to the UI. Hope this isn't too off-topic, just wanted to share what I expect would be a very common use-case for this feature.

@fabian-hiller
Copy link
Owner

How would you design the API for this? I would be happy to see pseudocode. I plan to tackle this on the weekend and next week.

@micahjon
Copy link

How would you design the API for this? I would be happy to see pseudocode. I plan to tackle this on the weekend and next week.

Good question. There are a couple different parts to this puzzle.

First off, how to best define a condition function and its dependencies. You could go the Yup route and do it explicitly, e.g.

const condition = [
    ["sibling1", "sibling2"], // dependencies
    (sibling1, sibling2) => sibling1 > sibling2 // condition
]

or you could do it implicitly by using some sort of getter that tracked dependencies automatically. Signals libraries tend to this, which is pretty neat, e.g.

const condition = (get) => get("sibling1") > get("sibling2")

Next, you'll need to decide if the condition applies to a single field or a group of fields in the schema. The former would be simpler, but it is pretty common on forms to have a bunch of fields depend on a single condition. An example of both APIs...

// single conditional field example
{
    companyName: string(),
    companyEmail: conditional(
        // show "company email" field if company isn't one we've already seen
        get => !knownCompanyNames.includes(get("companyName"))
        string([email()]),   
    )
}

// group of fields sharing the same condition (a "branch" in the schema tree, schema would ideally be a union)
{
    companyName: string(),
    ...conditionalProperties(
        // show "company email" and "company location" fields if company isn't one we've already seen
        get => !knownCompanyNames.includes(get("companyName")),
        {
              companyEmail: string([email()]),
              companyPostalCode: string([minLength(5)]),
              // a nested conditional...
              openHours: conditional(
                   // show "open hours" if company is nearby
                   get => get("companyPostalCode").startsWith("9720"),
                   string(),
              )
        },
    )
}

There is one glaring omission in these above examples, and that's what actually happens to the schema based on the condition. For form conditional logic, we're just showing & hiding fields (and only validating visible fields) so schema-wise we're either validating a field -or- omitting the field entirely. This is completely different from Zod's refine, which doesn't mutate the schema's shape at all and just adds additional validation. I assume with the above API users would want to be able to mutate the schema, not just add/remove fields. Here's an example of how that might look...

// only show "company email" field if company isn't one we've already seen (otherwise ignore & omit it)
{
    companyName: string(),
    companyEmail: conditional(
        get => !knownCompanyNames.includes(get("companyName")),
        [ string([email()]), omit() ],
     )
}
// only require valid "company email" field if company isn't one we've already seen (otherwise it's optional)
{
    companyName: string(),
    companyEmail: conditional(
        get => !knownCompanyNames.includes(get("companyName")),
        [ string([email()]), string() ], 
     )
}

or for multiple fields...

{
    companyName: string(),
    ...conditionalProperties(
        // show "company email" and "company location" fields if company isn't one we've already seen
        get => !knownCompanyNames.includes(get("companyName")),
        [
            {
                companyEmail: string([email()]),
                companyPostalCode: string([minLength(5)]),
                // a nested conditional...
                openHours: conditional(
                     // show "open hours" if company is nearby
                     get => get("companyPostalCode").startsWith("9720"),
                     [ string(), omit() ]
                )
            }, 
            null // don't add these fields if condition is falsy
        ],
    )
}

Implementing something like this seems like a real heavy lift and I'd be in way over my head. Impressed by what you've done with this library thus far. Hope these examples are helpful.

Regardless of what the final API ends up being, ideally it'd be possible to introspect the schema and extract the condition functions and dependency lists for every conditional schema field. This would make it possible for the schema to be the source of truth for a form with conditional logic, as we'd be able to use it to generate hooks for conditional rendering in the UI. Probably out of scope, but something to keep in mind, as having introspectable schemas means that people can do a lot more with them!

@fabian-hiller
Copy link
Owner

Thank you for your detailed response. I will take it under consideration.

@fabian-hiller
Copy link
Owner

Previously, the pipelines of complex schemas were only executed if there were no issues. With the new version v0.22.0, pipelines are now always executed if the input matches the data type of the schema, even if issues have already occurred. In addition, the new method forward helps you to forward issues to a child.

In summary, it is now possible to compare two fields, for example password1 and password2, and in case of an error, forward the issue with the error message to password2:

import { custom, email, forward, minLength, object, string } from 'valibot';

const RegisterSchema = object(
  {
    email: string([
      minLength(1, 'Please enter your email.'),
      email('The email address is badly formatted.'),
    ]),
    password1: string([
      minLength(1, 'Please enter your password.'),
      minLength(8, 'Your password must have 8 characters or more.'),
    ]),
    password2: string(),
  },
  [
    forward(
      custom(
        (input) => input.password1 === input.password2,
        'The two passwords do not match.'
      ),
      ['password2']
    ),
  ]
);

I look forward to hearing your feedback on this solution.

@fabian-hiller
Copy link
Owner

Update: Previously, check was not executed if the email field was untyped. This problem can now be solved with our new partialCheck action.

import * as v from 'valibot';

const RegisterSchema = v.pipe(
  v.object({
    email: v.pipe(
      v.string(),
      v.nonEmpty('Please enter your email.'),
      v.email('The email address is badly formatted.'),
    ),
    password1: v.pipe(
      v.string(),
      v.nonEmpty('Please enter your password.'),
      v.minLength(8, 'Your password must have 8 characters or more.'),
    ),
    password2: v.string(),
  }),
  v.forward(
    v.partialCheck(
      [['password1'], ['password2']],
      (input) => input.password1 === input.password2,
      'The two passwords do not match.',
    ),
    ['password2'],
  ),
);

@Demivan
Copy link
Contributor

Demivan commented Jun 19, 2024

There is an issue with partialCheck:

const schema = v.pipe(
  v.object({
    id: v.pipe(v.string(), v.minLength(3), v.transform(s => Number(s))),
    password: v.string(),
  }),
  v.partialCheck(
    [['id'], ['password']],
    (input) => {
      console.log('Checking', input)
      return input.id > 10
    },
    'Id must be greater than 10',
  ),
)

const output = v.safeParse(schema, {
  id: '123',
  password: '123456',
})
// Checking { id: 123, password: '123456' }

const output = v.safeParse(schema, {
  id: '12',
  password: '123456',
})
// Checking { id: '12', password: '123456' }

Input types are incorrect when there in an issue and a transform.

I think partialCheck should wait for fields to be valid, not just typed.

@fabian-hiller
Copy link
Owner

Thanks for reporting @Demivan. This happens because partialCheck currently only looks for schema issues and ignores validation issues, but I completely forgot about transformations. We could easily fix this by stopping partialCheck for both schema and validation issues, but the drawback is that the error message from partialCheck is only shown when all selected fields are free of issues. The optimal solution would be to track a typed state for each path, but this would affect the code of the entire library just to improve the behavior of partialCheck. What are your thoughts on this?

@Demivan
Copy link
Contributor

Demivan commented Jun 19, 2024

I think it is fine to wait for specified fields to be validated. For use cases like password confirmation there really is no need to check confirmation if the password is incorrect anyway.
Maybe someone has more examples, but all schemas that I have would be fine if partialCheck only executed after field validation.

@fabian-hiller
Copy link
Owner

Will release a fix in the next hour

@fabian-hiller
Copy link
Owner

I am closing this issue as I am not sure if a when API is still needed, but I am open to discussing ideas and will reopen this issue if there are good reasons.

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

9 participants