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

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

Closed
wants to merge 3 commits into from

Commits on Dec 9, 2022

  1. fix: restore DateTime to usable state

    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
    falkenhawk committed Dec 9, 2022
    Configuration menu
    Copy the full SHA
    9f31c95 View commit details
    Browse the repository at this point in the history
  2. fix: DateTime: timestamps are expected to be number of milliseconds

    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 committed Dec 9, 2022
    Configuration menu
    Copy the full SHA
    4bdc2f7 View commit details
    Browse the repository at this point in the history

Commits on Apr 26, 2023

  1. docs: include milliseconds to the example

    value of ISO string representation of ECMAScript timestamp
    
    Co-authored-by: Sumegh Ghutke <[email protected]>
    falkenhawk and sghutke-mdsol authored Apr 26, 2023
    Configuration menu
    Copy the full SHA
    796aae3 View commit details
    Browse the repository at this point in the history