fix: Unix timestamps are not serialized correctly for scalar DateTime#2141
fix: Unix timestamps are not serialized correctly for scalar DateTime#2141yamatsafi wants to merge 9 commits intographql-hive:masterfrom
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
| 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.
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
| 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?
| // Serializes Unix timestamp | ||
| [ | ||
| [854325678000, '1997-01-27T00:41:18.000Z'], | ||
| [876535000, '1970-01-11T03:28:55.000Z'], |
| // The maximum representable unix timestamp in seconds | ||
| [2147483647, '2038-01-19T03:14:07.000Z'], | ||
| // The minimum representable unit timestamp | ||
| [-2147483648000, '1901-12-13T20:45:52.000Z'], |
3 participants
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.