[WIP] Updates to the CalendarDay field

[timleslie]

This PR is a bit of a mixed bag to make general improvements to the CalandarDay field type.

I'll probably break it into multiple smaller PRs as it comes together and we find answers to some open questions.

The docs for this field type need a bit of work and there are a few open questions around the API which need to be clarified.

Things to consider:

• format only applies to the adminUI. Do we want to have this somehow apply to the graphQL API as well? ( Added a format argument to CalendarDay GraphQL queries #2702,
• yearRangeFrom and yearRangeTo only apply to the adminUI. Do we want to apply validation at the graphQL layer as well? This would probably required date from/to rather than year from/to. Probably would want to be a half-open range I guess [from, to).
• Is the Field view component, based on TextDayPicker doing what we want? Do we want to be able to configure it more? How easy is it for someone to use a different component here if they want?
• The Filter component uses ISO8601 formatting, rather than the format config value used in the Field component. Is this correct?

Need to make sure we fix #2748, #2565, #2366

Should also consider #1136

Should look into #215 and see if there's anything useful left in that issue that we still need to address. Joss makes a good comment in here: #215 (comment) that we should probably wait for react-day-picker v8 before launching into any serious UI work.

Loads of interesting related work in #1965.

• We should definitely upgrade to date-fns 2.x. This will be a breaking change, as they changed their format string handling completely.
• An example { from, to } API for date range restriction.

[changeset-bot]

💥 No Changeset

Latest commit: 2664c8c

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Vultraz]

FTR, I'm not sure in hindsight that the Material day picker was a great idea. It kinda clashed theme-wise with Arch, and it runs the risk of making everything look too much like generic Android.

Regarding date-fns 2.x, based on my work in #1965 the change wasn't massive. The biggest change is mostly stuff like yyyy -> YYYY and (IIRC) there is a compatibility layer one can use. I've been meaning to give the update a second pass at some point.

Definitely 👍 on date range replacing year range.

[timleslie]

Proposal 1 - Done

Add two new config options, { dateFrom, dateTo } to replace { yearRangeFrom, yearRangeTo }.

• Each is optional and takes a string in ISO8601 format (YYYY-MM-DD).
• Validation would be applied at the the graphQL API level.
• The admin UI could optionally use these values to perform client side validation and/or restrict the input fields range.
• The option value would be validated as a valid ISO8601 string when the system started and throw an error if invalid.
• If both options were set, they would be validated that dateFrom <= dateTo when the system started and throw an error if invalid.

This proposal would replace the { yearRangeFrom, yearRangeTo } which a) didn't support days/months and b) only applied client side validation in the admin UI. The graphQL API could still be used to set any value. It's hard to imagine a use case where you would want validation in the admin UI which wasn't also enforced by the graphQL API.

[timleslie]

Proposal 2 - done

Upgrade date-fns to version 2.x.

This will be a breaking change to this field type (and the other date/time fields) as the formatting rules of that library have changed. These changes make for a much more complete formatting API, which we should adopt.

Update: #2927 and #2930 Provide implementation for this.

[timleslie]

Proposal 3 - Done

Update the TextDayPicker component to use react-day-picker version 8.x, which is currently in alpha, but appears to be under active development at the moment.

Note: Arch component not used and can be deprecated

[Vultraz]

Ahhh, that react-day-picker looks nice! Much cleaner than some of the other options on offer (react-date-picker, react-datepicker, etc).

[timleslie]

Proposal 4 - wont fix

Add a format argument to graphQL queries as per #2702 (comment)

This would take a String, validate it as a valid format string, and cause the date to be formatted accordingly when resolved at the field level.

The default would continue to be the ISO860 format (YYYY-MM-DD). I do not think that we should support changing the default, as I want to maintain the symmetry between create/update and reading. I also do not think we should attempt to support alternate formats for create/update.

This would merely be a convenience to users querying the API to prevent them having to include explicit formatting code in their clients.

Update: The current view is that we should not implement this proposal, for reasons put forward by @molomby, which I happen to agree with. Unless someone comes forward with a solid argument in favour of this proposal we will not implement this.

[timleslie]

Proposal 5

Rename the current format argument to adminFormat and update the documentation to indicate that this format is only applied in the list view of the admin UI. It is currently not particularly obvious that this is the case, and users might think that this format is applied at either the storage of graphQL layer.

Update: Alternate proposals:

• { adminConfig: { format } }
• { adminConfig: { formatAs } }

[gautamsi]

Should this be moved to adminConfig for field? Can still call it format then

[gautamsi]

another improvement

also remove use of moment from @arch-ui/day-picker or unify the library being used. another alternative is to use dayjs which claims momentjs compatibility with 90% reduced size.

[MadeByMike]

@timleslie I like all these proposals (except for 4). I don't think we need to provide different options for incoming or out going dates in graphQL.

If we say graphQL queries accept and provide ISO860 format, I think this is much clearer and easier for users to work with. It's easy to implement a format options on the client to make sure this is in the format you want you want and it simplifies our work as well as potentially saving people from a mess of handling multiple different in one application.

