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

Have we gotten the date type right? #79

Closed
bjoerge opened this issue May 11, 2017 · 11 comments
Closed

Have we gotten the date type right? #79

bjoerge opened this issue May 11, 2017 · 11 comments

Comments

@bjoerge
Copy link
Member

bjoerge commented May 11, 2017

While working yesterday with importing data from firebase to Sanity, it struck me that our date-type has some issues wrt. developer ergonomics.

When importing documents that already had several fields with unix timestamps, I had to convert all of them to objects of this shape before importing:

{
  _type: 'date',
  local: '2017-02-08T01:30:00+01:00'
  offset: 120
  timezone: 'Europe/Oslo'
  utc: '2017-02-08T00:30:00Z'
}

WHY do I need to store it like this?

TBH this felt like a violation of the easy things are easy-principle. There's probably a ton of cases where it makes perfectly good sense to store dates like this, but at least I'd like to know the reasoning behind it (documentation issue).

HOW do I format a date like this?

Do I need a timezone library? Have to drag in moment? Which fields here is absolutely required? What if I only have UTC date, do I need to add timezone still? What should that be, system timezone?

There are date fields on documents already (_createdAt, _updatedAt). Of what type are these? Apparently not date.

Sometimes you just want a field to be stored as a utc date (just like _createdAt, etc.), but we have no schema type for it. So: If I actually just want to store an utc date as a field (and still have it editable with a date picker in the studio), there's currently no way to do it.

Additionally some queries ends up looking a bit awkward:

*[_type == 'article'] | order(_createdAt desc, publishAt.utc asc)

...and really easy to get wrong:
(this will not work as expected, nor return any errors telling you about your mistake!)

*[_type == 'article'] | order(_createdAt desc, publishAt asc)

Possible solution

I think we should optimize for the most common use cases, but still support the less common (but nonetheless important) use cases.

