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

DateTime doesn't serialize a timestamp #414

Closed
gmac opened this issue Jul 16, 2020 · 12 comments
Closed

DateTime doesn't serialize a timestamp #414

gmac opened this issue Jul 16, 2020 · 12 comments
Labels

Comments

@gmac
Copy link

gmac commented Jul 16, 2020

Using the DateTime scalar appears to serialize to a Date object, which subsequently prints into the GraphQL response using Date's default .toString() implementation. As a result:

My GraphQL response returns this: Fri, 16 Mar 2018 11:50:07 GMT
But I was expecting this: 2018-03-16 11:50:07.000Z

The relevant code is here – https://github.com/Urigo/graphql-scalars/blob/master/src/scalars/iso-date/DateTime.ts#L35-L49. The choice to always return a Date object from the serialize method appears to be intentional, though I can't figure why. Am I overlooking something, or is this a legit issue?

@ardatan
Copy link
Collaborator

ardatan commented Jul 16, 2020

@gmac Could you share a reproduction?

@ardatan ardatan added the bug label Jul 16, 2020
@gmac
Copy link
Author

gmac commented Jul 16, 2020

@ardatan – sure thing! This sums it up... for reference, the graphql-iso-date package correctly serializes timestamp strings.

Schema:

scalar DateTime

type Query {
  now: DateTime
}

Resolvers

import DateTimeResolver from 'graphql-scalars';

export default {
  DateTime: DateTimeResolver,
  Query: {
    now() {
      return new Date();
    }
  }
};

Query:

query {
  now
}

Result: the DateTime resolver is serializing a JavaScript Date object rather than a formatted timestamp. This date gets .toString() called on it while rendering the GraphQL response, giving a verbose default output.

{
  "data": {
    "now": "Thu Jul 16 2020 10:56:09 GMT-0400 (Eastern Daylight Time)"
  }
}

Expected: the DateTime resolver should serialize a formatted timestamp string.

{
  "data": {
    "now": "2020-07-16T14:56:09.000Z"
  }
}

@danielrearden
Copy link
Contributor

@gmac Thanks for providing an example. I can't reproduce this issue with the latest version of graphql-scalars. I know we changed the DateTime scalar to align with graphql-iso-date starting with version 1.1.6. Maybe you're using an older version?

Here's a sandbox that demonstrates the expected behavior: https://codesandbox.io/s/wizardly-beaver-ekf3s

@yvann
Copy link

yvann commented Jul 29, 2020

Another way to see this is : The DateTime scalar type cannot parse its serialized value.

@danielrearden
Copy link
Contributor

@yvann The scalar's value is serialized as an RFC 3339 date time (such as "2020-07-29T14:48:52.866Z") and it's able to parse such a value if it's submitted as input.

I'm not able to reproduce this issue with the latest version of the library (1.2.2). If you've updated to the latest version and still seeing an issue, it would be helpful to have a Code Sandbox or complete repo with a reproduction.

@yvann
Copy link

yvann commented Jul 29, 2020

@yvann The scalar's value is serialized as an RFC 3339 date time (such as "2020-07-29T14:48:52.866Z") and it's able to parse such a value if it's submitted as input.

It's not the behavior I see and not what the code says here https://github.com/Urigo/graphql-scalars/blob/master/src/scalars/iso-date/DateTime.ts#L37
When "serialize" is called, it returns a "Date" object.

By the way, thank you for your fast reply and for the whole library !

@ardatan
Copy link
Collaborator

ardatan commented Jul 29, 2020

It uses Date object internally so your server/transport implementation will serialize it via toJSON method of JS Date because you probably use JSON.stringify and the result will be a valid date time string format.

@yvann
Copy link

yvann commented Jul 30, 2020

It uses Date object internally so your server/transport implementation will serialize it via toJSON method of JS Date because you probably use JSON.stringify and the result will be a valid date time string format.

Yes, but still looks wrong to me that we can't "parse" the "serialized" value, it throws an Error. Maybe it's the "parser" that has to accept Date object ?

I see it like this:

parse(value: TExternal): TInternal;
serialize(value: TInternal): TExternal;

@ardatan
Copy link
Collaborator

ardatan commented Jul 30, 2020

@yvann You're right! This has been fixed, now parseValue accepts Date instance.
@gmac Feel free to open a new issue with more details because we couldn't reproduce the issue.

@ardatan ardatan closed this as completed Jul 30, 2020
@yvann
Copy link

yvann commented Jul 30, 2020

🙏 great, thank you for this release !

@dyst5422
Copy link

dyst5422 commented Jan 13, 2021

This is still an issue when used with introspection. See the code here https://github.com/graphql/graphql-js/blob/master/src/utilities/astFromValue.js#L106-L141 that drives introspection when using default values. The default values need to be serialized. Unfortunately, graphqljs doesn't call JSON.toString() and so this hits the throw clause at the end there.

Still not sure whether this needs to be fixed here to serialize to a string, or on graphqljs to call JSON.toString() if its an object.

I suspect the approach has the same vulnerability in others of the scalar types if you relying on JSON.toString() to actually due the final serialization

@dyst5422
Copy link

dyst5422 commented Sep 8, 2022

I find myself back here over a year later reading my own comment. This is still an issue and at this point, I've given up on being able to still have a default value for this scalar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment