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

[RFC] New Scalar for date-time #579

Closed
andimarek opened this issue Apr 24, 2019 · 28 comments
Closed

[RFC] New Scalar for date-time #579

andimarek opened this issue Apr 24, 2019 · 28 comments
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)

Comments

@andimarek
Copy link
Contributor

andimarek commented Apr 24, 2019

This is a new attempt to add a date/time scalar to GraphQL in order to represent date and time. There has been previous discussions, which resulted in the decision not to extend GraphQL (see #315 )

Why a standard scalar for date/time

I think a scalar handling date/time is the most fundamental scalar missing in GraphQL. The arguments for having a standard scalar (or multiple) are:

  • it is required by enough schemas to represent a date/time
  • it is not easy do deal with date/time correctly. Having a well defined standard scalar would make it easier for the GraphQL community and take away the burden of coming up with a solution again and again on their own.
  • because date/time handling is not trivial, different date/time scalars are likely to be incompatible across different GraphQL implementations if not specified

Proposal: DateTime

The proposal is to add one new scalar: DateTime.

An DateTime is a specific point in time. It is represented in input and output as yyyy-MM-dd'T'HH:mm:ss.SSSZ. (The ultimate authority for the format is RFC 3339). For example 2011-12-03T10:15:30.123+01:00 is a valid value.

It doesn't include full timezone information (for example "Europe/Berlin").

The goal here is be very precise in the naming and strict in the allowed formats: it is an explicit non-goal to support a wide range of different formats.

Date/Time as defined in RFC 3339

The specific details of the format are defined in RFC 3339. This proposal doesn't introduce or change any new concepts.

Format definition

The format is defined in RFC 3339 section 5.6 section 5.6 date-time.

Offset is not a full Time zone

This scalar only deals with offsets (hours and minutes differences to UTC) and not full time zones. Full time zones also include rules regarding daylight saving time. For example "Europe/Berlin" represents a full timezone, but +02:00 is an offset used by "Europe/Berlin" during the summer.

Comparison is not trivial

A comparison between different DateTime is not trivial because two DateTime can represent the same point in time: For example 2008-12-03T11:00+01:00
and 2008-12-03T12:00+02:00 are both equal in this regard, but the strings are not

Why not have more formats?

The goal here is to make it easier to communicate a specific point in time. Having one well defined (and widely supported) format is the best way to achieve that imho.

What about other scalars?

I wanted to keep this proposal as simple as possible and I think DateTime is a good start for supporting date/time better in GraphQL.

I think having a LocalDateTime or LocalDate or LocalTime are worth having a discussion, but at the same time they can be represented as an DateTime in some way and I didn't want to make this proposal more complex.

Why not restrict to just UTC?

This is a possible alternative. (In this case the format would be yyyy-MM-dd'T'HH:mm:ss.SSS'Z' with a const Z at the end). I would prefer calling this scalar not DateTime anymore, but Instant.

By restricting it to just UTC it would be somehow simpler, but we would also loose flexibility.

With DateTime one can easily imagine a type like this:

type ZoneDateTime {
 dateTime: DateTime
 zone: String
}

This would allow for a point in a Time in a specific zone. (for example a recurring calendar entry).
Using an Instant in this way is not really possible, because it would always be formatted in UTC.
I see Instant as a possible additional scalar which could be introduced later (if needed).

DateTime or OffsetDateTime or ....

The first version of this RFC named the scalar OffsetDateTime. It was then renamed to DateTime to reflect that this is the name most people use for a date-time scalar.

@andimarek andimarek changed the title [RFC] OffsetDateTime [RFC] New Scalar OffsetDateTime Apr 24, 2019
@IvanGoncharov IvanGoncharov added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Apr 24, 2019
@jdehaan
Copy link

jdehaan commented Apr 25, 2019

IMHO the extension to use time zones has to be absolutely avoided. Timezone information is not constant and has to be resolved inside a context, that makes things not more complicated but practically impossible to handle...

As far as I am concerned I would greatly appreciate a specified format (even simplified to a pure UTC only) in other to make some more things working nicely across "vendors" without tedious conversions. NodaTime/JodaTime have the concept of "Instant" that is what is useful and good to take (which is the similar to this RFC = amount of seconds/ms since an epoch or its equivalent ISO representation).

One questionable point is if we should allow that to be a "sum" type: have an alternative string representation and (possibly large) integer representation at the same time (Union type in GraphQL terms but a more elegant one without an explicit type resolver, as it can be deduced from the format).

Here some Blog references about this topic:

This shows some pitfalls Lee Byron also pointed out in #315. The semantic around the type should be very clearly documented, so people choosing to use that type know what it is intended for and not intended for.

The type having Date in its name still has something not universal enough. The Gregorian calendar is a very common system across the world but by no mean absolutely unique... So this type would be a bit opinionated, providing support for extensions would probably be a good thing.

@mathstuf
Copy link

This doesn't use time zones (like America/New_York), but offsets. Offsets don't have DST problems or anything like that. Even Git stores its timestamps with an offset if you want some precedence.

I'd also be fine with forcing everything to convert to UTC for a DateTime scalar, but that may end up with some loss of information and require a separate field anyways.

@andimarek
Copy link
Contributor Author

I updated the proposal based on the feedback so far.

@zzz6519003
Copy link

zzz6519003 commented May 3, 2019

🍺 what about the complexity it may bring

besides, time zone itself may not be a universal good standard for big(wide) country like China(we only use Beijing Time) IMHO

@andimarek
Copy link
Contributor Author

@zzz6519003 @jdehaan please have a look at the explanation above about "Offset is not a full Time zone"

@andimarek
Copy link
Contributor Author

I updated the proposal to make the relationship to RFC 3339 clear.

@andimarek andimarek changed the title [RFC] New Scalar OffsetDateTime [RFC] New Scalar for Date/Time (OffsetDateTime) Jun 26, 2019
@andimarek andimarek changed the title [RFC] New Scalar for Date/Time (OffsetDateTime) [RFC] New Scalar for date-time Jul 3, 2019
@IvanGoncharov
Copy link
Member

Just as a follow-up to last WG: I like Lee's idea of referencing URL of spec. I would propose to call introspection field describedBy and have matching directive(similar to @deprecate and deprecationReason):

scalar DateTime @descibedBy(url: "https://www.ietf.org/rfc/rfc3339.txt")

I also would suggest allowing this directive on any type not only scalar that way we would be able to use it for complex specs (e.g. Relay connection spec or Realy global ID spec)

interface Node @describedBy(url: "https://facebook.github.io/relay/graphql/objectidentification.htm") {
  id: ID
}

@nodkz
Copy link

nodkz commented Jul 23, 2019

Just read last WG notes:

Andi: What happens if you already have a DateTime scalar? Adding a standard DateTime scalar means a breaking change for some existing clients.
...
Lee: We can look at individual entries, so we can filter it down just to people who call their type DateTime and then check on how the type is formatted.
...
Andi: In order to keep it non-breaking we would have to add a way to deal with the DateTime scalar without breaking changes. Exchanges different proposals sounds like an exciting idea.
Lee: What do you mean by second-class scalars?
Andi: The DateTime scalar from the spec could collide with other custom (second-class) scalars.

My five cents how I solved collisions with custom scalars in graphql-compose eg. Date, MongoID – I allow to override my custom scalars.

1) Override build-in type before type construction

import { schemaComposer } from 'graphql-compose';

// before any type construction user may change build-in types
schemaComposer.set('Date', MyCustomDateType);
schemaComposer.set('MongoID', MyCustomMongoID);

schemaComposer.createOTC({
  name: 'User',
  fields: {
    id: { type: 'MongoId' },
  }
});

2) Override build-in type on the first usage (no type in registry with such name):

import { schemaComposer } from 'graphql-compose';

const MyCustomMongoID = new GraphQLScalarType({
  name: 'MongoId',
  ...
});

schemaComposer.createOTC({
  name: 'User',
  fields: {
    id: { type: MyCustomMongoID }, // <-- override `MongoId` type in registry, if it not used yet
  }
});

schemaComposer.createOTC({
  name: 'Article',
  fields: {
    id: { type:'MongoID' }, // <-- will use `MyCustomMongoID` from registry
  }
});

const MyCustomMongoID222 = new GraphQLScalarType({
  name: 'MongoId',
  ...
});

schemaComposer.createOTC({
  name: 'User',
  fields: {
    id: { type: MyCustomMongoID222 }, // <-- throw an error, cause in registry already exists type instance with same name `MongoId`
  }
});

So if we allow overriding built-in scalars in graphql we provide a workaround for users who already use DateTime like an object/scalar type.

graphql-compose has type registry and tracks any type addition/usage so it allows overriding type in two ways described above.

But with graphql-js we can provide just 1 way of type overriding - something like:

import { GraphQLDateTime, overrideBuildInType } from 'graphql';

console.dir(GraphQLDateTime); // outputs graphql-build-in type

const MyCustomDate = new GraphQLObjectType({
  name: 'DateTime',
  fields: () => {},
});
overrideBuildInType(MyCustomDate);

console.dir(GraphQLDateTime); // outputs overrided MyCustomDate type

Internally overrideBuildInType will replace object prototype and all properties from MyCustomDate.


My suggestion looks ugly for graphql-js but maybe somebody suggests a better solution for js implementation. But with graphql-compose it works like a charm due to build-in type registry and allows to track the type name collisions.

Anyway, if type overriding cannot be beautifully solved in graphql-js and in other popular languages then my suggestion "add to SPEC build-in type overriding" – have no sense. 👐

@dmitriid
Copy link

What happens if you already have a DateTime scalar? Adding a standard DateTime scalar means a breaking change for some existing clients.

Why would the spec be concerned about clients already implementing a glaring hole in the spec? Specs are supposed to evolve, and DateTime is the most common thing that people implement precisely because the current spec doesn't have it.

Libs implementing the spec will upgrade. People using the libs will upgrade. It's not the end of the world.

Also from the notes:

few have the complexity of date/time. Not straightforward to represent.

🤦 ISO 8601 and RFC 3339 exists for a reason. All the spec has to say is: scalar DateTime guarantees that the field is presented in full RFC 3339 form 2020-09-15T20:19:30+00:00, and not create assumptions as to what people want to represent through these dates.

A voice of reason in the WG:

If you define it as specified in the RFC, most tools should be compatible

Indeed.

@dmitriid
Copy link

Also: it's been more than a year, and there's still no resolution to this bikeshedding

@benjie
Copy link
Member

benjie commented Sep 16, 2020

@dmitriid There's work being done on this; please keep in mind that GraphQL Foundation is ran mostly by volunteers - if you'd like to see things happen faster, please get involved! The GraphQL Working Group have decided to create a GraphQL Scalars project which you can find here:

https://github.com/graphql/graphql-scalars/issues

the beginnings of a website about it here:

https://www.graphql-scalars.com/

and you can read about it in the notes from recent WG calls e.g. https://github.com/graphql/graphql-wg/blob/afff6661e8494b1d96b674b4046d0311439705ec/notes/2020-06-11.md#graphql-scalars-hosting-and-coordination-10m-evan-and-andi

You're more than welcome to start collaborating with Evan, Andi and team to help this project move faster.

@excitedbox
Copy link

Date/time is a must for standardized implementation in the spec and should match the spec used by other systems to prevent DB errors

I just had to fix a Wordpress DB because Advanced Custom Fields changed the date format a while ago and instead of updating their DB they added all kinds of UNMARKED and UNDOCUMENTED "fixes" to their functions.php. My keyboard has a forehead shaped impression now.

This was all ONLY possible because Advanced Custom Fields uses strings and post_meta instead of a date field for storing dates. I had to use fancy regex to distinguish between ALL the types of meta data and pick non-delimited dates (YYYYmmdd) out of field IDs, etc in a table with half a million rows. Just my luck that certain date conversion functions in mysql don't work on fields marked as string.