We can do this by introduce a schema type for utc date strings (date, dateTime, instant?). My guess is that this would cover the majority of use cases involving dates. If one wants to preserve timezone information, we should provide a separate builtin schema type (maybe zonedDateTime?), which is an object that includes timezone information much like the date type we have today. (This data type could actually just be modelled by the schema author too, but I'm ok with making it a builtin type).

Terminology

It may be a good idea try align our terminology as closely as possible to the current ECMAScript temporal proposal which is based on established terminology from years of accumulated experience from other programming languages:

https://github.com/maggiepint/proposal-temporal

Thoughs?

@thomax
Copy link
Contributor

thomax commented May 11, 2017

Our current date type does indeed support storing only the utc version of a point in time, but as you point out this is not explicitly documented.

Disregarding our lacking documentation, this discussion comes down to wether or not we should include a date-like type which is only represented by a single string:

publishedAt = '2017-02-08T00:30:00Z'

In addition to the current type, which is an object:

publishedAt = {
  _type: 'date',
  utc: '2017-02-08T00:30:00Z'
}

...amirite?

If so, I'm all for implementing such a type, date picker and all!

I think instant is a fitting name for this type. Obviously not date, because it's occupied by the existing type. Also, date-time does not work because it implies that our current date type has a resolution of 24 hours.

@bjoerge
Copy link
Member Author

bjoerge commented May 11, 2017

...amirite?

Yes!

I think instant is a fitting name for this type. Obviously not date, because it's occupied by the existing type. Also, date-time does not work because it implies that our current date type has a resolution of 24 hours.

Regardless of the choices we've already made, I still think its worth having a discussion about terminology, figure out how it should have been, and then explore whether its possible to fix it.

If someone asked me to guess what type to use for a field to get a widget that gives me the value 2017-02-08T00:30:00Z, I'm pretty sure I would have guessed that that type was named date.

I think its very common to describe those as ISO-formatted date strings too.

Also, I think its mildly confusing that this is what a date looks like:

publishedAt = {
  _type: 'date',
  utc: '2017-02-08T00:30:00Z'
}

...while this in contrast would be a value of some other type:

publishedAt = '2017-02-08T00:30:00Z'

Ideally I'd like the type of the ISO date string to be named date (or datetime), and choose a more descriptive name for the type of values that preserves additional metadata like timezone, etc (e.g. zonedDate).

@bjoerge
Copy link
Member Author

bjoerge commented May 11, 2017

@rexxars
Copy link
Member

rexxars commented May 16, 2017

After thinking about this for a few days, I tend to agree with @bjoerge - we should probably provide a simple date / datetime type which is basically just a string in ISO-8859-1 format (as with the automatic _createdAt fields etc, and rename the current date implementation to something else.

I think we got a little caught up in trying to figure out how to do a proper date/time type that could cater to even advanced use cases that we forgot to factor in the difficulty of using it. With good documentation, we should be able to surface both types and the difference between them.

@thomax
Copy link
Contributor

thomax commented May 16, 2017

Then everybody's all for a string-based date-type, it seems?

Renaming the current date type: The only real hurdle would be to make/help users update their schemas and migrate all their _type: 'date' --> _type: 'dat-new-date-obj' data. But should be doable?

I'm still of the opinion that date and datetime are mutually exclusive type names, because of what they imply about each other.

Are you guys set on date, rather than instant, as the name of the string-based date type? I absolutely see the sense in reserving the date name for the (probably) most frequently used type, but I struggle to come up with a decent name for the object-based date type.

@thomax
Copy link
Contributor

thomax commented May 22, 2017

After a RL discussion we agree that we want two types for storing dates:

date - A plain ISO-formatted UTC string, e.g.:

2017-05-22T08:43:51.823Z

Can be configured in the schema to only store date (not time) information, in which case the time portion of the string is set to midnight, e.g. 2017-05-22T00:00:00.000Z.

richDate - An object containing various detailed date/time info, e.g.:

{
  utc: `2017-05-22T08:43:51.823Z`,
  local: `2017-05-22T10:43:51.823+02:00`,
  timezone: 'Europe/Oslo',
  offset: 120
}

The richDate will probably also support precision (year only etc) in the future.

Questions in need of clarification:

  1. Does the date type have a schema option for timezone so it can be interpreted for use in a specific (single) locality?
  2. Does the richDate type keep local key? Pro: Quick lookup of local time when reading data. Con: Feels bureaucratic when writing data. Maybe make the local key optional?
  3. Changing content of the date type from object to string is a breaking change. How do we handle migrating (customer) schemas and data?

@bjoerge
Copy link
Member Author

bjoerge commented May 22, 2017

Sounds good to me!

Does the date type have a schema option for timezone so it can be interpreted for use in a specific (single) locality?

I think that makes sense. The default should be to display it in user (browser) timezone, but we could provide an option to specify that the date should be displayed in a specific timezone. Would that require including a timezone database? How do we deal with this today?

Does the richDate type keep local key? Pro: Quick lookup of local time when reading data. Con: Feels bureaucratic when writing data. Maybe make the local key optional?

The more I think about this, the less I'm in favor of a storing a redundant local key. While it may be tempting to do for the sake of convenience, it may also be confusing and cause potential bugs. E.g. what happens if the utc field is updated without the local following suit?

@thomax
Copy link
Contributor

thomax commented Jun 8, 2017

We now have a date type which is represented by a string. And a richDate type which is an object (just like date used to be).

Currently, both types can be configured to accept and store localized (e.g. 2017-02-21T10:15:00+01:00) or UTC (e.g. 2017-02-12T09:15:00Z) time using the options.inputUtc. I'm thinking it's probably a good idea to let inputUtc default to false, as most user input will probably be from a localized perspective. We could also granularize this further with options.inputUtc and options.storeUtc which controls what the user sees and what the stored data looks like, respectively? Or we could let inputUtc only control the input, and always store a UTC string?

Any thoughts on this?

Other things to consider, but which will probably not cause breaking changes and can thus be postponed:

  • Move from moment to date-fns for a smaller bundled footprint. However, we're currently using react-kronos (https://github.com/dubert/react-kronos) as UI widget which in turn relies on moment, so no benefit unless we swap out react-kronos.
  • Consider if we really need/want the timezone for richDate, as it increases bundled size from 17 kB to 42 kB (moment vs moment-timezone)
  • Enable user input of timezone?
  • Let schema configure which timezones are available to choose from?

Here's the code, which should be production ready unless we want to change some behavior:
https://github.com/sanity-io/sanity/commits/feature/date-refactor

@thomax
Copy link
Contributor

thomax commented Jun 12, 2017

After another RL discussion, we've agreed to roll with the above implementation. To summarize:

  • Two date types. Date (string) and RichDate (object).
  • options.inputUtc is default false and controls both what the user inputs and what is stored in the document
  • Because groq/gradient should soon be able to support both comparing and sorting both utc and localized time strings, we're all set!

Up next:

  1. Make a plan for migrating customer data + schemas.
  2. Merge to production & deploy.

@thomax
Copy link
Contributor

thomax commented Jun 15, 2017

For the record, Gradient issue on new handling of date strings here https://github.com/sanity-io/gradient/issues/402

@bjoerge
Copy link
Member Author

bjoerge commented Sep 21, 2017

Renamed to richDate in v0.113.0 🎉

@bjoerge bjoerge closed this as completed Sep 21, 2017
kristofferjs pushed a commit that referenced this issue Feb 6, 2018
* [components] Inherit color on state colored dialogs. Fixes #33 (#571)

* [form-builder] Center with flex. Fixes #79
kristofferjs pushed a commit that referenced this issue Feb 12, 2018
* [components] Inherit color on state colored dialogs. Fixes #33 (#571)

* [form-builder] Center with flex. Fixes #79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants