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(gatsby): add support for microsecond and alternative iso datetime formats #12533

Merged
merged 6 commits into from
Mar 20, 2019
Merged

feat(gatsby): add support for microsecond and alternative iso datetime formats #12533

merged 6 commits into from
Mar 20, 2019

Conversation

himynameistimli
Copy link
Contributor

@himynameistimli himynameistimli commented Mar 13, 2019

I've pulled this PR against this branch per @freiksenet's request

Original Description

When attempting to use formatString to format dates in GraphQL, we get "Invalid date", which I believe is due to the precision format of seconds.

From what I've seen in other issues/PRs, it seems to be related to the list of formats in type-date.js file. It seems to only have ISO formats with millisecond precision (YYYY-MM-DDTHHmmss.SSS). We're using a python backend api for page generation and their default precision is in microseconds (YYYY-MM-DDTHHmmss.SSSSSS).

I've gone and added additional support for YYYY-MM-DDTHHmmss.SSSSSS and YYYY-MM-DDTHHmmss.SSSSSSZ as well as support for YYYY-MM-DD HHmmss.SSSSSS and YYYY-MM-DD HHmmss.SSSSSSZ

Notes

Per @sidharthachatterjee I tried to add some tests. Please forgive, I'm not so familiar with tests in JS, so I tried to follow from elsewhere to put together the tests. If anyone has any advice or suggestions on how I can improve them, please let me know.

One issue that I've found is after adding the support for microsecond, not only do microseconds validate, but nanoseconds (YYYY-MM-DDTHHmmss.SSSSSSSSS) and any precision validates (e.g. 2018-08-31T23:25:16.0123456789999+02:00) as well. I'm not sure how to interpret it, but I've just skipped tests that pass when in theory should fail. I've also commented out explicit nanosecond precision support until the tests can pass.

Related Issues

Previous PR #12511
Previous related issue #4813
Should also address #11520 @miktirr plz comment

@wardpeet wardpeet added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Mar 13, 2019
@wardpeet
Copy link
Contributor

@himynameistimli thanks so much for recreating this PR on our graphql refactoring branch! We'll have a look shortly.

@DSchau DSchau changed the title Add support for microsecond and alternative iso datetime formats feat(gatsby): add support for microsecond and alternative iso datetime formats Mar 19, 2019
@himynameistimli himynameistimli requested review from a team March 20, 2019 09:29
@himynameistimli himynameistimli requested a review from a team as a code owner March 20, 2019 09:29
@freiksenet freiksenet changed the base branch from schema-refactor-new to master March 20, 2019 09:30
@freiksenet
Copy link
Contributor

I've rebased in against master and made this target it, because we merged schema refactoring. I'll merge as soon as this passes CI.

Thanks a lot!

@freiksenet freiksenet merged commit dfe93ee into gatsbyjs:master Mar 20, 2019
@gatsbot
Copy link

gatsbot bot commented Mar 20, 2019

Holy buckets, @himynameistimli — we just merged your PR to Gatsby! 💪💜

Gatsby is built by awesome people like you. Let us say “thanks” in two ways:

  1. We’d like to send you some Gatsby swag. As a token of our appreciation, you can go to the Gatsby Swag Store and log in with your GitHub account to get a coupon code good for one free piece of swag. (Currently we’ve got a couple t-shirts available, plus some socks that are really razzing our berries right now.)
  2. We just invited you to join the Gatsby organization on GitHub. This will add you to our team of maintainers. Accept the invite by visiting https://github.com/orgs/gatsbyjs/invitation. By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.

If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants