cockpit-project / cockpit

Cockpit is a web-based graphical interface for servers.
http://www.cockpit-project.org/
GNU Lesser General Public License v2.1
10.6k stars 1.07k forks source link

Look into dropping date-fns #20653

Open allisonkarlitskaya opened 4 days ago

allisonkarlitskaya commented 4 days ago

We only use this for relative time formatting and we can do that with ~50 lines (possibly copied from date-fns) + Intl.RelativeTimeFormat().

Also: the way we use locales in date-fns means we end up shipping the entire bundle :(

jelly commented 4 days ago

The main entrypoint for formatting dates in Cockpit is timeformat.js. There seems to be one thing the Intl API can't do https://github.com/cockpit-project/cockpit/blob/main/pkg/lib/timeformat.js#L55 so that needs to be checked.

So the two things which need to be ported over are:

// The following options are helpful for placeholders
// yyyy/mm/dd
export const dateShortFormat = () => locales[dateFormatLangDateFns()].formatLong.date({ width: 'short' });
// about 1 hour [ago]
export const distanceToNow = (t, addSuffix) => formatDistanceToNow(t, { locale: locales[dateFormatLangDateFns()], addSuffix });

And specific date-fns usage in podman and machines

Both funnily enough the same function.

Update: See https://github.com/cockpit-project/cockpit-machines/pull/1694

martinpitt commented 4 days ago

@jelly the firstDayOfWeek() function is entirely self-contained. This doesn't use Intl or date-fns. For the two formatRelative() calls in podman/machines I think we should create a wrapper in timeformat.js.

I propose to first create some unit tests for timeformat.js to cover some corner cases and pin down expected behaviour. Then we can more easily port this to a different underlying API. I'm happy to work on this part.

martinpitt commented 4 days ago

See #20655 for type-ifying and unit-testing the current API.

martinpitt commented 4 days ago

The two functions in podman/machines format a date like this:

> formatRelative(Date.now() - 7200000, Date.now())
'today at 7:05 AM'

> formatRelative(Date.now() - 3 * 24 * 60 * 60000, Date.now())
'last Saturday at 9:06 AM'

> formatRelative(Date.now() - 17 * 24 * 60 * 60000 - 60000, Date.now())
'06/08/2024'

This format is used for snapshot creation time (c-machines) and various image/container creation times and health check occurrences.

I want both projects to stop using date-fns directly. Either we move this function to timeformat.js (if we can build it using Intl), or we just replace these with normal absolute dates/times or distanceToNow().

Intl.RelativeTimeFormat() is very basic, though:

> new Intl.RelativeTimeFormat("de", { numeric: "auto" }).format(-1, "days")
'gestern'

> new Intl.RelativeTimeFormat("de", { numeric: "auto" }).format(-3, "days")
'vor 3 Tagen'

> new Intl.RelativeTimeFormat("de", { numeric: "auto" }).format(-1.3, "days")
'vor 1,3 Tagen'

i.e. you have to determine the unit yourself, and it also doesn't do any kind of "fuzzy" rounding. I suppose that's the "~ 50 lines copied from date-fns" part. node.js doesn't even know translations, the above appears in English (like "yesterday" or "3 days ago"). It also doesn't support a "no suffix" mode, i.e. dropping the "ago"/"vor" -- ~but we could use DurationFormat for that~ -- no we can't, not supported by most browsers.

And of course this doesn't get anywhere near the sophistication of formatRelative(). So I vote for compromising on that and just changing the output in machines/podman.

@jelly @allisonkarlitskaya opinions?

allisonkarlitskaya commented 4 days ago

I'm strongly in favour of platform APIs, even if they're "not quite as good". It's not enough of a win to use third party stuff.

Here's the thing we're gonna wanna copy, btw, and we could maybe make it a bit more colourful if you feel it's appropriate:

https://github.com/date-fns/date-fns/blob/main/src/intlFormatDistance/index.ts#L146

jelly commented 4 days ago

I like formatRelative RelativeTimeFormat is not super useful as one already needs to calculate a time diff to see in what "unit" you want to display the relative time. I spent ~ 5 minutes looking around for a small library which implements the same functionality as formatRelative but I couldn't find anything.

martinpitt commented 2 days ago

date-fn's parse() function is rather hard to replace though -- Date.parse() doesn't know about locales, e.g. Date.parse("23.02.2024") is just NaN, and for Date.parse("05/08/2024") you can't specify if it's US or British order. (D***n you America for getting this so wrong! :wink: )

This is such a hard problem, PatternFly's DatePicker even delegates that parsing bit to the caller -- that's the reason why we have this function. (Setting server date/time and scheduled shutdown, both of which use that widget). We wrap that in parseShortDate().

I quickly thought about doing some cockpit.spawn(date -d), but (1) that doesn't even work for German dates even with LC_CTIME=de_DE.UTF8, (2) we can't rely on the matching locale being present on the server, and (3) it's just gross.

I had a quick check on npm, and there's a few hits for "parse date", but none that make me jump for joy..

That function wouldn't be too hard to write if we could assume dateShortFormat() -- then we can do some sort of strptime(). But getting the locale's date format is just as hard -- I found Locale.getCalendars(), but it doesn't even work on Firefox.

We could give up trying to do this, and only allow ISO format (YYYY-mm-dd) as manual date input in that dialog. (The UI calendar selector widget would be unaffected of course). That gets rid of dateShortFormat() and parse, both of which are problematic. Our target audience (admins) can hopefully deal with an input hint that says "YYYY-mm-dd"..

Once we have a solution/decision for these, reimplementing distanceToNow() is a fairly simple SMOP (and there's lots of existing code).

martinpitt commented 2 days ago

Matrix conversation:

17:16:50      pitti │ Garrett: quick gut check: you remember our calendar selector widget, in the timer create and scheduled shutdown dialogs? they also offer you to manually type in the date                                             
17:17:39      pitti │ Garrett: how would you feel like if we stop trying to interpret a locale specific date (what's 03/08/2024? Is it UK or US English?) and just supported ISO format? yyyy-mm-dd; it should be ok for our target audience
17:18:06      pitti │ also, we don't even have an option to select British locale, so it's even broken for them right now (switching month and day)                                                                                         
17:21:12    Garrett │ yeah, sure                                                                                                                                                                                                            
17:21:15    Garrett │ that makes sense                                                                                                                                                                                                      
17:21:32    Garrett │ yeah, we don't have a british or intl. english                                                                                                                                                                        
17:21:38    Garrett │ and that would remove confusion too                                                                                                                                                                                   
17:21:55    Garrett │ and, you're right that our audience is probably familiar with yyyy-mm-dd                                                                                                                                              
17:23:37      pitti │ we've also fixed this part like three times for different languages, it's so brittle                                                                                                                                  
17:23:52    Garrett │ yeah                                                                                                                                                                                                                  
17:23:58      pitti │ Garrett: I'm asking because there's no real browser API for localized date input/parsing, so this is annoyingly hard                                                                                                  
martinpitt commented 2 days ago

I sent PR #20668 to with the above. Not only does that drop the problematic API, it even fixes two ages old bugs!

martinpitt commented 1 day ago

I did an experimental build with removing date-fns, and the size difference is quite significant: packagekit in production mode shrinks from 728 kB to 600 kB, in devel mode from 14 MB to 11 MB. systemd, storage, and sosreport shrink by a similar amount.