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

Fix issue #1611 #1620

Merged
merged 4 commits into from
Dec 12, 2022
Merged

Fix issue #1611 #1620

merged 4 commits into from
Dec 12, 2022

Conversation

john-schmitz
Copy link
Contributor

@john-schmitz john-schmitz commented Nov 30, 2022

closes #1611

I quickly drafted a working solution so we could iterate and discuss its design.

I also added the same proposed behavior to strings as well, since both strings and arrays share the .length function.

Looking further into the codebase, I think it would be ideal to create a new check (something like "exact_legth") and a ZodIssueCode so we can share the same locale behavior between Arrays and Strings.

Whats your take? How should I integrate those changes into Zod's current codebase?

@netlify
Copy link

netlify bot commented Nov 30, 2022

Deploy Preview for guileless-rolypoly-866f8a ready!

Name Link
🔨 Latest commit 36396bb
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/6396782a29a13b0008c7327e
😎 Deploy Preview https://deploy-preview-1620--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.

@maxArturo
Copy link
Contributor

Whats your take? How should I integrate those changes into Zod's current codebase?

IMO this seems very sensible - especially since there's no API changes and seems in line with the current way of organizing error messages. Just my $0.02 USD. Thanks for the pr @john-schmitz !

@john-schmitz
Copy link
Contributor Author

Glad to help. The impact Zod had on my workflow is immense.
Always good to give back.

@santosmarco-caribou
Copy link
Contributor

santosmarco-caribou commented Dec 10, 2022

Hmm... I'm not sure if I like this solution. (but correct me if I'm wrong)

You are setting the default message directly on the method, promoting it to the highest priority level when generating the final message.

Say, for example, I've decided to define my error map on the .parse() call:

const myArraySchema = z.array(z.string()).length(2).parse(["foo"], {
  errorMap: (issue) => {
    if (issue.code === "too_small") {
      return { message: "Hey yo, the array must have a length of 2." };
    }
    return { message: "Nope" };
  },
});

Since I'm not setting the "Hey yo, the array must have a length of 2." message in the .length(2) call, it will be overwritten by your default message because messages set directly on the check ('length') call have the highest priority (I'm not setting it there, but you are).

Default messages should always have the lowest priority.

Can you please add a test of the above code and see the result?

@colinhacks
Copy link
Owner

As @santosmarco-caribou says, the original approach does break Zod's error message hierarchy. I pushed a commit that uses a different approach, adding an optional exact flag (alongside inclusive) that we can use to customize the default error message here. Achieves the goal in a backwards compatible way.

@colinhacks colinhacks merged commit 9828837 into colinhacks:master Dec 12, 2022
dcroote added a commit to api3dao/airnode that referenced this pull request Dec 17, 2022
- recursive type definition
- exact flag for too_big and too_small:
colinhacks/zod#1620
- options property of ZodDiscriminatedUnion was renamed to optionsMap:
colinhacks/zod#1290 (comment)
dcroote added a commit to api3dao/airnode that referenced this pull request Dec 19, 2022
* Bump zod from 3.19.1 to 3.20.0

* Bump @api3/ois from 1.3.0 to 1.4.0

* fix: zod 3.20.0 issues

- recursive type definition
- exact flag for too_big and too_small:
colinhacks/zod#1620
- options property of ZodDiscriminatedUnion was renamed to optionsMap:
colinhacks/zod#1290 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create exact error message for z.array().length().
4 participants