zod icon indicating copy to clipboard operation
zod copied to clipboard

Extended issue data to include type and description

Open WazzaJB opened this issue 2 years ago • 12 comments

In line with issue #1208 I required visibility of validator type and description within my error maps to create user friendly errors, this PR adds that support.

I have not yet added documentation or tests but will do so providing I am on the right lines and this is something we want to move forward with. Thanks for Zod!

WazzaJB avatar Jul 04 '22 13:07 WazzaJB

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
Latest commit 7aedcd151fc074eb92d74c8960b0b40858a77bb7
Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/62c2f1fbeaa2d80009af608f
Deploy Preview https://deploy-preview-1241--guileless-rolypoly-866f8a.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jul 04 '22 13:07 netlify[bot]

I believe type already is included in most issues? It's just sometimes it's called something else, like for the invalid_type issue where it's called expected instead.

Svish avatar Jul 11 '22 10:07 Svish

Hey @Svish 👋

I believe it is on certain issues, but I think only ever equal to "array" | "string" | "number" | "set" as opposed to the full Zod type at https://github.com/colinhacks/zod/pull/1241/files#diff-b89cb81c2ff2ce8f5647cfb7d9fcd505bd7271abebfb8de24230a0ebb022f1daR38

I could be mistaken however, and there is likely a better approach to this.

WazzaJB avatar Jul 11 '22 10:07 WazzaJB

Yeah, I'm not sure what would be best here. I don't even require the type myself really, I just want a way to label stuff.

For my issue #1208, it feels like the cleanest solution would be to have all schemas add a label option, in addition to description, and then have both of those be available to the error map and possible to get from a schema. That would cover both being able to include the label in error messages, and adding labels (<label> or labelled-by) and descriptions (described-by attribute) to stuff based upon a schema.

Svish avatar Jul 11 '22 10:07 Svish

To be honest, the existing type would probably be enough to cover my needs too. However, it means you have to conditionally check the issue code in order to satisfy TS that type exists in line with the union. This PR goes some way to solve that by making type optional on the base issue type.

As an example, the "too_small" and "too_big" messages do not differ based on type (I believe) which can lead to you having a string which is too short throwing a message like "String is too small" as opposed to "String is too short".

My gist shows how you could create a locale definition similar to Yup and differ your messages based on type:

https://gist.github.com/WazzaJB/2eca3df3369c91317edaad3ed96862a6#file-playground-ts-L4

In place of label however, I would be in favour of adding some kind of meta to schemas. I know this conversation has been had before and it was deemed out of scope I believe.

WazzaJB avatar Jul 11 '22 10:07 WazzaJB

I already check for type within each code, so that's not an issue for me. My current error map looks like this, and I find it fairly clean actually:

setErrorMap((issue, ctx) => ({ message: zodErrorMap(issue, ctx) }));

function zodErrorMap(...[issue, ctx]: Parameters<ZodErrorMap>): string {
  /* eslint-disable @typescript-eslint/switch-exhaustiveness-check */
  switch (issue.code) {
    case ZodIssueCode.invalid_type:
      switch (issue.expected) {
        case 'number':
          return 'må være et gyldig tall';

        case 'integer':
          return 'kan ikke ha desimaler';

        case 'date':
          return 'må være en gyldig dato';

        case 'array':
          switch (issue.received) {
            case 'null':
            case 'undefined':
              return 'må fylles ut';
            default:
              return 'må være en liste';
          }

        case 'object':
          switch (issue.received) {
            case 'null':
            case 'undefined':
              return 'må fylles ut';
            default:
              return 'må være et objekt';
          }

        case 'string':
          return 'må fylles ut';

        case 'boolean':
          switch (issue.received) {
            case 'null':
            case 'undefined':
              return 'må fylles ut';
            default:
              return 'må være en boolsk verdi';
          }
      }

      // TODO: Replace this mess with `case 'enum':` when possible [torber]
      // @see https://github.com/colinhacks/zod/issues/1217
      if (/^(?:'(\w+)'(?: \| )?)+$/.test(issue.expected)) {
        return 'må være valgt';
      }

      break;

    case ZodIssueCode.too_small:
      switch (issue.type) {
        case 'number':
          return [
            'må være større enn',
            issue.inclusive ? 'eller lik' : '',
            issue.minimum,
          ].join(' ');

        case 'string':
          return issue.minimum === 1 && issue.inclusive
            ? 'må fylles ut'
            : [
                'må være',
                issue.inclusive ? 'minst' : 'lengre enn',
                issue.minimum,
                'tegn',
              ].join(' ');

        case 'array':
          return issue.minimum === 1 && issue.inclusive
            ? 'må fylles ut'
            : [
                'må ha',
                issue.inclusive ? 'minst' : 'flere enn',
                issue.minimum,
              ].join(' ');
      }
      break;

    case ZodIssueCode.too_big:
      switch (issue.type) {
        case 'number':
          return [
            'må være mindre enn',
            issue.inclusive ? 'eller lik' : '',
            issue.maximum,
          ].join(' ');

        case 'string':
          return issue.inclusive
            ? `kan ikke være mer enn ${issue.maximum} tegn`
            : `må være kortere enn ${issue.maximum} tegn`;
      }
      break;

    case ZodIssueCode.invalid_literal:
      return `må være "${String(issue.expected)}"`;

    // ... continued
  }

  // Fallback
  console.warn(`Missing error translation in ${zodErrorMap.name} function`, {
    issue,
    ctx,
  });
  return ctx.defaultError;
}

