ceramicstudio / js-composedb

ComposeDB is a decentralized GraphQL database built on Ceramic Network
https://composedb.js.org
Other
91 stars 35 forks source link

Ensure DateTime scalar inputs are converted to string #53

Closed PaulLeCam closed 1 year ago

PaulLeCam commented 2 years ago

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.

morozj01 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

https://github.com/ceramicstudio/js-composedb/pull/54 This could be a temporary solution if you think it's not worth waiting for them to merge that PR @stbrody @PaulLeCam

PaulLeCam commented 2 years ago

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

ashutosh887 commented 1 year ago

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

PaulLeCam commented 1 year ago

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

ashutosh887 commented 1 year ago

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

PaulLeCam commented 1 year ago

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 commented 1 year ago

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