fix: restore serialization of DateTime scalar to pre-1.2.0 (values must be serialized to string, not to instance of Date) + clarify that timestamps are ECMAScript (milliseconds), not unix timestamps (seconds) #1641
Conversation
|
@falkenhawk is attempting to deploy a commit to the The Guild Team on Vercel. A member of the Team first needs to authorize it. |
|
I initially restored handling of Unix timestamps in #48402fe , as it was expected with but then I noticed it was decided in #387 (comment) that timestamps with milliseconds are expected here to be used instead. (i.e. ECMAScript timestamps, https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#the_ecmascript_epoch_and_timestamps) I removed all references to "unix timestamps" in 95e86c0 to make it clear what is expected. |
falkenhawk
changed the title
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
I'll just +1 that this should be fixed. I ran into this as a breaking change while trying to migrate from graphql-iso-date. |
|
what is blocking here? |
pre-1.2.0
A value cannot be serialized into something other than js primitives (boolean string, number) or array, object, null
otherwise there is no guarantee for it to be transported properly over graphql protocol.
Here `DateTime` scalar was being serialized to an instance of `Date` - in contrary to `Date` or `Time` scalars, which have been left untouched in v1.2.0 and still serialize to string - and that broke the expected outputs, mangling data for fractional seconds and failing to comply with graphql spec which does not allow such values.
Moreover, `astFromValue('2021-10-02T00:00:00.000Z', DateTime)` (a function from `graphql/utilities`) fails on such "serialized" value (here: `Date`) with `TypeError: Cannot convert value to AST: 2021-10-02T00:00:00.000Z.` - that was the initial reason for me to submit the PR - as it broke our toolset.
The affecting change happened in Urigo@3b1352c#diff-5bff20d592f8d56ae20cad088bf374d5ce38af414afd5631ab82f42481bb8473
with no message explaining why, no linked PR, moreover there is no release notes for v1.2.0 (https://github.com/Urigo/graphql-scalars/releases/tag/v1.2.0) only v1.2.1 but there is nothing mentioning the change in any of future releases.
Additionally, fixed inconsistencies in handling unix timestamps (maybe that was initial motivation to go with those changes in v1.2.0?)
# Conflicts:
# src/scalars/iso-date/DateTime.ts
# tests/iso-date/DateTime.integration.test.ts
# tests/iso-date/DateTime.test.ts
# tests/iso-date/formatter.test.ts
graphql-iso-date operated on Unix timestamp values, but graphql-scalars operates on ECMAScript timestamps (number of milliseconds since January 1, 1970, UTC) as decided in Urigo#387 (comment) It has to be clear which is used. Certainly values are not Unix timestamps and all references must be removed. Docs are updated. ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date#the_ecmascript_epoch_and_timestamps
falkenhawk
force-pushed
the
fix/date-time
branch
from
95e86c0
to
4bdc2f7
Compare
value of ISO string representation of ECMAScript timestamp Co-authored-by: Sumegh Ghutke <sghutke@mdsol.com>
|
Thank you for the PR! |
|
I see you downvoted my comment. Could you explain what is the problem with the new scalar? It doesn't use JavaScript |
|
@ardatan Nobody wants to deal with strings, but The issue flagged here is that a breaking change in this API takes deterministic APIs that used to convert dates to strings have now been moved to rely on platform |
|
Ok we introduced |
Description
fix: restore serialization of
DateTimescalar to pre-1.2.0A value cannot be serialized into something other than js primitives (boolean string, number) or array, object, null
otherwise there is no guarantee for it to be transported properly over graphql protocol.
Additionally, fixed inconsistencies in handling
unix timestamps (maybe that was initial motivation to go with those changes in v1.2.0?)edit: ECMAScript timestamps, ref: #387 (comment) 95e86c0(possibly) Related #414 (comment)
Related discussion: #1617 (reply in thread)
another one: #835
Type of change
fixes String versions of date scalars #1275
fixes Date parsing fails #1770
fixes DateTime doesn't serialize a timestamp #414
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate
How Has This Been Tested?
Changes to automated test suite included.
Checklist:
Further comments
I tried to migrate from https://github.com/excitement-engineer/graphql-iso-date to this package (as recommended in their README)
Here
DateTimescalar was being serialized to an instance ofDate- in contrary toDateorTimescalars, which have been left untouched in v1.2.0 and still serialize to string - and that broke the expected outputs, mangling data for fractional seconds and failing to comply with graphql spec which does not allow such values.Moreover,
astFromValue('2021-10-02T00:00:00.000Z', DateTime)(a function fromgraphql/utilities) fails on such "serialized" value (here:Date) withTypeError: Cannot convert value to AST: 2021-10-02T00:00:00.000Z.- that was the initial reason for me to submit the PR - as it broke our toolset.The affecting change happened in 3b1352c#diff-5bff20d592f8d56ae20cad088bf374d5ce38af414afd5631ab82f42481bb8473
with no message explaining why, no linked PR, moreover there is no release notes for v1.2.0 (https://github.com/Urigo/graphql-scalars/releases/tag/v1.2.0) only v1.2.1 but there is nothing mentioning the change in any of future releases.