bbc / simorgh

The BBC's Open Source Web Application. Contributions welcome! Used on some of our biggest websites, e.g.
https://www.bbc.com/thai
Other
1.39k stars 219 forks source link

Investigate library for dates #637

Closed sareh closed 5 years ago

sareh commented 6 years ago

Is your feature request related to a problem? Please describe. Investigate library for dates and timestamps, for the Persian article.

moment https://momentjs.com, with moment Jalali plugin, for the Persian calendar https://github.com/jalaali/moment-jalaali date-fns https://date-fns.org/

Example of Persian timestamps/dates:

persian timestamps

Here is a codepen showing absolute and relative timestamps https://codepen.io/sareh/pen/rrvaqE

Describe the solution you'd like As a result of this investigation, we need to have an understanding of the pros and cons of these libraries and a recommendation for which to use (or neither) with sufficient reasoning. We should write issues for implementing the recommended implementation.

Write an ADR, writing up our decision.

pjlee11 commented 5 years ago

The following is a brain dump from the group investigation performed by those assigned to the PR.

Brief Comparison of libraries

Name Size (gzip) Methods richness Pattern Timezone Support Locale Relative timestamps
Moment.js 329K(69.6K) High OO Good (moment-timezone) 123 Yes
date-fns 78.4k(13.4k) High Functional Not yet 32 Yes
dayjs 6.5k(2.6k) Medium OO Not yet 23 Yes (as a plug-in)

More detailed comparrison of the libraries - https://github.com/you-dont-need/You-Dont-Need-Momentjs

Moment.js

Pros

Cons

date-fns

Pros

Cons

Day.js

Pros

Cons

Build our own

This would be similar to how we did timestamps for AMP.

Pros

Would be built to our use case and optimised for size

Cons

Summary of libraries

Around the BBC

Outcome

Each of the following should be evaluated in terms of size (gzipped and webpack bundled) and user pros / cons

pjlee11 commented 5 years ago

For each POC we should install the library and attempt to create the following timestamps:

The markup should be

<time datetime="YYYY-MM-DD HH:MM:SS{+Timezone}">{formattedTimestamp}</span>

We should then detail the weight the library (including plugins) adds to the application, which should be possible to see following a npm run build and opening the following file in a browser simorgh/reports/webpackBundleReport.html. Also detail any other thoughts/findings

pjlee11 commented 5 years ago

As part of the knowledge sharing day, together we created the branch timestamp-investigation which creates a new component LocalisedTimestamp and adds tests for /news and /persian expecting absolute and relative timestamps for both. This should be used as a base for the proof of concepts in a red green refactor methodology.

pjlee11 commented 5 years ago

Sareh, Alison, Marcos and Brian are performing a POC using date-fns. Myself, Ben, Che and Alex are performing a POC using dayjs.

sareh commented 5 years ago

Date-fns

Pros

Cons

//en
dateFns.format(timestamp * 1000, 'd MMMM yyyy');
//fa
dateFns.format(timestamp * 1000, 'd MMMM yyyy', {locale: fa});
bcmn commented 5 years ago

dayjs

Pros

Cons (or areas that would need to be explored, at the very least...)


The proof of concept is on the branch dayjs.

If I get a chance, I might have a quick look into using moment-timezone alongside dayjs to verify our current assumption that it would be possible to use the two together.

pjlee11 commented 5 years ago

Following the 2 POC's we have the following questions: 1) Which non-Gregorian calendars do we need to support? Jim to ask WS product and WS editorial 2) How much work is it to override numerals for a given locale in dayjs? 3) In the short term could we implement the math for a conversion to the Jalali calendar locally before attempting to get changes made in dayjs? 4) Is a dual calendar a useful real world thing or is it very BBC specific and therefore should we use a library to create a Gregorian calendar, a Jalali calendar and combine them within our codebase?

Answers 2) We should POC locally overriding the latin numerals

jimjohnsonrollings commented 5 years ago
bcmn commented 5 years ago
  • POC: Moment.js with locales and timezone
  • POC: Date-fns - Findings
  • ~POC: Date-fns with moment timezone~ (From the above findings we don't think date-fns supports our use case)
  • POC: Day.js Findings
  • POC: Day.js with moment timezone
  • SPIKE: Make moment more granular and locked into a single locale, could be raised as an issue/question in moment's github repo

Following up on this, from their docs, it does not appear it is possible to use moment-timezone with non-moment libraries:

To use moment-timezone, you will need moment@2.9.0+, moment-timezone.js, and the moment-timezone data.

bcmn commented 5 years ago

Just to quickly show how big an impact Moment & Moment-timezone has on the webpack bundle report, following a very quick look into it this morning:

With DayJS screen shot 2018-11-06 at 10 58 24

With Moment screen shot 2018-11-06 at 10 52 37

bcmn commented 5 years ago

Just to document all these thoughts somewhere: I recently tried to make a proof of concept of the numeral switching we'd discussed adding to DayJS. While a basic implementation was possible, it raised some interesting issues:

I plan to open an issue/talk it over with the DayJS devs to see if they have a preferred solution I'm missing.

bcmn commented 5 years ago

Delayed because I was on holiday/accidentally deleted a draft, but here is the DayJS issue on non-arabic numerals.

ChrisBAshton commented 5 years ago

We tackled this recently in VJ: https://github.com/bbc/cmu-timeline/pull/21

TLDR; opted for toLocaleString, falling back to a simple dd/mm/YY format for browsers that don't support it. Wanted to avoid the bloat of a library.

amywalkerdev commented 5 years ago