lib-user: Use a getDefaultDateRange() utility #6420
Conversation
| thisWeekQuery.project_contributions = true | ||
| const { data: thisWeekData } = useStats({ | ||
| sourceId: user?.id, | ||
| query: thisWeekQuery | ||
| }) | ||
|
|
||
| const allTimeQuery = getDateInterval({ | ||
| endDate: todayUTC, | ||
| startDate: getStatsDateString(user?.created_at) | ||
| endDate: getStatsDateString(new Date()), // Today's UTC date string like 24-09-23. |
There was a problem hiding this comment.
Pedantic, but not sure if the 20 in 2024 should be included
| endDate: getStatsDateString(new Date()), // Today's UTC date string like 24-09-23. | |
| endDate: getStatsDateString(new Date()), // Today's UTC date string like 2024-09-23. |
| @@ -1,6 +1,10 @@ | |||
| /* | |||
| Stats dates are always handled in UTC strings such as 24-09-23. | |||
There was a problem hiding this comment.
Similar to other comment
| Stats dates are always handled in UTC strings such as 24-09-23. | |
| Stats dates are always handled in UTC strings such as 2024-09-23. |
There was a problem hiding this comment.
Shoot I just realized I never included these suggested changes. I'll include it in #6472
| return 'year' | ||
| } | ||
|
|
||
| export function getDateInterval({ endDate, startDate }) { |
There was a problem hiding this comment.
Not sure if we need to 1) check endDate or startDate are the date strings we expect and/or 2) set default values (maybe based on getDefaultDateRange)?
There was a problem hiding this comment.
getDateInterval() is only ever called with an object containing endDate and startDate. Checking for those strings and setting default values inside getDateInterval() is how the two actions became intertwined 1) Getting the interval of day, week, month, or year, and 2) setting a default endDate and startDate. The intertwining of those two actions is giving lib-user a bit of spaghetti code.
getDateInterval() is more like formatMyErasQueryForMe(). Instead of using it has a catch-all for cases where endDate and startDate are undefined, I'd rather it be a one-purpose function. You've already type checked endDate and startDate in app-root's App Router pages, and I assume grommet's Calendar component has its own type checking for date range variables.
Does that sound like a good plan to you? I'm definitely willing to disregard this PR in favor of keeping the old getDateInterval() if you prefer, but the above mentioned intertwining of actions and the repeated setting of default props with endDate and startDate as "undefined" feels like a bit of a code smell to me.
There was a problem hiding this comment.
That sounds good to me! I think the separate functions are preferable.
Package
lib-user
Linked Issue
Describe your changes
getDefaultDateRange()utility function to replace falsey date ranges like { endDate: undefined, startDate: undefined }.getDateInterval()'s handling of Javascript Dates and UTC, but realized it's much cleaner to separate out the definition of a default date range from getting the interval for the ERAS query (days, week, month, year).How to Review
Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrapordocker-compose up --buildand FEM works as expected