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

feat: add support for LocalDate and LocalTime scalars #533

Merged
merged 2 commits into from
Nov 19, 2020

Conversation

chrskrchr
Copy link
Contributor

@chrskrchr chrskrchr commented Oct 15, 2020

ref: #480, #531

This PR adds support for two new scalars -LocalDate and LocalTime - that represent dates and times with no associated timezone. They mimic the Java classes of the same name - LocalDate and LocalTime.

The scalar values are intentionally NOT serialized into JS Date objects on the backend as a signal to resolvers that they must apply thoughtful handling to the value so as not to accidentally apply any TZ offset to the value like what might happen when serializing and deserializing from a JS Date object.

One potential point of contention is that 24:00 is treated as a valid time in the LocalTime scalar. My project needs to use the LocalTime scalar to represent the exclusive end time of a time block, and so we need to support 24:00 as a signal that a block ends on midnight at the end of a day. For what it's worth, the ISO 8601 spec also allowed 24:00 as a valid time for the same reason, at least up until a 2019 revision of the spec:

https://en.wikipedia.org/wiki/ISO_8601#Times

Also worth noting is that this PR adds a dependency on the luxon library for validating that the provided dates and times are actually valid dates and times, e.g., not things like 25:99:99 or 2020-13-45. This resulted in a size increase of the final build and a need to raise the maxSize of the final bundle to something larger than 90kb. I didn't think this was a huge deal seeing that the project is only used in backend GraphQL server implementations AFAIK.

bundle-test/package.json Outdated Show resolved Hide resolved
tests/LocalTime.test.ts Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
… and disallow that value in the `LocalTime` scalar
@ardatan
Copy link
Collaborator

ardatan commented Nov 19, 2020

Thanks @chrskrchr !

@ardatan ardatan merged commit 31a0c70 into Urigo:master Nov 19, 2020
@chrskrchr chrskrchr deleted the ck.localDateTime branch November 21, 2020 15:06
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.

None yet

3 participants