@sungam3r
Copy link
Contributor

Just wondering. What prevents you from making your own custom scalar (with all that formatting/validation logic) for handling dates properly?

@dmitriid
Copy link

@sungam3r Because it doesn't scale. If I implement this in my own code in my own custom way, it doesn't mean it will be implemented by some other library, or by code that interfaces with my GraphQL server.

As a result, the support for this core data type is as varied as are GraphQL libraries. Apollo or graphql-java don't provide it out of the box, but graphql-go does (but only provides DateTime, with no support for just Date). When such things (and, I repeat myself, these are core data types used by nearly everyone), you really want them properly specced and not rely on implementations to be accidentally compatible.

@mathstuf
Copy link

My 2¢: having a defined type in the spec would allow codegen and "generic" GraphQL libraries to more usefully work with the types. They're, IME, fairly common in practice, have nice, unambiguous RFC specs to refer to (to avoid overly verbose things in the GraphQL spec itself), and are generally well-supported in the languages in use today.

@excitedbox
Copy link

Exactly Data Types need to be standardized. I am guessing you are too young to know about all the issues there used to be with computers back in the day. My Uncle is still scared for life from the trauma 🤣

In addition EVERY system should have record keeping and logs. That means EVERY dev would need to implement it themselves. Then you have devs with little to no experience making things that might end up in production environments.

FUN FACT: The GUI library used by many Arduino projects was actually a C learning project for someone in their twenties which is now in millions of IOT devices along with all the legacy bugs and issues. <---- YOU DON'T WANT THAT IN YOUR DB.

I just started using graphQL and I wouldn't want to be implementing DateTime functions myself. Especially since you have to account for linux, sql and other legacy implementations along with time zones, daylight savings time (which changes on different dates around the globe). A misplaced comma can cost hundreds of millions in damages. Imagine a stock trading app that sells a stock an hour late because it is using daylight saving time on 1 server and not on the other. Or the stock doesn't sell at all because the time is invalid. By the time a problem like that is caught a share price can fluctuate several $ adding up to huge losses.

If you ever want graphQL to be used for anything serious it will need to add a lot of features for safety and security but also convenience. DateTime will have to be part of that sooner or later. I do see it heading that route though. Especially since it already has very good documentation unlike Dgraph. I can't wait until they finally add resolvers into the base implementation.

@bkarlow-optimo
Copy link

"If you ever want graphQL to be used for anything serious it will need to add a lot of features for safety and security but also convenience. DateTime will have to be part of that sooner or later"

Precisely. I reviewed the possibility of using Graphql for a LARGE implementation I am heading up. I cannot use Graphql in that implementation for precisely this reason. This would have been a VERY prestigious implementation of graphql. Now not happening.

@dmitriid
Copy link

dmitriid commented Mar 4, 2023

The GraphQL Working Group have decided to create a GraphQL Scalars project which you can find here:

https://github.com/graphql/graphql-scalars/issues

Meanwhile in March 2023 there's no progress on this.

Meanwhile literally every graphql library has an add-on, plugin or extension to add Date/DateTime to the available types. If this is not a failure of the spec and of the process, I don't know what is.

@benjie
Copy link
Member

benjie commented Mar 5, 2023

@dmitriid
Copy link

dmitriid commented Mar 5, 2023

@benjie

  • A full "working group"
  • Took two years to say "yeah, as everyone was saying from the start, RFC 3339 is good actually"
  • Produced a "spec" that basically says "RFC 3339"
  • Has no mention in the GraphQL spec [1], has no normative requirements, or an obligation on spec developers to follow it

It looks like there's literaly no intention to move this forward.

[1] Well, the current spec draft from Feb 10 2023 has exactly one reference to it: in code in the the scalars section.

@benjie
Copy link
Member

benjie commented Mar 5, 2023

@andimarek I think this issue is complete now; if you agree please go ahead and close it to avoid confusion :)

@dmitriid I don't really know what you mean by move this forward, it seems complete to me. If you'd like to contribute something more, you can add an agenda item to the next working group or you can send a pull request to the DateTime spec or to the GraphQL spec or if it's RFC3339 that you have an issue with you can contribute your own DateTime spec that uses ISO8601 or whatever alternative you would like.