[molomby]

My brain dump...

Re: Proposal 1 - From/To Validation Range

Add two new config options, { dateFrom, dateTo } to replace { yearRangeFrom, yearRangeTo }.

Yep, this all sound great. Some ideas:

• We could still support years by assuming from is from the start and to is till the end. Eg. { dateFrom: '2000', dateTo: '2007' } would be interpreted as { dateFrom: '2000-01-01', dateTo: '2007-12-31' }. Supporting this would enable us to be mostly backward compatible with the current yearRangeFrom/yearRangeTo options (by aliasing them).
• CalendarDay values (like these options) are weird pseudo-dates. When I'm working with values like this I distinguish them from actual dates by referring to the former as "days" (as in the name of this type). As such, I don't love having "date" in the name of these options. It would also be nice to call out in the option name that these are used for validation. Maybe something like fromDay, toDay or minValid, maxValid?

I'm not invested in either of these ^^ ideas, just throwing them in the mix.

Re: Proposal 4 - Server-Side Formatting

format only applies to the adminUI. Do we want to have this somehow apply to the graphQL API as well??

Add a format argument to graphQL queries as per #2702 (comment)

To me, applying formatting to a date is about getting it ready for human consumption. This doesn't seem like something that should be part of a data API like GraphQL.

One of the issues is how consistent we want CalendarDay fields to be with DateTime fields, so let me segue into the DateTime type for a bit first...

If you're formatting a a UTC datetime for someone and not accounting for time zones and localisation, you're doing it wrong. 2020-03-09T04:31:11z in d mmmm YYYY isn't "9 February 2020"; if you're in Ecuador it's "8 Febrero 2020". Supporting this server-side requires a function that takes a mask, locale and either a time zone identifier or an offset.

And to what benefit? There are a few use case I can think of where this would be helpful:

• Bundle size -- You're building a site for mobile and trying to keep it really light, server-side date formatting can save you from downloading and parsing 20 to 95 Kb of compressed JS. This is a pretty legitimate use case.
• Data size -- You're building an export that includes createAt and modifiedAt DateTime fields but you actually only need the date (YYMMDD). Stripping the excess data on the server saves you 36 Kb for every 1000 records. Pretty minor but worth mentioning.

Am I'm missing others?

My question is, would we ever want to build out server-side formatting for the DateTime type, complex enough to actually work well, just to solve these limited use cases?

Then, if the answer is "no" (and I suspect it is) then would be want add this functionality to just the CalendarDay field?

Again, I suspect the answer is "no". It's not quite as complex as datetimes as we can ignore time zones but..

• We should really still solve for localisation
• It makes the CalendarDay type conspicuously inconsistent DateTime
• There doesn't seem to be much benefit (unless there's some use case I'm missing)
• Formatting on the server remains architecturally questionable

TL:DR; Supporting this well and constantly will be a lot of work for little gain. Besides, formatting data for human consumption is the browsers job.

(Ps. If we did support this, don't miss the naming discussing in #2702.)

Re: Proposal 5 - Admin Format

Yep, this is useful as config for the Admin UI but, as as @gautamsi notes, it should probably be in adminConfig. Though, personally, I prefer the clarity of formatAs for the name.

Re: The Rest

Proposals 2 and 3 sound good to me.

[Vultraz]

I propose we use the standard interval terminology. Something like interval: { start, end } would fit perfectly with date-fns's own API: https://date-fns.org/v2.13.0/docs/isWithinInterval

[timleslie]

We could still support years by assuming from is from the start and to is till the end. Eg. { dateFrom: '2000', dateTo: '2007' } would be interpreted as { dateFrom: '2000-01-01', dateTo: '2007-12-31' }. Supporting this would enable us to be mostly backward compatible with the current yearRangeFrom/yearRangeTo options (by aliasing them).

I'm not such a fan of this. It makes our API more complex and it introduces an element of magic. If I stumble across someone else's config and it says { dateFrom: '2000', dateTo: '2007' } then it's probably not going to be obvious to me that this goes from the 2000-01-01 to 2007-12-31 and not some other particular choice of days for each year. I'd rather that we have exactly one explicit way to encode this info. I think the maintenance/docs/ongoing confusion cost outweighs the backwards compatibility benefit.

[jesstelford]

I'm +1 for a format input param:

✅ The bundle size issue is quite bad. If I can remove libs like moment or date-fns, etc, from the client, that is a huge win
✅ It'd be an optional parameter, so it's only additive
✅ I would never expect the date to be formatted for any timezone than the one it was stored as (we store the offset info already afaik, so we have everything we need). If the user wants the same moment in time represented in their local timezone, then that has to be done client side because there's no possible way for the server to know where they are.
✅ When I'm querying the GraphQL API, I'm looking at the Playground and what options are available there. if I see a format option, I will use it. But if it's hidden somewhere in the docs and I have to add it myself to my Keystone config, I'm unlikely to use it.

[gautamsi]

@jesstelford dayjs seems to have small footprint and moment compatible (I have not personally tested extensively)

[jesstelford]

In support of a filter input parameter on the GraphQL API... Here's a fun one I just ran into. I get this error from React when doing SSR:

Warning: Text content did not match. Server: "December 2, 2020." Client: "2 December 2020."

This is generated after running an ISO8086 datestring through this function:

new Date(time).toLocaleDateString(undefined, {
    day: 'numeric',
    month: 'long',
    year: 'numeric',
  })

The warning arises because it is run on both server-side and client side. Even on the same computer, there are differences between how the browser formats the date, and how node formats it.

If I could normalise that so it is only ever run in the server (ie; the GraphQL resolver), I would not get such warnings.

Note that this isn't only a console warning, it also causes a flash of wrong content (ie; it shows the node.js version on page load, but after the first client render, it shows the browser version). That's bad.

[Vultraz]

The bundle size issue is quite bad. If I can remove libs like moment or date-fns, etc, from the client, that is a huge win

That was actually what gave me the idea for #2702. The date formatting functionality is already there, it's just a matter of exposing it. Otherwise, you end up with the end user having to add a date lib dependency just to format potentially just one date string 😬

[timleslie]

Re: prop 4

I think the bundle size argument is a particularly compelling argument for this prop. An important question to ask is: can the user get server-side-formatted dates using some other mechanism? I put together the following example using the Virtual field type:

keystone.createList('Todo', {
  schemaDoc: 'A list of things which need to be done',
  fields: {
    name: { type: Text, schemaDoc: 'This is the thing you need to do', isRequired: true },
    date: { type: CalendarDay, adminDoc: 'For testing only' },
    niceDate: {
      type: Virtual,
      resolver: item => item.date && format(parseISO(item.date), 'do MMMM, yyyy'),
    },
    nicerDate: {
      type: Virtual,
      resolver: (item, { formatAs = 'do MMMM, yyyy' }) =>
        item.date && format(parseISO(item.date), formatAs),
      args: [{ name: 'formatAs', type: 'String' }],
    },
  },
});

Caveat: The args config option for Virtual fields doesn't actually exist yet, I added it as a quick hack in my local setup, but I think I'll ship it as a separate PR, because there's no reason not to have it. (Edit: Implemented in #2947 )

I think this pattern is a simple enough mechanism for devs to implement (I'd happily write up a guide on how to do this for the docs) and it allows them the flexibility to make their implementation as simple/complex as they need/want, allows them to pick their formatting library of choice, etc, etc. I think overall handing over these decisions to the dev is a better solution than us trying to make a one size fits all API.

[molomby]

Re: server-side formatting, cont.

Bundle size is important, but remember, for this to actually be a win all date times used in the app (or on a page?) need to be loaded through GraphQL. If you're doing anything else with dates other than loading them from Keystone, you're going to need in-browser formatting capacity.

All that's going to happen is, people will start building an app using the server-side format option, then, after a few months, will need to format a date in the browser and end up pulling in a date library anyway. They'll have half their dates being formatted on the server and half in the browser. We'll shepherd them into a situation that's worse than if they'd just kept formatting concerns on the client (where they belong!) from the start.

Plus, we're talking about 17.5 Kb for the whole of [email protected] and, if you really want to squeeze your bundle you don't even need the whole thing.

It might seem complicated to require and pass locales as options, but unlike Moment.js which bloats your build with all the locales by default date-fns forces developer to manually require locales when needed.

So, given..

• How rare these "smaller bundle" circumstances are likely to be in practice
• How much work is involved to get server-side formatting right, tested, documented, etc.
• How it kind of blurs presentation logic into the data API
• How it shepherd solution devs down what's likely to be a technical dead-end
• And how small the bundle size "win" would actually be..

Is it really worth considering?

The saving grace here is that, yes, it is "purely additional" at an API level and, yes, we can actually solve the entire problem well if we choose to. Formatting like this will be convenient for some small number of apps and, maybe more importantly, it demo's well. It's good for the "first sprint" (I guess).

So yeah, I think it's a bad idea but at having it isn't going to create serious problems.

[Vultraz]

RE removing moment and luxon, there's date-fns-tz which is compatible with date-fns 2 and includes some helpers for timezone-related functionality. Might be able to replace those two and use date-fns[-tz] everywhere.

[MadeByMike]

Did a little spike on adding functions to dayTo/From ... had to revert because of course you can't pass functions across the webpack barrier. This kinda sucks because the most common dayTo value is probably "TODAY". Easy enough to have a function set this at build time, but this needs to be enforced at runtime and shared with the AdminUI.

Proposal 6: Solve this

[MadeByMike]

Ok I've gone through all this work and I'm pretty sure this PR has run it's course...

Proposals 1-4 have been implemented.

No graphQL formatting yet but I like Tim's suggestions of using the virtual field.

Re: 5. - Admin format is contentious in terms of how it should be implemented and definitely not essential, not to be solved here.

Re: 6 - Also not to be solved here.