Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace 000Z suffix with dayjs.utc call #2087

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

henrycatalinismith
Copy link
Collaborator

This is a partial fix for #2079. It addresses only the task status, not the "Invalid Date Invalid Date" further down the page. This microsecond precision thing can be as big of a project as we want it to be, and I've had a couple of doomsday-sized PRs lately, so I'm shooting for a smaller first step here. Happy to follow up with more, and then we can go as deep into this issue as you like.

As per my post on the issue, the correct status for this task is "active" as it has a published date in the past. The problem is that the date has microsecond precision, and so the addition of the .000Z suffix makes it unparsable to dayjs: Both dayjs(published + '.000Z').isBefore(now) and dayjs(published + '.000Z').isAfter(now) return false if published contains microseconds.

Thing is, dayjs does actually support parsing microseconds. So there's no problem with that. It supports UTC too. So my thinking here is we just need to approach this differently. Instead of chucking .000Z on the end to tell dayjs we want our date parsed as UTC, we can replace the dayjs(published) call with dayjs.utc(published), which achieves the desired goal without depending on smuggling our intended timezone in via the string param.

Before After
Screenshot 2024-07-17 at 22-07-52 A second dummy task Screenshot 2024-07-17 at 22-07-21 A second dummy task

Want excruciating detail? You got it! The following sample code runs through dayjs date comparison outcomes with three slightly different input formats to explain why I think dayjs.utc() is the way to go here.

import dayjs from 'dayjs';
import dayjsPluginUTC from 'dayjs-plugin-utc'
dayjs.extend(dayjsPluginUTC)

const published = '2024-05-27T10:16:54.625596'
const now = dayjs('2024-05-27T10:00:00Z')

// In Sweden the first one here returns true and the second returns false. This
// is incorrect. It happens because the lack of a .000Z suffix causes it to be
// parsed as a local time.
console.log(dayjs(published).isBefore(now))
console.log(dayjs(published).isAfter(now))

// Both of these checks return false. This is also incorrect. This happens
// because dayjs is unable to parse '2024-05-27T10:16:54.625596.000Z'.
console.log(dayjs(published + ".000Z").isBefore(now))
console.log(dayjs(published + ".000Z").isAfter(now))

// In Sweden the first one here return false and the second returns true.
// This is the correct behavior..
console.log(dayjs.utc(published).isBefore(now))
console.log(dayjs.utc(published).isAfter(now))

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work investigating this. I'm sorry for the delay in giving feedback. I took a look at this in the summer but didn't fully understand it. I've revisited it now and understand what's going on. I think the solution is fine!

Longer term we are beginning to incorporate proper timezone support into the Zetkin API and frontend, but for now it's correct to assume UTC.

@richardolsson richardolsson merged commit baf6f1c into main Sep 11, 2024
5 checks passed
@richardolsson richardolsson deleted the issue-2079/dayjs.utc branch September 11, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants