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

Ensure DateTime scalar inputs are converted to string #53

Closed
PaulLeCam opened this issue Oct 28, 2022 · 10 comments
Closed

Ensure DateTime scalar inputs are converted to string #53

PaulLeCam opened this issue Oct 28, 2022 · 10 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@PaulLeCam
Copy link
Contributor

Description

The DateTime scalar from the graphql-scalars library converts inputs to JS Date objects rather than strings as expected.
There is a PR already open to fix the issue but we should address it directly here in the meantime.

Technical Information

The PR to fix the issue in graphql-scalars could a good solution to look into, implementing similar changes in our @composedb/graphql-scalars package.

@PaulLeCam PaulLeCam added bug Something isn't working good first issue Good for newcomers labels Oct 28, 2022
@morozj01
Copy link
Contributor

@PaulLeCam

It seems like @composedb/graphql-scalars is implemented as a simple pass through of the scalar types defined in the graphql-scalars package.

The PR you referenced is fixing an issue in the serialize function of the datetime scalar which that package is exporting

I could implement a custom definition for the datetime scalar type (to use temporarily in place of the one being exported by graphql-scalars) but that may be overkill for a package which is currently very simple - especially with a PR pending with a fix.

Happy to work on a temporary solution but wanted to get your feedback before proceeding.

@stbrody
Copy link
Contributor

stbrody commented Oct 31, 2022

Shouldn't dates get converted into number types, or ideally to native date objects in the underlying db, so that range queries can work effectively?

@stbrody
Copy link
Contributor

stbrody commented Oct 31, 2022

I guess the problem is that JSON doesn't have a native encoding for dates. Sigh, this was one of the reasons that MongoDB's BSON added extra types beyond the base supported in JSON. We might want to consider defining a JSON-compatible serialization format for dates so that Ceramic nodes can actually treat dates like dates

@morozj01
Copy link
Contributor

morozj01 commented Nov 7, 2022

Exactly - ceramic is importing a library which is supposed to serialize dates into strings already but there is a bug where it is not being done.

They have a PR open with a potential fix but it has been inactive for a while

#54
This could be a temporary solution if you think it's not worth waiting for them to merge that PR
@stbrody @PaulLeCam

@PaulLeCam
Copy link
Contributor Author

Thanks @morozj01, I left some comments on your PR.

@ashutosh887
Copy link

@PaulLeCam I would like to continue working on it if needed

@PaulLeCam
Copy link
Contributor Author

@ashutosh887 thanks but this has already been fixed in the mentioned PR, the latest version of ComposeDB packages have the fix.

@ashutosh887
Copy link

I'm willing to contribtute to the ecosystem @PaulLeCam
Please let me know if there are any other issues I could pick

@PaulLeCam
Copy link
Contributor Author

Thanks, there are some issues labelled as "good first issue" in the Ceramic repository: https://github.com/ceramicnetwork/js-ceramic/labels/good%20first%20issue

@ashutosh887
Copy link

Just saw... @PaulLeCam
They look quite old... I'll check if they're already resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants