Use date-fns instead of moment #258
Conversation
There was a problem hiding this comment.
Awesome, thanks! This looks great to me, especially since date-fns work with tree shaking. This should save us some bytes.
Thanks for considering the tests. It would be hard to do a realistic test regarding several of these changes since they involve headers returned from the server, but they appear to be straightforward so 👍 .
| @@ -235,7 +234,8 @@ export function uploadFile(file, uiOptions, onProgress, onChange, onError) { | |||
| } else { | |||
| let errorMessage = req.statusText; | |||
| if (req.status === 429) { | |||
| errorMessage = `You’ve reached the limit for the number of submissions we can accept at this time. Please try again in ${timeFromNow(moment.unix(parseInt(req.getResponseHeader('x-ratelimit-reset'), 10)))}.`; | |||
| const resetDate = new Date(req.getResponseHeader('x-ratelimit-reset') * 1000); | |||
There was a problem hiding this comment.
Noting that the date conversion here looks right since the original was in seconds-since-epoch and JS is milliseconds-since-epoch. There seems to be some controversy about whether this is a relative time or an absolute one. I can see the point about absolute being risky because it assumes relatively synchronized clocks. In any case, this still does whatever it did before, whether right or wrong. 😺
| @@ -62,7 +61,7 @@ export default function SubmitButtons(props) { | |||
| <div className="usa-alert usa-alert-error schemaform-failure-alert"> | |||
| <div className="usa-alert-body"> | |||
| <p className="schemaform-warning-header"><strong>We’ve run into a problem</strong></p> | |||
| <p>We’re sorry. Your submission didn’t go through because we received too many requests from you. Please wait {timeFromNow(moment.unix(submission.extra))} and submit your request again.</p> | |||
| <p>We’re sorry. Your submission didn’t go through because we received too many requests from you. Please wait {timeFromNow(new Date(submission.extra * 1000))} and submit your request again.</p> | |||
There was a problem hiding this comment.
Weird, this is basically the same message but phrased a different way?
| * @param date {Moment Date} The future date to check against | ||
| * @param userFromDate {Moment Date} The earlier date in the range. Defaults to today. | ||
| * @param date {Date} The future date to check against | ||
| * @param userFromDate {Date} The earlier date in the range. Defaults to today. | ||
| * @returns {string} The string description of how long until date occurs | ||
| */ | ||
| export function timeFromNow(date, userFromDate = null) { |
There was a problem hiding this comment.
Just noting for future optimization that the only place where we use timeFromNow (and thus all the differenceIn* from date-fns is the "Please don't push Submit so often" message. Subtracting the two dates and expressing in terms of integer minutes is probably all we need there. I think we should land this as-is though and work on these details later.
|
@timwis Sorry, I waited too long to land this and it has merge conflicts. It should be fine if you rebase against current master, the raven-js dependency is gone now. Let me know if you have problems and I can do it locally. |
timwis
force-pushed
the
refactor/date-fns
branch
from
4618d61 to
9cabe84
Compare
|
@dmethvin-gov no worries, should be all sorted now. 👍 |
This reverts commit 96755f4.
Helps address #152, replacing the
227 KBof moment.js with29 KBof date-fns, a popular utility library for date functions that's structured similarly to lodash.Please note that I don't believe all the affected functions have unit tests, and I'm not deeply familiar with this codebase, so I'd appreciate a second set of eyes on it! I would hate to have accidentally changed the logic of an otherwise minor function.
Also note that we may be able to remove
timeFromNowin favour of date-fn'sdistanceInWordsToNowand possibly a few other DIY date functions in here, but I wanted to limit the scope of this PR.Types of changes
Checklist:
npm run lint.npm run build.npm test.