graphql-iso-date icon indicating copy to clipboard operation
graphql-iso-date copied to clipboard

Strip time component of parsed Date

Open MarkPolivchuk opened this issue 6 years ago • 9 comments

Fixes #106.

This PR strips the time component from a date string when being parsed as a GraphQLDate to allow services both using GraphQLDate fields to delegate to each other.

MarkPolivchuk avatar Jun 17 '19 21:06 MarkPolivchuk

Codecov Report

Merging #107 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #107   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         146    148    +2     
  Branches       40     40           
=====================================
+ Hits          146    148    +2
Impacted Files Coverage Δ
src/date/index.js 100% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8979ce8...8218033. Read the comment docs.

codecov[bot] avatar Jun 17 '19 21:06 codecov[bot]

If I'm reading this correctly, won't this still deserialize dates as a Date object, which will then get stringified to full datetime strings?

We ran into this issue, too, and we solved it by wrapping the return values in custom Date class that looks like

class DateOnly extends Date {
  toISOString() {
    return super.toISOString().slice(0, 10);
  }
}

sloria avatar Aug 14 '19 19:08 sloria

They get stringified to full datetime strings but with a zero-ed time portion. This change makes the ingress more flexible to allow full datetime strings, but ignoring the time portion.

MarkPolivchuk avatar Aug 27 '19 23:08 MarkPolivchuk

This would be great to get merged in, as I'm having the same problem referenced in #106

tonycapone avatar Aug 31 '19 18:08 tonycapone

edit: This approach doesn't work. If you're looking for a solution, use @sloria 's example below.

I think @sloria is on to something. I ended up taking his code and creating a custom scalar that decorates the GraphQLDate class with his custom Date class:

import { GraphQLScalarType } from "graphql";
import { GraphQLDate } from "graphql-iso-date";

// Here we're overriding the parsing behavior in GraphQLDate to provide
// some better parsing of Dates
// see https://github.com/excitement-engineer/graphql-iso-date/issues/106
export default class GraphQLDateOnly extends GraphQLScalarType {
  constructor() {
    super({
      name: "Date",

      serialize(value) {
        return GraphQLDate.serialize(value);
      },

      parseValue(value) {
        const date = GraphQLDate.parseValue(value);
        return new DateOnly(date);
      },

      parseLiteral(ast) {
        const date = GraphQLDate.parseLiteral(ast);
        return new DateOnly(date);
      }
    });
  }
}

class DateOnly extends Date {
  toISOString() {
    console.log("Parsing date");
    return super.toISOString().slice(0, 10);
  }
}

tonycapone avatar Sep 05 '19 18:09 tonycapone

Yes, we did something similar:

/* @flow */

import { GraphQLScalarType } from 'graphql';
import { GraphQLDate as BaseGraphQLDate } from 'graphql-iso-date';

class DateOnly extends Date {
  toISOString() {
    return super.toISOString().slice(0, 10);
  }
}

const {
  parseValue: baseParseValue,
  parseLiteral: baseParseLiteral,
  ...config
} = BaseGraphQLDate.toConfig();

export default new GraphQLScalarType({
  ...config,
  parseValue: value => new DateOnly((baseParseValue(value): any)),
  parseLiteral: ast => new DateOnly((baseParseLiteral(ast): any)),
});

sloria avatar Sep 05 '19 18:09 sloria

Can we please merge this in? My team is encountering the same issue, and we are implementing our own version of this in our code as a workaround.

cdillon85 avatar Nov 08 '19 15:11 cdillon85

I'm still not sure this PR does the correct thing. I think using DateOnly that stringifies to a date (not datetime) is a more correct approach. Thoughts @MarkPolivchuk ?

sloria avatar Nov 08 '19 15:11 sloria

How would you go about converting 2017-01-07T00:00:00+01:20 (which is equal to 2017-01-06T22:40:00Z)? Should this be converted to 2017-01-07 or 2017-01-06? There is no clear definition as to what the actual date is given that it is dependent on the time offset. I want to avoid these kind of ambiguities in this library and keep in line with the RFC 3339 specification.

excitement-engineer avatar Nov 09 '19 07:11 excitement-engineer