Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Change moment to date-fns and use generic dates #1256

Closed
wants to merge 1 commit into from

Conversation

jonathaningram
Copy link
Contributor

Measurements (not gzipped):

Before:

$ du -sh static/assets/*.js
1.0M	static/assets/bundle.js

After:

$ du -sh static/assets/*.js
692K	static/assets/bundle.js

So we shave off a little more than 300KB un-gzipped by using
date-fns instead of moment.

It's worth noting too that there are really only a handful of places at
the moment that we are even needing to format dates: +300KB for
those few places is pretty heavy in my opinion.

date-fns is significantly smaller than moment and has the benefit that
it can be tree-shaken to reduce the bundle size even more.

With this change, dates are now formatted similar to how the activity
log dates are formatted so that they are not specific to US formatting
(i.e. MM/DD/YYYY).

The only thing that is "lost" with this change is the IANA timezone in
the formatting. Instead, it will now just show the timezone offset. It
is arguable whether or not this is a huge loss and long term it may even
be a better idea to only show the local datetime parts and let the user
copy the original RFC3339 time if they need it.

Notice also that I have added TZ=UTC to all of the scripts that run
tests. This is important so that our tests have a reliable base to use for
testing times across local dev and any CI.

Showing dates in action with day/month change and timezone now offset:

image

image

date-fns is significantly smaller than moment and has the benefit that
it can be tree-shaken to reduce the bundle size even more.

With this change, dates are now formatted similar to how the activity
log dates are formatted so that they are not specific to US formatting
(i.e. MM/DD/YYYY).

The only thing that is "lost" with this change is the IANA timezone in
the formatting. Instead, it will now just show the timezone offset. It
is arguable whether or not this is a huge loss and long term it may even
be a better idea to only show the local datetime parts and let the user
copy the original RFC3339 time if they need it.
Copy link
Contributor

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

date-fns v2 isn't stable yet. and i asked around and we prefer the IANA support. but it looks like date-fns is going to support that when v2 comes out. date-fns/date-fns#180. date-fns/date-fns#180 (comment) If you want to move to date-fns when that happens and supports IANA, we will gladly accept it!

On another note, we do like the switch from MM/DD/YYYY to MMM DD YYYY. If you want to make a patch for that, we will take it now.

@@ -69,6 +69,7 @@
"cloudgov-style": "^2.6.5",
"codecov.io": "^0.1.6",
"css-loader": "^0.23.0",
"date-fns": "^2.0.0-alpha.7",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to use prod ready packages (i.e. no alpha)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries.

@jonathaningram
Copy link
Contributor Author

On another note, we do like the switch from MM/DD/YYYY to MMM DD YYYY. If you want to make a patch for that, we will take it now.

Will make that change separately and leave this PR for now until date-fns 2.0 prod.

@jcscottiii
Copy link
Contributor

jcscottiii commented Oct 13, 2017

@jonathaningram on another note, @mogul recently brought up a good point about human readable relative times (e.g. 5 days ago, 1 hour ago)

Example library: https://github.com/yahoo/intl-relativeformat

he also brought up that you can imagine a UX where we show that and if a person hovers above that, they get the absolute time with the time zone if needed

thoughts?

@jonathaningram
Copy link
Contributor Author

human readable relative times (e.g. 5 days ago, 1 hour ago)

@jcscottiii I think that human readable times are great, but we should carefully select where they are used. Github uses this technique for your home page activity feed which is a good use for it - I think one specific reason is that fine granularity does not matter for Github activity.

For us though, I don't think that we should do this for our logs:

image

The reason is that you want that granularity in seeing the seconds and being able to trace what happens second by second (even more granular than just sections). I know this section only shows the most recent 10 (I think) logs but I think that we can actually make this section better by following a similar process to what Google Cloud does and grouping by day:

image

You'll notice that by doing this we remove all that day stutter and users can focus on what's really important.

So then the only other place where we show a date is in the marketplace services list. I think we could show a human readable date there - I honestly don't know what value I personally get out of even showing any date at all there (but maybe I need to use marketplace services more...)

BTW, we can do this using moment (https://momentjs.com/docs/#/displaying/fromnow/) and date-fns (https://date-fns.org/v1.29.0/docs/distanceInWords) so I'd avoid importing another library to do this.

cc @mogul

@mogul
Copy link
Contributor

mogul commented Oct 16, 2017 via email

This was referenced Oct 31, 2017
@jonathaningram
Copy link
Contributor Author

@jcscottiii I'll close this now, although I'd love to revisit it when date-fns v2 is stable so we can reduce bundle size.

Since it came up in this PR, I'll just mention that internally we've made a change to the activity log to show logs in daily buckets. I haven't made a PR for this yet, but here's two screenshots:

screen shot 2017-11-08 at 12 00 03 pm

screen shot 2017-11-08 at 12 01 13 pm

Of course, it looks better when there's successful and failed log entries intertwined 😄

I'll get around to making a PR for this.

cc @mogul

@jonathaningram jonathaningram deleted the ji-date-fns branch November 8, 2017 01:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants