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

Add an advanced refine method (similar to Zod's superRefine) #597

Closed
ruiaraujo012 opened this issue May 23, 2024 · 36 comments
Closed

Add an advanced refine method (similar to Zod's superRefine) #597

ruiaraujo012 opened this issue May 23, 2024 · 36 comments
Assignees
Labels
enhancement New feature or request priority This has priority

Comments

@ruiaraujo012
Copy link

ruiaraujo012 commented May 23, 2024

Currently, there is no way to directly add custom/global issues to the validation, for example:

{
    "primaryEmail": "foo@email.com",
    "otherEmails": ["bar@email.com", "foo@email.com"]
}

To validate that the primaryEmail is not present in otherEmails there is no method that allows it, because I need some actions that add issues globally.

The forward action doesn't work because I cannot put the key and index of the duplicated email, in this case otherEmails.1.

One possible solution is a method that allow us to add issues to the list of issues already created by valibot validations. Something like:

v.someMethod((inputs, ctx) => {
// ...(my custom validation with ctx.addIssue())...
return ctx.issues // Or other type of return
}) 

Related: #487, #546

@fabian-hiller fabian-hiller self-assigned this May 23, 2024
@fabian-hiller fabian-hiller added enhancement New feature or request priority This has priority labels May 23, 2024
@jansedlon
Copy link

Just wanna say that this is the thing that would allow me to completely switch over to valibot :)

@fabian-hiller
Copy link
Owner

I plan to work on it next week 🙌

@jansedlon
Copy link

Awesome 🤩

@ruiaraujo012
Copy link
Author

Hello @fabian-hiller, from what I can tell, the codemod worked well to me, but I cannot finalize the migration because of this feature request. I don't want to rush you, I just wanted to know if you have an estimate of when it will be ready.

Thanks :)

@fabian-hiller
Copy link
Owner

Probably next week, as I should migrate some old docs pages first. Sorry for the delay.

@ruiaraujo012
Copy link
Author

It's ok, just to be aware ;)

@fabian-hiller
Copy link
Owner

What should we call this super action? One idea is to call it refine, similar to Zod, and another is to just call it action.

@ruiaraujo012
Copy link
Author

I'm not sure, refine seems to represent better what it does, but I like action 🙈

@jansedlon
Copy link

I'd call it refine

@ruiaraujo012
Copy link
Author

Yeah, refine is better.

@fabian-hiller
Copy link
Owner

action might make sense because it gives direct access to the underlying action API. refine might make sense as it has a stronger meaning and makes the code more readable. Do you have alternative names in mind?

@jansedlon
Copy link

Action gives me the impression of a single action or a single purpose thing but idk 😄

@ruiaraujo012
Copy link
Author

What about enhance? Not sure, but came to my mind.

@fabian-hiller
Copy link
Owner

I will think about it. Thank you for your feedback!

@fabian-hiller
Copy link
Owner

I like the names refine and enhance because they are short and match well with the names of the other actions. On the other hand, we distinguish between validation actions and transformation actions, and already support two actions with check and transform, which gives you full control over validation and transformation. Therefore it might make sense to add two functions instead of one as transformations are handled differently than validations.

For example, we could add two functions with the same name but a prefix in front. Since both give you access to the raw implementation a prefix like raw, super or power might make sense. So check and transform are the high level implementation with a nice DX and rawCheck and rawTransform are the low level version giving you full control over the dataset of the pipeline. What do you think of this idea?

@ruiaraujo012
Copy link
Author

The raw prefix is more self explanatory than refine or enhance and you're right, this method is no more than a check with an option to access the error layer and change it by adding new errors.

So I think the rawCheck and rawTransform would be a better name, even if less fancy than enhance or refine.

@fabian-hiller
Copy link
Owner

rawCheck and rawTransform are now available. Please give me feedback about the DX! There is no documentation yet, but here is a simple example showing the API.

import * as v from 'valibot';

const Schema1 = v.pipe(
  v.number(),
  v.rawCheck(({ dataset, addIssue }) => {
    if (dataset.typed && dataset.value <= 0) {
      addIssue({ message: 'Error!!!' });
    }
  })
);

const Schema2 = v.pipe(
  v.string(),
  v.rawTransform(({ dataset, addIssue, NEVER }) => {
    const length = dataset.value.length;
    if (length < 3) {
      addIssue({ message: 'Error!!!' });
      return NEVER;
    }
    return length;
  })
);

@ruiaraujo012
Copy link
Author

I will try soon, but I have a question, if I apply rawCheck to an object, what will be the field with the error? Is it possible to add an issue to a field?