@dmitriid
Copy link

dmitriid commented Mar 5, 2023

@benjie

How is it complete? It's not in the main spec, there's nothing to force consistent and compatible implementations, it's mentioned in passing in the main spec in the same breath (and authority) as literally any other "spec" from any random person on the internet.

@dmitriid
Copy link

dmitriid commented Mar 5, 2023

@benjie

To put it differently: how exactly is this different from the situation right now where every single library implements their own date time?

@benjie
Copy link
Member

benjie commented Mar 5, 2023

When a server implements the above DateTime scalar (which they may call DateTime or RFC3339DateTime or KittenSparkles or whatever they like), they should use the @specifiedBy directive to indicate that the scalar implements the given specification.

Note that the GraphQL specification states:

When defining a custom scalar, GraphQL services should provide a scalar specification URL via the @specifiedBy directive or the specifiedByURL introspection field. This URL must link to a human-readable specification of the data format, serialization, and coercion rules for the scalar.

So whenever there's a custom scalar in a GraphQL API it should have a @specifiedBy directive. When you see a custom scalar using the above spec then you know it claims to conform to that spec, so you can treat it as an RFC3339 datetime.

So:

  • @specifiedBy is in the main spec
  • all schemas should use @specifiedBy for custom scalars
    • changing this to must would be a breaking change to GraphQL
  • the custom scalar specifications force consistent and compatible implementations of those scalars

To put it differently: how exactly is this different from the situation right now where every single library implements their own date time?

Libraries should use @specifiedBy on custom scalars. Assuming that they do, it's a bit of effort to write a custom scalar spec, so most likely they will pick an existing spec. Hopefully the best specs will "float to the top" and become the most popular.

@dmitriid
Copy link

dmitriid commented Mar 5, 2023

@benjie

These are custom scalars. Translation: they are not enforced by the spec.

should doesn't equal to must. And even if some server implements this custom scalar doesn't mean that clients or other server will.

Hopefully the best specs will "float to the top" and become the most popular.

Ah yes. "Hopefully". Exactly what a spec should be and exactly what this issue was about. Not to have an authoritative specification but a "hopefully".

So? How exactly is this issue done when literally nothing has changed?

@benjie
Copy link
Member

benjie commented Mar 5, 2023

If you don't feel this is done, perhaps you should put the work in to advance it yourself? I suggest that you first read the Guiding Principles and figure out how you can make this a must without breaking a large number of existing GraphQL schemas across the planet, and then write up your proposal as an RFC and raise it at a GraphQL Working Group for discussion.

Also note that just because something existing in the spec doesn't mean people are mandated to use it. There are plenty of GraphQL APIs out there that don't use the ID type and instead use String, Int, or even a custom scalar for that purpose. This is entirely reasonable - schema designers may choose the scalars (custom or built-in) that best fit their use cases.

I am finding this discussion to be neither positive nor productive, so I will not be responding further to you unless you get involved in a positive spirit and put forward a proposal to fix the issues that you see.

@andimarek
Copy link
Contributor Author

I consider this issue as successfully done.

We now have a working GraphQL project at https://github.com/graphql/graphql-scalars which allows everybody to publish their custom scalar spec, which then can be used as specifiedBy value.

I published my own preferred version of date time at https://scalars.graphql.org/andimarek/date-time

@dmitriid
Copy link

dmitriid commented Mar 5, 2023

unless you get involved in a positive spirit and put forward a proposal to fix the issues that you see.

What more positive spirit do you want anyone to be in the face of uncaring disinterested people who turn "I think this is a fundamental type that should be a standard GraphQL type" into "yeah, shunt it off to a side project that people will hopefully perhaps use at one point and if not, who cares"

which then can be used as specifiedBy value.

Of course it cannot be used. Because specifiedBy points to a human readable spec and has literally no normative or authoritative power.

But oh well. I've said my piece. If you don't care, why should I?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

No branches or pull requests