Replace moment with date-fns in hca #12112
Conversation
It uses a helper that uses moment, and it isn't used anywhere else
Uses moment and it isn't used anywhere else
Note: the formatting is slightly different since date-fns doesn't append a period to truncated month names like moment apparently does.
The lack of tests make me worry a bit - I may add some.
The dates are passed in as ISO date strings
This is a bit tricky - the function was using `_.get()` to try to access a var that didn't exist, so the default formData object was always used instead. When moment() is called with any non-date object as its parameter, it defaults to now. So, essentially the `dob` variable was always being assigned to the current datetime.
This undos some changes in the previous commit so that moment & lodash are still used to calculate the dob const. I may have missed something in the refactor
I misread the lodash function. formData isn't the default, it is the root object that the first argument (the string path) is applied to. If _.get() returns undefined, moment() will parse it as the current time. The native JS Date.parse() will return NaN instead
This isn't really necessary for the PR, but it is an improvement
| @@ -65,7 +65,9 @@ export class ConfirmationPage extends React.Component { | |||
| <li> | |||
| <strong>{dateTitle}</strong> | |||
| <br /> | |||
| <span>{moment(response.timestamp).format('MMM D, YYYY')}</span> | |||
| <span> | |||
| {format(parseISO(response.timestamp), 'MMM d, yyyy')} | |||
There was a problem hiding this comment.
response.timestamp is expected to be an ISO-formatted date string.
| @@ -89,7 +89,7 @@ export function getEnrollmentDetails( | |||
| blocks.push( | |||
| <> | |||
| <strong>You applied on: </strong> | |||
| {moment(applicationDate).format('MMMM D, YYYY')} | |||
| {format(Date.parse(applicationDate), 'MMMM d, yyyy')} | |||
There was a problem hiding this comment.
applicationDate is the first argument in getEnrollmentDetails. Even though there isn't a unit test specifically for this function, it is used in getWarningStatus(), and the second argument is passed directly through.
| @@ -98,7 +98,7 @@ export function getEnrollmentDetails( | |||
| blocks.push( | |||
| <> | |||
| <strong>We enrolled you on: </strong> | |||
| {moment(enrollmentDate).format('MMMM D, YYYY')} | |||
| {format(Date.parse(enrollmentDate), 'MMMM d, yyyy')} | |||
There was a problem hiding this comment.
enrollmentDate has the same story as applicationDate. It may be wise to add some unit tests for this function.
| @@ -971,7 +971,7 @@ export function getAlertContent( | |||
| blocks.push( | |||
| <p key="you-applied-on"> | |||
| <strong>You applied on:</strong>{' '} | |||
| {moment(applicationDate).format('MMMM D, YYYY')} | |||
| {format(Date.parse(applicationDate), 'MMMM d, yyyy')} | |||
There was a problem hiding this comment.
This is in an untested function. The only place it is used is in src/applications/personalization/dashboard/components/HCAStatusAlert.jsx
| @@ -26,7 +26,7 @@ describe('hca <ConfirmationPage>', () => { | |||
| const tree = SkinDeep.shallowRender(<ConfirmationPage form={form} />); | |||
|
|
|||
| expect(tree.subTree('.claim-list')).to.exist; | |||
| expect(tree.everySubTree('span')[2].text()).to.contain('Jan. 1, 2010'); | |||
| expect(tree.everySubTree('span')[2].text()).to.contain('Jan 1, 2010'); | |||
There was a problem hiding this comment.
The formatting for abbreviated months in date-fns does not include a period for truncated months.
There was a problem hiding this comment.
The content style guide indicates they have to have a period. I think we can use the escape character to add a period in the format string. Maybe
{format(parseISO(response.timestamp), "MMM'.' d, yyyy")}
There was a problem hiding this comment.
I'll mess around with this - we may have some edge cases to account for with shorter months (i.e. March. 14, 2020), so I'll see how it handles those.
There was a problem hiding this comment.
That true - we aren't supposed to abbreviate those.
There was a problem hiding this comment.
I've seen some code that Eugene wrote to handle this exact thing for downtime notifications.
There was a problem hiding this comment.
Closing for now. The extra work to ensure that every format call meets the content guidelines isn't worth it in my opinion. I'd like for date-fns to provide a way of changing the default locale so we have an easier way of controlling things like this.
| )} (${endDateInfo.description} from today)`, | ||
| ); | ||
| } | ||
|
|
||
| if (veteranDateOfBirth) { | ||
| const dateOfBirth = moment(veteranDateOfBirth); | ||
| const dateOfBirth = Date.parse(veteranDateOfBirth); |
There was a problem hiding this comment.
veteranDateOfBirth is also an ISO date string.
|
|
||
| if (dateOfBirth.add(15, 'years').isAfter(moment(lastEntryDate))) { | ||
| if (isAfter(addYears(dateOfBirth, 15), Date.parse(lastEntryDate))) { |
There was a problem hiding this comment.
lastEntryDate is also an ISO date string.
| const vetDOB = Date.parse(veteranDateOfBirth); | ||
| const spouseDOB = Date.parse(spouseDateOfBirth); | ||
| const marriage = Date.parse(marriageDate); |
There was a problem hiding this comment.
marriageDate is the second argument in validateMarriageDate. All of these three arguments are ISO date strings.
vets-website/src/applications/hca/tests/validation.unit.spec.js
Lines 93 to 97 in 8f57a94
| const dob = moment(_.get(`dependents[${index}].dateOfBirth`, formData)); | ||
| const dependent = Date.parse(dependentDate); | ||
| const dob = | ||
| Date.parse(formData?.dependents?.[index]?.dateOfBirth) || Date.now(); |
There was a problem hiding this comment.
This is a bit confusing. The arguments in the previous _.get() call seem swapped - the object should be first and the path second.
However, the _.get() call was likely resulting in undefined each time, and moment(undefined) returns the current datetime:
Note: Function parameters default to undefined when not passed in. Moment treats moment(undefined) as moment().
There was a problem hiding this comment.
if the optional chaining returns undefined, Date.parse(undefined) will be NaN, so the || Date.now() should achieve the same thing that was previously happening.
There was a problem hiding this comment.
The arguments in the previous _.get() call seem swapped
It's lodash/fp.
if the optional chaining returns undefined, Date.parse(undefined) will be NaN, so the || Date.now() should achieve the same thing that was previously happening
This looks fine. maybe add a comment explaining that Date.parse returns NaN when applied to undefined.
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
| import moment from 'moment'; | |||
| import { format } from 'date-fns'; | |||
There was a problem hiding this comment.
love it when libraries use common terms for their functionality
| @@ -26,7 +26,7 @@ describe('hca <ConfirmationPage>', () => { | |||
| const tree = SkinDeep.shallowRender(<ConfirmationPage form={form} />); | |||
|
|
|||
| expect(tree.subTree('.claim-list')).to.exist; | |||
| expect(tree.everySubTree('span')[2].text()).to.contain('Jan. 1, 2010'); | |||
| expect(tree.everySubTree('span')[2].text()).to.contain('Jan 1, 2010'); | |||
There was a problem hiding this comment.
The content style guide indicates they have to have a period. I think we can use the escape character to add a period in the format string. Maybe
{format(parseISO(response.timestamp), "MMM'.' d, yyyy")}
|
Closing for now: #12112 (comment) |
Description
This is part of department-of-veterans-affairs/va.gov-team#6763
This removes all explicit usage of
momentfrom thehcaapp. The forms system still uses moment, so it will still get used implicitly in the app.Testing done
Local unit testing
Screenshots
Acceptance criteria
Definition of done