fix: Unix timestamps are not serialized correctly for scalar DateTime #2141
Conversation
The DateTime scalar (GraphQLDateTime) fails to convert any 10-digit unix timestamp, for example, 1695416400, produces a date like 1970-01-20T14:56:56.400Z. In javascript, if one tries to convert a UNIX timestamp into a human-readable date, one must do: const humanReadableDate = new Date(1695416400 * 1000).toISOString(); // result 2023-09-22T21:00:00.000Z
yamatsafi
changed the title
yamatsafi
changed the title
|
This is a breaking change and i am not sure if I want to sacrifica the support for the other timestamp format. |
Not sure how this is sacrificing support for other timestamp formats, but this needs to be addressed as it doesn't work for unix timestamps. Fixed broken tests. |
Arda, any further thoughts on this? |
|
@ardatan added support for both seconds and milliseconds. I would appreciate it if you could take a look, please. |
|
@ardatan We've just discovered this issue as well. Any chance of merging this? |
|
https://github.com/Urigo/graphql-scalars/pull/2141/files#diff-5bff20d592f8d56ae20cad088bf374d5ce38af414afd5631ab82f42481bb8473L90 |
src/scalars/iso-date/DateTime.ts
Outdated
| @@ -33,7 +33,14 @@ export const GraphQLDateTimeConfig: GraphQLScalarTypeConfig<Date, Date> = /*#__P | |||
| throw createGraphQLError(`DateTime cannot represent an invalid date-time-string ${value}.`); | |||
| } else if (typeof value === 'number') { | |||
| try { | |||
| return new Date(value); | |||
| // Find out if the value is in seconds or milliseconds | |||
| const dateInMilliseconds = Date.now(); | |||
There was a problem hiding this comment.
Instead of getting the current Date, we can check the number of digits with a fixed value like
value.toString().length === X
There was a problem hiding this comment.
@yamatsafi Do you have time to fix this? I could help otherwise.
There was a problem hiding this comment.
I'd appreciate if you can :)
There was a problem hiding this comment.
This is future-proof instead of checking for 13-digits today which may not be true in a few years :-) I know it is a long way but still. @jeremyzahner Please feel free to add unit tests as @ardatan suggested or I can add it quickly.
There was a problem hiding this comment.
made changes, ready for review
| @@ -26,6 +26,10 @@ const schema = new GraphQLSchema({ | |||
| type: GraphQLDateTime, | |||
| resolve: () => '2016-02-01T00:00:00-11:00', | |||
| }, | |||
| validUnixTimestamp: { | |||
There was a problem hiding this comment.
Does this have the same prop name with the one below?
| @@ -87,11 +87,16 @@ describe('GraphQLDateTime', () => { | |||
| // Serializes Unix timestamp | |||
| [ | |||
| [854325678000, '1997-01-27T00:41:18.000Z'], | |||
| [876535000, '1970-01-11T03:28:55.000Z'], | |||
| // The minimum representable unit timestamp | ||
| [-2147483648000, '1901-12-13T20:45:52.000Z'], |
Description
The DateTime scalar (GraphQLDateTime) fails to convert unix timestamp, for example, 1695416400, produces a date like 1970-01-20T14:56:56.400Z. Since the Unix timestamp represents the number of seconds since January 1st, 1970 at UTC, and the Javascript Date() function's [Time value or timestamp number] argument is in milliseconds, not seconds, the number of seconds needs to be multiplied by 1000.
In javascript, if one tries to convert a UNIX timestamp into an iso human-readable date string, one must do:
const humanReadableDate = new Date(1695416400 * 1000).toISOString();
// result 2023-09-22T21:00:00.000Z
Related # (issue)
#2139
Type of change
Screenshots/Sandbox (if appropriate/relevant):
How Has This Been Tested?
Test A
Test B
Changing this particular line in the library locally fixes our issue.
Test Environment:
Additional context
Checklist:
Further comments
This change is more in line with graphql-iso-date package.