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

Bugs in Date scalar #826

Closed
alfredchanfq opened this issue Apr 16, 2021 · 2 comments
Closed

Bugs in Date scalar #826

alfredchanfq opened this issue Apr 16, 2021 · 2 comments

Comments

@alfredchanfq
Copy link

alfredchanfq commented Apr 16, 2021

Hi. I'm been trying to use the Date scalar, and I found 2 problems related to it.

Problem 1:

The Date scalar accepts value like '2021-07-32', but then when it's turned to new Date(), the Date constructor returns 'Invalid Date' which doesn't throw exceptions. The Graphql layer should not let such values get pass its validators.

The problem is that https://github.com/Urigo/graphql-scalars/blob/master/src/scalars/iso-date/validator.ts doesn't check for months that have 31 days.

Problem (?) 2:
new Date("2020-01-01") returns a Date with 0 hour offset, assuming to be UTC. While this works for production servers that use UTC, it breaks date manipulation in date-fns, which uses local time zone, when I'm running the server locally for development. This one might not be a real problem but my choice of library. However, imagine the following:

let dateFns = require("date-fns")
let a = new Date("2020-06-31") // 2020-07-01T00:00:00.000Z
dateFns.startOfDay(a) // 2020-06-30T04:00:00.000Z because my time zone is GMT-4

I don't think this is what you'd want either. One possible fix is, since you already have the year, month, day parts, you can return new Date(year, month - 1, day) instead of new Date(date /* string */).

ardatan added a commit that referenced this issue Apr 16, 2021
@ardatan
Copy link
Collaborator

ardatan commented Apr 16, 2021

For the first problem you reported, I tried to reproduce it but 2021-07-32 fails as expected;
See https://github.com/Urigo/graphql-scalars/pull/827/files

For the second one, you can try LocalDate if you work with timezones.

If I'm missing something, I'd appreciate if you create failing tests for those cases.

@alfredchanfq
Copy link
Author

alfredchanfq commented Apr 16, 2021

Hi @ardatan, for the first problem. I did encounter "Invalid Date" in my backend (the answer passed through the Graphql layer raw, without any date manipulation) a while ago. Because I didn't realize there's LocalDate and I need to work with time zones, so I panicked and rolled out my own scalar similar to LocalDate with date-fns, for a fix quick. So for the first problem, I didn't use the latest version of Date from this library for a while, so it is probably(?) invalid now, and I don't remember the specific details about this issue, except a scenario where:

  • somehow yyyy-MM-32 would "roll" over to the next month in new Date(), so it's not an "Invalid Date," so the scalar would "let go" of it.
  • then I use date-fns to parse the date, which outright rejects the day of 32, giving you an "Invalid Date".

For the second "problem", thanks for pointing me out about LocalDate. I will replace my own custom scalar with it when I have time.

Aside, another scalar that I roll out is the ObjectId (after reading #429), since my project already includes mongodb as a dependency -- it would be nice if this libary's ObjectId would wrap the string into a mongodb.ObjectId.

ardatan added a commit that referenced this issue Apr 21, 2021
ardatan added a commit that referenced this issue Apr 24, 2021
* Add tests for #826

* Support lower case date time strings #469

* Update Node on CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants