-
Notifications
You must be signed in to change notification settings - Fork 50
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
Strip time component of parsed Date #107
Strip time component of parsed Date #107
Conversation
Codecov Report
@@ 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.
|
If I'm reading this correctly, won't this still deserialize dates as a We ran into this issue, too, and we solved it by wrapping the return values in custom 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
|
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 |
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. |
Fixes #106.
This PR strips the time component from a date string when being parsed as a
GraphQLDate
to allow services both usingGraphQLDate
fields to delegate to each other.