fix: improve date parsing for collection items #732

dcshzj · 2024-10-08T02:18:04Z

Problem

Some of the dates in collections show up as invalid date because the allowed date format is very specific.

Solution

Breaking Changes

Yes - this PR contains breaking changes
No - this PR is backwards compatible

Bug Fixes:

Improve the date parsing of collection items.

vercel · 2024-10-08T02:18:09Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
isomer-studio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 11, 2024 8:16am

datadog-opengovsg · 2024-10-08T02:21:32Z

Datadog Report

Branch report: fix/parse-collection-dates
Commit report: 84b1fc2
Test service: isomer-studio

✅ 0 Failed, 8 Passed, 1 Skipped, 7.2s Total Time

karrui

suggest keeping ISO format instead of the new dd/mm/yyyy format. please use date-fns instead!

packages/components/src/templates/next/layouts/Collection/Collection.stories.tsx

packages/components/src/templates/next/layouts/Collection/utils.ts

seaerchin

do you think we should have an equivalent function for dates in studio so that this implicit contract is kept?

seaerchin · 2024-10-09T18:48:56Z

packages/components/src/utils/getFormattedDate.ts

- "DD MMM YYYY",
- "YYYY-MM-DDTHH:mm:ss.SSSZ",
- ]).format("D MMMM YYYY")
+export const getFormattedDate = (dateString?: string) => {


we can make use of a branded type here so that we know this is a special "formatted date" string when this function finishes processing

I think usually the result is directly used and not further processed, but added in 2daaaf2 for forward compatibility.

seaerchin · 2024-10-09T19:00:37Z

packages/components/src/utils/getParsedDate.ts

+ try {
+ if (isMatch(dateString, "dd/MM/yyyy")) {
+ return parse(dateString, "dd/MM/yyyy", new Date())
+ }
+
+ if (isMatch(dateString, "d MMM yyyy")) {
+ return parse(dateString, "d MMM yyyy", new Date())
+ }
+
+ if (isMatch(dateString, "d MMMM yyyy")) {
+ return parse(dateString, "d MMMM yyyy", new Date())
+ }
+
+ if (isMatch(dateString, "dd MMM yyyy")) {
+ return parse(dateString, "dd MMM yyyy", new Date())
+ }
+
+ if (isMatch(dateString, "dd MMMM yyyy")) {
+ return parse(dateString, "dd MMMM yyyy", new Date())
+ }
+
+ if (isMatch(dateString, "yyyy-MM-dd")) {
+ return parse(dateString, "yyyy-MM-dd", new Date())
+ }
+
+ if (isMatch(dateString, "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'")) {
+ return parse(dateString, "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'", new Date())
+ }


what do we feel about using a reducer pattern here - [formats].reduce(...) with the starting value as new Date?

Ah yes, good idea, updated in 2daaaf2.

seaerchin

lgtm. we should shift the try/catch inside cos that impacts readabiliy and the wideness of the try/catch makes it difficult to tell exactly what we're catching

leave it to you if we want to refactor the reduce orders

seaerchin · 2024-10-11T06:48:22Z

packages/components/src/utils/getParsedDate.ts

+ if (isMatch(dateString, format)) {
+ return parse(dateString, format, new Date())
+ }


nit: we should shift the try/catch here since this is the only thing that can fail

Done in 715e22c.

seaerchin · 2024-10-11T06:51:23Z

packages/components/src/utils/getParsedDate.ts

+ "yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
+]
+
+export const getParsedDate = (dateString: string) => {


actually the flow i had in mind was reduce(function, new Date()

then in our reducer function:

if (isMatch(date)) { // parse }

and we can put supported date formats in increasing order of priority, so that the code looks cleaner overall. not a big deal though, just that this was not quite what i had in mind

Hmm I a bit confused, I think that's what I'm doing right now? The reducer function is indeed going by priority (from first to last element) and it starts with undefined to differentiate between whether the date string has been parsed or not. If parsed then the type is of Date, otherwise it will remain as undefined.

fix: improve date parsing for collection items

6fe440e

dcshzj requested a review from a team as a code owner October 8, 2024 02:18

dcshzj temporarily deployed to staging October 8, 2024 02:18 — with GitHub Actions Inactive

karrui reviewed Oct 8, 2024

View reviewed changes

packages/components/src/templates/next/layouts/Collection/Collection.stories.tsx Outdated Show resolved Hide resolved

packages/components/src/templates/next/layouts/Collection/utils.ts Outdated Show resolved Hide resolved

ref: switch to use date-fns instead of dayjs

df8e63f

vercel bot had a problem deploying to Preview October 8, 2024 11:07 Failure

dcshzj had a problem deploying to staging October 8, 2024 11:07 — with GitHub Actions Failure

chore: revert to use ISO date format instead

3b67c53

vercel bot had a problem deploying to Preview October 8, 2024 11:10 Failure

dcshzj had a problem deploying to staging October 8, 2024 11:10 — with GitHub Actions Failure

dcshzj requested a review from karrui October 8, 2024 11:10

vercel bot deployed to Preview October 8, 2024 11:32 View deployment

dcshzj temporarily deployed to staging October 8, 2024 11:32 — with GitHub Actions Inactive

chore: fix different versions of date-fns

390ee8e

dcshzj force-pushed the fix/parse-collection-dates branch from 971d95d to 390ee8e Compare October 8, 2024 14:56

vercel bot deployed to Preview October 8, 2024 14:57 View deployment

dcshzj temporarily deployed to staging October 8, 2024 14:57 — with GitHub Actions Inactive

seaerchin reviewed Oct 9, 2024

View reviewed changes

fix: use reducer pattern and add branded type

2daaaf2

vercel bot deployed to Preview October 10, 2024 02:02 View deployment

dcshzj temporarily deployed to staging October 10, 2024 02:02 — with GitHub Actions Inactive

dcshzj requested a review from seaerchin October 10, 2024 02:03

seaerchin approved these changes Oct 11, 2024

View reviewed changes

chore: move try/catch into reducer function

715e22c

vercel bot deployed to Preview October 11, 2024 08:10 View deployment

dcshzj temporarily deployed to staging October 11, 2024 08:10 — with GitHub Actions Inactive

dcshzj enabled auto-merge (squash) October 11, 2024 08:14

dcshzj merged commit 79b9193 into main Oct 11, 2024
18 of 21 checks passed

dcshzj deleted the fix/parse-collection-dates branch October 11, 2024 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve date parsing for collection items #732

fix: improve date parsing for collection items #732

dcshzj commented Oct 8, 2024

vercel bot commented Oct 8, 2024 •

edited

Loading

datadog-opengovsg bot commented Oct 8, 2024 •

edited

Loading

karrui left a comment

seaerchin left a comment

seaerchin Oct 9, 2024

dcshzj Oct 10, 2024

seaerchin Oct 9, 2024

dcshzj Oct 10, 2024

seaerchin left a comment

seaerchin Oct 11, 2024

dcshzj Oct 11, 2024

seaerchin Oct 11, 2024

dcshzj Oct 11, 2024

fix: improve date parsing for collection items #732

fix: improve date parsing for collection items #732

Conversation

dcshzj commented Oct 8, 2024

Problem

Solution

vercel bot commented Oct 8, 2024 • edited Loading

datadog-opengovsg bot commented Oct 8, 2024 • edited Loading

Datadog Report

karrui left a comment

Choose a reason for hiding this comment

seaerchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seaerchin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Oct 8, 2024 •

edited

Loading

datadog-opengovsg bot commented Oct 8, 2024 •

edited

Loading