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)

[falkenhawk]

Description

fix: restore serialization of DateTime scalar to 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.

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

• Bug fix (non-breaking change which fixes an issue)
  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:

• I have followed the CONTRIBUTING doc and the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests and linter rules pass locally with my changes
• Any dependent changes have been merged and published in downstream modules

Further comments

I tried to migrate from https://github.com/excitement-engineer/graphql-iso-date to this package (as recommended in their README)

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 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.

[vercel]

@falkenhawk is attempting to deploy a commit to the The Guild Team on Vercel.

A member of the Team first needs to authorize it.

[falkenhawk]

I initially restored handling of Unix timestamps in #48402fe , as it was expected with graphql-iso-date and still as described in docs of graphql-scalars https://www.graphql-scalars.dev/docs/scalars/date-time

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.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
graphql-scalars	❌ Failed (Inspect)		Jul 6, 2022 at 3:55PM (UTC)

[martinblostein]

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.

[wzrdtales]

what is blocking here?

[falkenhawk]

@Urigo @ardatan I've rebased the branch and fixed all conflicts. (actually had to manually re-apply the changes over again which required significant effort, which would not be necessary if this PR had a chance to be addressed before i.a. #1764)

[wzrdtales]

bug bug @Urigo @ardatan

[kleinsch]

@ardatan @Urigo How can we get this merged? This is a breaking change and violates the contract of this package: it's supposed to serialize a Date to a string using a consistent format, not serialize to a Date and rely on toString in the underlying platform to do it.

[ardatan]

Thank you for the PR!
But we decided to introduce a new scalar that is based on string; LocalDateTime.
You can use it instead of DateTime if you want to deal with strings.

[ardatan]

I see you downvoted my comment. Could you explain what is the problem with the new scalar? It doesn't use JavaScript Date and it keeps DateTime as string like you expected?

[kleinsch]

@ardatan Nobody wants to deal with strings, but Date isn't a GraphQL type so we have to - https://graphql.org/graphql-js/basic-types/ . The purpose of this package is to convert JS types to GraphQL types. Date isn't a GraphQL type, so string serialization is necessary.

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 toString behavior that's nondeterministic. You can add a new type, but that doesn't address the issue that you've punted core types like Date into nondeterministic behavior.

[ardatan]

Ok we introduced DateTimeISO scalar that serializes Date to string.