@fabian-hiller
Copy link
Owner

fabian-hiller commented Jun 14, 2024

You can still wrap rawCheck in our forward method or specify the path in addIssue by hand next to the message.

@ruiaraujo012
Copy link
Author

Oh, I didn't remember the forward.

Using the second options:

const Schema = v.pipe(
  v.object({
    email: v.pipe(v.string(), v.email()),
    password: v.pipe(v.string(), v.minLength(8)),
  }),
  v.rawCheck(({ dataset, addIssue }) => {
    if (dataset.typed && dataset.value.email.length < 10) {
      addIssue({
        input: dataset.value.email,
        message: 'Error!!!',
        path: [
          {
            input: dataset.value,
            key: 'email',
            origin: 'value',
            type: 'object',
            value: dataset.value.email,
          },
        ],
      });
    }
  }),
);

It's a bit bad in DX having to populate all those values in path

@ruiaraujo012
Copy link
Author

Otherwise, I find that while you don't have the items method, this work well:

const Schema = v.pipe(
  v.object({
    email: v.pipe(v.string(), v.email()),
    emails: v.array(v.pipe(v.string(), v.minLength(8))),
  }),
  v.rawCheck(({ dataset, addIssue }) => {
    if (dataset.typed) {
      const repeatedIndexes: number[] = [];

      dataset.value.emails.forEach((email, index) => {
        if (email === dataset.value.email) {
          repeatedIndexes.push(index);
        }
      });

      if (repeatedIndexes.length > 0) {
        repeatedIndexes.forEach((index) => {
          addIssue({
            input: dataset.value.emails,
            message: `Repeated ${index}`,
            path: [
              {
                input: dataset.value.emails,
                key: index,
                origin: 'value',
                type: 'array',
                value: dataset.value.emails[index],
              },
            ],
          });
        });
      }
    }
  }),
);

const result = v.safeParse(Schema, {
  email: 'jane@example.com',
  emails: ['jane1@example.com', 'jane@example.com', 'jane3@example.com'],
});

I'll try that with react-hook-forms soon

@fabian-hiller
Copy link
Owner

Thank you for your feedback!

It's a bit bad in DX having to populate all those values in path

I agree, but the other option would be to increase the bundle size and make it less powerful with less control by using a key path as with forward.

@ruiaraujo012
Copy link
Author

I can see that, it's ok, I think this method will be less used, so probably it won't hurt much. Thank you for this addition.

@fabian-hiller
Copy link
Owner

Thank you! Feel free to share more feedback in the long run. In the end, I am interested in the best solution and feedback helps me a lot to make the best decisions.

@jansedlon
Copy link

jansedlon commented Jun 15, 2024

@fabian-hiller good job on the progress! One question, how does this play out with i18n in general? What about specifying custom error messages using i18next in different scenarios using this new function?

@ruiaraujo012
Copy link
Author

ruiaraujo012 commented Jun 15, 2024

@fabian-hiller good job on the progress! One question, how does this play out with i18n in general? What about specifying custom error messages using i18next in different scenarios using this new function?

I usually create a namespace only for forms, and then use it in the schemas.

{
    "validation": {
        "email": "The email is not valid."
        ...
    },
    "required": {
        "email": "The email is required."
        ...
    },
}

Then in the schemas

const Schema = v.object({
    email: v.pipe(v.string(), v.nonEmpty('form:required.email'), v.email('form:validation.email'))
})

And then, using react I pass the message from valibot to t(message)

I could share a sandbox with this example if you want.

@jansedlon
Copy link

@ruiaraujo012 thank you for the example. Coming from zod-i18next, what's honestly truly the best is that you just pass params in addIssue where you define namespace and key and it's almost magically translated. If there was something close to this, it would be amazing. What do you think @fabian-hiller , could you look at that? Or I can try to find out how that params works exactly.

@ruiaraujo012
Copy link
Author

Oh, I see, I looked into zod-i18next, actually it'll be a nice addition. I think you could open an issue describing that and.

@jansedlon
Copy link

Will do :)

@fabian-hiller
Copy link
Owner

Have you read this guide? Paraglide JS looks nice. I am happy to answer questions about i18n.

@jansedlon
Copy link

jansedlon commented Jun 17, 2024

@fabian-hiller Yeah, i know about Paraglide, however we don't really have the time to migrate to paraglide a i had some issues with it 🙈 We have (probably) thousands of translations keys right now and the migration would take a lot of time.

In valibot i use setSpecificMessage right now which is bounded to a specific validator, e.g.

