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

Validate dates in .datetime() #2825

Closed
wants to merge 5 commits into from

Conversation

samchungy
Copy link
Contributor

@samchungy samchungy commented Oct 1, 2023

Resolves #2357

I think this change makes sense. If your JS system using Zod is going to process a datetime string, it will likely use new Date() to process it at some point in the system, just maybe not right now. So we should validate that new Date() works on it to get rid of things like invalid months, invalid days, invalid seconds etc.

However, this change is technically a small breaking change as #1797 (comment) introduced a date time format which new Date() does not accept which seems like a strange addition. I would suggest that users using that format should use a custom regex to validate instead as I will assume the majority of users would not be using that format.

@netlify
Copy link

netlify bot commented Oct 1, 2023

Deploy Preview for guileless-rolypoly-866f8a ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4d0ba08
🔍 Latest deploy log https://app.netlify.com/sites/guileless-rolypoly-866f8a/deploys/6620aaef88c76200085b08a1
😎 Deploy Preview https://deploy-preview-2825--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 configuration.

@samchungy samchungy marked this pull request as ready for review October 1, 2023 09:51
@@ -435,7 +440,6 @@ test("datetime parsing", () => {
datetimeOffset.parse("2020-10-14T17:42:29+00:00");
datetimeOffset.parse("2020-10-14T17:42:29+03:15");
datetimeOffset.parse("2020-10-14T17:42:29+0315");
datetimeOffset.parse("2020-10-14T17:42:29+03");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't parseable by new Date().

@samchungy samchungy changed the title Validate datetime Validate dates in .datetime() Oct 1, 2023
@samchungy samchungy changed the title Validate dates in .datetime() Validate dates in .datetime() Oct 1, 2023
@peterjdrb
Copy link

peterjdrb commented Jan 16, 2024

I've been discussing using Zod at work and whilst one of my collegues was doing a proof of concept, he noticed is was accepting dates such as the 99th of June. Looked at the code and realised it was just doing some regex. Whilst this was good start as it checks the format, it doesn't cover a number of scenarios including the one mentioned above.

@samchungy - your work is still an improvement but there are some cases that you are missing. For example it will accept a the 29th of Febuary 2021 even though this is not a valid year. Also I don't think that the day will validate for the correct months. I.e. if you put the 31st September then the validaiton will not fail when it should

Co-authored-by: Peter Robertson <peterjdrb@outlook.com>
@samchungy
Copy link
Contributor Author

I've been discussing using Zod at work and whilst one of my collegues was doing a proof of concept, he noticed is was accepting dates such as the 99th of June. Looked at the code and realised it was just doing some regex. Whilst this was good start as it checks the format, it doesn't cover a number of scenarios including the one mentioned above.

@samchungy - your work is still an improvement but there are some cases that you are missing. For example it will accept a the 29th of Febuary 2021 even though this is not a valid year. Also I don't think that the day will validate for the correct months. I.e. if you put the 31st September then the validaiton will not fail when it should

I have already added some tests and written some code on your branch @samchungy. I will push these changes up for you to review

Hey mate, I went a slightly different approach but I stole some of your test cases

@colinhacks
Copy link
Owner

colinhacks commented Apr 17, 2024

Okay, I've rolled back that PR you mentioned @samchungy. Good catch. All successful results should definitely be parsable by new Date().

As for this particular PR, it's much slower in Node.js than the regex-based approach. I added a benchmark in 35f0a38 that can be run with yarn benchmark --datetime.

$ yarn benchmark --datetime
yarn run v1.22.19
warning ../../../package.json: No license field
 $ tsx src/benchmarks/index.ts --datetime
datetime: new Date() x 7,195,365 ops/sec ±1.23% (96 runs sampled)
datetime: regex validation x 29,761,251 ops/sec ±0.42% (96 runs sampled)
datetime: simple regex with validation x 6,113,522 ops/sec ±0.66% (97 runs sampled)

Given that I'm inclined to stick with regex. But Sam if you see any glaring issues with that benchmark let me know 👍

@samchungy
Copy link
Contributor Author

Okay, I've rolled back that PR you mentioned @samchungy. Good catch. All successful results should definitely be parsable by new Date().

As for this particular PR, it's much slower in Node.js than the regex-based approach. I added a benchmark in 35f0a38 that can be run with yarn benchmark --datetime.

$ yarn benchmark --datetime
yarn run v1.22.19
warning ../../../package.json: No license field
 $ tsx src/benchmarks/index.ts --datetime
datetime: new Date() x 7,195,365 ops/sec ±1.23% (96 runs sampled)
datetime: regex validation x 29,761,251 ops/sec ±0.42% (96 runs sampled)
datetime: simple regex with validation x 6,113,522 ops/sec ±0.66% (97 runs sampled)

Given that I'm inclined to stick with regex. But Sam if you see any glaring issues with that benchmark let me know 👍

Oooo yeah that is a significant degradation. Let me cook something up and see if I can make it better

@samchungy
Copy link
Contributor Author

Latest master covers all of my new test cases so I'm going to close this.

@samchungy samchungy closed this Apr 18, 2024
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.

.datetime() validation
3 participants