Svish avatar Jul 11 '22 11:07 Svish

Hi folks, interesting discussion here.

I seems like adding a label property and passing it into error maps is an unnecessary layer of indirection. Same for description, which I only grudgingly added to support various projects that were trying to do code-generation from Zod schemas.

Error messages can be customized in this way with existing parameters:

z.string({
  required_error: "First name is required",
  invalid_type_error: "First name is not valid"
});

You can build your own abstractions on top of Zod if you find this too verbose.

const labeledString = (label: string) => z.string({
  required_error: `${label} is required`,
  invalid_type_error: `${label} is not valid`
})

I'll leave this open for more discussion for a while but currently I'm not convinced this presents a better alternative to the current APIs. Zod represents types, not forms, so introducing concepts of "labels" and the like strikes me as too domain-specific.

colinhacks avatar Jul 18 '22 03:07 colinhacks

@colinhacks

I seems like adding a label property and passing it into error maps is an unnecessary layer of indirection. Same for description, which I only grudgingly added to support various projects that were trying to do code-generation from Zod schemas.

Error messages can be customized in this way with existing parameters:

z.string({
  required_error: "First name is required",
  invalid_type_error: "First name is not valid"
});

You can build your own abstractions on top of Zod if you find this too verbose.

const labeledString = (label: string) => z.string({
  required_error: `${label} is required`,
  invalid_type_error: `${label} is not valid`
})

I'll leave this open for more discussion for a while but currently I'm not convinced this presents a better alternative to the current APIs. Zod represents types, not forms, so introducing concepts of "labels" and the like strikes me as too domain-specific.

Although I understand the wish to keep Zod focused, this basically removes us being able to use the error_map, or at least much more awkward.

Error messages can be customized with existing parameters, but that would require us to customize all error messages, for every rule, everywhere. Especially with teams, it's difficult to remember doing this everywhere, and also to be consistent everywhere. Not to mention the mess it would be if we needed to reword one of those later.

Building abstractions on top of Zod would also be quite a mess, in my opinion, especially since you'd also need to override all the error messages for other rules, like min and max. Not even sure how we'd do that, without things becoming super messy.

Zod is such a nice library, and the error_map has been very nice and clean for creating a easy to maintain, single source, of translated error messages. The only thing it misses, is the ability to include a label in the error message, so that the error messages ca be more readable and clear to users, and accessibility compliant.

Really hope you can reconsider this issue. A label (or description, or whatever you want to call it) that can be attached to a rule, and be available via a getter and to the error map, would make Zod basically perfect.

Svish avatar Jul 18 '22 12:07 Svish

@Svish has summed up my thoughts better than I could have too.

I think keeping Zod focussed is definitely sensible, but as it stands it's not really flexible enough (I don't think) to set nice global error rules. The ecosystem around Zod is super impressive and I feel that some flexibility here would create lots of opportunity.

What if we took a step back to something like a meta property which was available in the error mapper? It would provide tonnes of flexibility and not carry any opinions or preferential use? Having this meta interface overridable in some way would mean Zod dependants could enforce typings on some meta keys?

With this we wouldn't be restricted to just a label/description and this might mean there is less maintenance overhead? Ie use meta for whatever you want and it's your job as a consumer not to break it?

Apologies if I am not conveying my thoughts well or they are too far from Zod's mission.

I'm going to have a go at wrapping Zod as you have suggested and see if I can arrive at a similar result regardless.

WazzaJB avatar Jul 18 '22 12:07 WazzaJB

@colinhacks would it be fair to me to say that based on the signature of "RawCreateParams" I can only set the invalid type and required error strings? In order to replace invalid_regex or min/max errors for example with labels, I would be unable to?

Or rather, I would need to set the string with the label on every single validation method? I'm struggling to see how I can achieve the same result as my previous gist without essentially overriding or providing alternative methods (with label arguments) for absolutely everything.

WazzaJB avatar Jul 19 '22 10:07 WazzaJB

Well, I attempted to think about this a bit differently but feel I failed. https://gist.github.com/WazzaJB/5700b07c6bcf0c5847a2a50051f68310

Use "label" instead of Zod and set the error map within there, doesn't work though as just overrides the labels. Needs to be set on a per method basis still.

WazzaJB avatar Jul 19 '22 11:07 WazzaJB

@colinhacks is code generation an important use case for Zod? We're doing a lot of the same stuff and thinking of storing JSON.stringified string inside description field. 🙈

tonyxiao avatar Sep 07 '22 16:09 tonyxiao

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Nov 06 '22 17:11 stale[bot]