v.setSpecificMessage(v.url, (issue) =>
    t("url", { received: issue.received, ns: "valibot" }),
  );

In zod-i18next you register error map like this

z.setErrorMap(
    makeZodI18nMap({
      // @ts-ignore
      t: t,
      ns: ["zod", "custom-zod"],
    }),
  );

and then when you're doing validation (like with rawCheck) you're passing the translation key and it uses the t function passed above, e.g.

function createSchema(
  intent: Intent | null,
  constraints?: {
    isEmailUnique?: (email: string) => Promise<boolean>;
  },
) {
  return z
    .object({
      email: EmailSchema.pipe(
        z.string().superRefine((val, ctx) => {

          return constraints.isEmailUnique(val).then((isUnique) => {
            if (!isUnique) {
              ctx.addIssue({
                code: "custom",
                params: {
                  i18n: "custom-zod:emailIsTaken",
                },
              });
            }
          });
        }),
      ),
    })
}

In this example it's not bound to any existing validator, which is really useful. I can just type namespace:translation-key in params.i18n key.

Is there a similar way in valibot now?

As of now, i call a global function like

setValibotTranslations(t);

which sets global messages for existing validators

export function setValibotTranslations(t: TFunction<["valibot"]>) {
  v.setSpecificMessage(v.bic, (issue) =>
    t("bic", { received: issue.received, ns: "valibot" }),
  );

  // Other translations...
}

@jansedlon
Copy link

jansedlon commented Jun 17, 2024

I'm wondering if there's a way around that. From the type definition of IssueInfo$1 which is the interface i can pass to addIssue in valibot, there's no type property. This could be used to differentiate things. Like if {type: 'custom', message: 'namespace:key'} i could catch that in the setValibotTranslations and parse the message as a translation key and use the t function to translate it 🤔

Just throwing ideas around

Example:

function setValibotTranslations(t: TFunction<['valibot']>) {
  // Or set a string type? ↓↓↓↓↓
  v.setSpecificMessage(v.custom, (issue) =>
    {
      const isTranslationKeyRegex = /[a-zA-Z0-9_-]+:[a-zA-Z0-9_-]+$/;

      if (isTranslationKeyRegex.test(issue.message)) {
        return t(issue.message, { received: issue.received, ns: "valibot", defaultValue: issue.message });
      }
      
      return t("custom", { received: issue.received, ns: "valibot" })},
  );
}

@fabian-hiller
Copy link
Owner

i could catch that in the setValibotTranslations and parse the message as a translation key

This will not work because the message of rawCheck is more specific. So setSpecificMessage is ignored in this case.

You could try to create a function that takes a code as the first argument and returns another function that matches our ErrorMessage type. This way you could pass this function as the last optional argument to any schema or action to translate the error message. You can try it out in this playground.

import * as v from 'valibot';

type TranslationCode =
  | 'email:invalid'
  | 'email:format'
  | 'password:invalid'
  | 'password:length';

const translations: Record<string, Record<TranslationCode, string>> = {
  en: {
    'email:invalid': 'The email has the wrong data type',
    'email:format': 'The email is badly formatted',
    'password:invalid': 'The password has the wrong data type',
    'password:length': 'The password is too short',
  },
  de: {
    'email:invalid': 'Die E-Mail hat den falschen Datentyp',
    'email:format': 'Die E-Mail ist falsch formatiert',
    'password:invalid': 'Das Passwort hat den falschen Datentyp',
    'password:length': 'Das Passwort ist zu kurz',
  },
};

function t(code: TranslationCode): v.ErrorMessage<v.BaseIssue<unknown>> {
  return (issue) => translations[issue.lang || 'en']?.[code] ?? issue.message;
}

const Schema = v.object({
  email: v.pipe(v.string(t('email:invalid')), v.email(t('email:format'))),
  password: v.pipe(
    v.string(t('password:invalid')),
    v.minLength(8, t('password:length')),
  ),
});

@ruiaraujo012
Copy link
Author

@fabian-hiller Just to let you know that I've migrated today all the codebase of a project I'm working on to version v0.32.0, and it went well with the help of rawCheck. There are some places where I will change the checkItems when it's ready, but I was able to do a workaround with rawCheck for now.

Thank you.

@fabian-hiller
Copy link
Owner

checkItems is already implemented but not released as I am also working on a partialCheck action and some other stuff. I expect to release it today or in the next few days.

@ruiaraujo012
Copy link
Author

Np, the important one was rawCheck, checkItems is a plus for this codebase :)

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

3 participants