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

fix: implement temporary schema definition to support datetime scalars #54

Merged
merged 4 commits into from
Nov 11, 2022

Conversation

morozj01
Copy link
Contributor

@morozj01 morozj01 commented Nov 7, 2022

Ensure DateTime scalar inputs are converted to string - #53

Description

This is meant to be a temporary solution to enable datetime scalar types in Ceramic models while this pull request is in review (as there has been no activity for some time.)

This change introduces a custom datetime scalar definition in a new file which is meant to be deleted after the solution is merged into the graphql-scalars package

This schema definition is essentially a simplified copy of the logic defined in the graphql-scalars package.

It purposely kept minimal and therefore only ISO strings in UTC time are supported - as per the documentation

Passing in Unix timestamp or JS date object will throw an error.

How Has This Been Tested?

Describe the tests that you ran to verify your changes. Provide instructions for reproduction.

Currently creating documents with a datetime scalar field will throw this error

With this change, I was able to upload datetime scalar values without issue

IDs:

  • kjzl6kcym7w8y5f7jspauwxecxeuzrri0rigwwl5h7x8i9ebhmh7hi2thxmksx0
  • kjzl6kcym7w8y60p5d46t282vvl4b8t4pkfk1364rlhbxtqhb71i2rtsckvqrvb
  • kjzl6kcym7w8y5u9ktpponjvo0jh0pjl54tc9tx0pl3ayupzxaamdb7vpyduc0y

Model: kjzl6hvfrbw6c65z7bpyak6mgz9maadbucil8sey0lkuy9ebiinbjl0vd9ygal4

Let me know if/what additional testing is needed

Definition of Done

Before submitting this PR, please make sure:

  • The work addresses the description and outcomes in the issue
  • I have added relevant tests for new or updated functionality
  • My code follows conventions, is well commented, and easy to understand
  • My code builds and tests pass without any errors or warnings
  • I have tagged the relevant reviewers
  • I have updated the READMEs of affected packages
  • I have made corresponding changes to the documentation
  • The changes have been communicated to interested parties

Copy link
Contributor

@PaulLeCam PaulLeCam left a comment

Choose a reason for hiding this comment

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

Thanks for your PR!
Looks good to me overall, just a few comments.

packages/graphql-scalars/src/datetime.ts Show resolved Hide resolved
packages/graphql-scalars/src/datetime.ts Outdated Show resolved Hide resolved
packages/graphql-scalars/src/datetime.ts Outdated Show resolved Hide resolved
packages/graphql-scalars/src/datetime.ts Outdated Show resolved Hide resolved
@morozj01 morozj01 requested a review from PaulLeCam November 8, 2022 00:58
PaulLeCam
PaulLeCam previously approved these changes Nov 8, 2022
Copy link
Contributor

@PaulLeCam PaulLeCam left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@PaulLeCam
Copy link
Contributor

@morozj01 could you update the scalar description to use the one from graphql-scalars so it matches the existing test snapshots please?

@morozj01
Copy link
Contributor Author

@morozj01 could you update the scalar description to use the one from graphql-scalars so it matches the existing test snapshots please?

For sure - just updated

Copy link
Contributor

@PaulLeCam PaulLeCam left a comment

Choose a reason for hiding this comment

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

Thanks!

@PaulLeCam PaulLeCam merged commit c34573b into ceramicstudio:main Nov 11, 2022
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.

2 participants