Migrate to datefns #5065
Migrate to datefns #5065
Conversation
To make the tests pass, I've made it so that the toDate function emulates some of momentjs' questionable choices like using undefined and null to indicate current time. MomentJS also converted undefined and null to 0 in the add and subtract functions. date-fns returns an invalid date in such cases, which caused another test to fail. This is a workaround for that.
I didn't read the comment above and accidentally removed the timezone information.
Earlier, the timeline length was being calculated by moment(end - start).weeks() This would give incorrect results because end - start returns an integer - the number of milliseconds between the two dates(which can then be converted to weeks). Calling moment on it converts it to a date instead of a duration.
While doing some more reading about dates in Javascript, I found that the Date constructor always returns dates in the local timezone. So having a date object in a different timezone is not really possible. Since moment-js had its own wrapper Date object, it didn't have this problem. date-fns operates on the native Date object, so it limited in that regard. So while we can't get a Date object with UTC timezone, getting a string with UTC timezone is possible. This is what toISOString() does
Most of these methods didn't need to be in UTC since they were only comparing dates. So I've used local time for these. The only place where we display the time in UTC was in the StatisticsUpdateModal. I've kept it this way.
|
Are there any UX changes you know of from this? If so, can you provide before/after screenshots? |
|
Well, the only thing I can think of is in Before
Now, its all consistent.
Other than that, there shouldn't be any major differences if I've done everything right |
| @@ -8,5 +8,48 @@ export const toDate = (date) => { | |||
| if (date instanceof Date) { | |||
| return date; | |||
| } | |||
| return parseISO(date); | |||
| // should not really happen in production, but it's here for making the tests pass | |||
There was a problem hiding this comment.
How hard would it be to fix the tests so that this isn't necessary? If it shouldn't happen in production, hopefully it doesn't... but it this would probably prevent us from discovering it if it was happening in production.
There was a problem hiding this comment.
Hmm, I think this affects one or two React tests so shouldn't be too difficult to fix. Should I just raise an Error if this ever happens? That would fail the CI so if it is happening right now we would have an idea where and how.
There was a problem hiding this comment.
Yes, I think raising a console error (and then returning the new Date, so that it would still work the same way as with moment.js) would be a good way to flush out any places where this happens in production.
| return parsedDate; | ||
| }; | ||
|
|
||
| export const formatWithTime = (date) => { |
There was a problem hiding this comment.
I think formatWithTime and formatWithoutTime might have room for improvement in the names. Format what? A Date? A DateTime? I think this works for a Date or a DateTime, right?
Maybe formatDateAndTime and formatDate (or formatDateWithoutTime)?
There was a problem hiding this comment.
It works for any valid ISO 8601 string or a Date object. I'll improve the function names to reflect that.
| return parsedDate; | ||
| }; | ||
|
|
||
| export const formatWithTime = (date) => { |
There was a problem hiding this comment.
Comments documenting the expected constraints on the date input (strings? Date objects?) would be helpful for all these functions.
There was a problem hiding this comment.
All the functions here should work for valid ISO 8601 strings or Date objects. I'll add a comment for that.
this should tell us if this happens in production
A couple of tests were failing because we were accessing the incorrect attribute, while in another one we weren't setting the date. There's still a JS error for the user profile page but that will require a bit more work and refactoring to solve satisfactorily. For now, this suppresses that error.
I've also changed the output format for datetime strings. Now, everything is 0 padded. This just makes it easier to test
Moment did this by default but date-fns requires you to pass an option.
This removes
moment-jsas a dependency.With this,
vendors.jsgoes from1.42MBto1.37MB