Codecov Report

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

@@          Coverage Diff          @@
##           master   #107   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         146    148    +2     
  Branches       40     40           
=====================================
+ Hits          146    148    +2

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.

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);
  }
}

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.

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

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);
  }
}

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)),
});

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.

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 ?

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.