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

ZodString.minLength defaults to null to be consistent with maxLength #1248

Conversation

Balastrong
Copy link
Contributor

@Balastrong Balastrong commented Jul 6, 2022

This PR fixes #1246.

minLength and maxLength are inconsistent in ZodString, this PR makes the behaviour more consistent by returning null if no max is defined (not changed) and if no min is defined (instead of -Infinity).

This PR also improves the syntax by removing the incorrect usage of Array.map that had side effects & no return).

A couple tests are also added to make sure it works as intended.

@netlify
Copy link

netlify bot commented Jul 6, 2022

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 960edc4
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/62c5c15d82e9c8000858c768
😎 Deploy Preview https://deploy-preview-1248--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.

@Balastrong Balastrong changed the title Feature/1246 improved min max string length ZodString.maxLength defaults to Infinity as number to be consistent with minLength Jul 6, 2022
@ghost
Copy link

ghost commented Jul 6, 2022

Looking at other types in the library, it seems to me that the most consistent API would be to return number | null for both min and max, rather than returning number. Some examples below:

// ZodNumber
    get minValue(): number | null;
    get maxValue(): number | null;
// ZodArrayDef
    minLength: {
        value: number;
        message?: string;
    } | null;
    maxLength: {
        value: number;
        message?: string;
    } | null;
// ZodSetDef
    minSize: {
        value: number;
        message?: string;
    } | null;
    maxSize: {
        value: number;
        message?: string;
    } | null;

Therefore, I would suggest this instead:

// ZodString
    get minLength(): number | null;
    get maxLength(): number | null;

@Balastrong
Copy link
Contributor Author

Hey @ealmansi-uc, I guess you're right. I have updated the PR to reflect the same logic used in ZodNumber and use number | null. Should look better now. Thank you!

@ghost
Copy link

ghost commented Jul 6, 2022

@Balastrong LGTM now, although I'm not a contributor to this repo 😅 you'll probably want to update the PR's title
keep in mind this is a breaking change, I'm not sure how conservative this project is around breaking changes

@Balastrong Balastrong changed the title ZodString.maxLength defaults to Infinity as number to be consistent with minLength ZodString.minLength defaults to null to be consistent with maxLength Jul 6, 2022
@colinhacks colinhacks merged commit d78047e into colinhacks:master Jul 17, 2022
@colinhacks
Copy link
Owner

Thanks, good PR!

@Balastrong Balastrong deleted the feature/1246-improved-min-max-string-length branch July 18, 2022 06:46
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.

Getter maxLength in ZodString has incorrect type (null)
2 participants