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

fix: ensure dates are parsed timezone agnostically #1234

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

Skaiir
Copy link
Contributor

@Skaiir Skaiir commented Aug 14, 2024

Closes #1223

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Aug 14, 2024
@Skaiir Skaiir requested a review from vsgoulart August 14, 2024 07:46
@github-actions github-actions bot temporarily deployed to demo-1223-date-endless-countdo August 14, 2024 07:46 Destroyed
@Skaiir Skaiir force-pushed the 1223-date-endless-countdown branch from db11477 to d089fbd Compare August 14, 2024 09:22
@github-actions github-actions bot temporarily deployed to demo-1223-date-endless-countdo August 14, 2024 09:23 Destroyed
@@ -86,7 +86,7 @@ export function Datetime(props) {

switch (subtype) {
case DATETIME_SUBTYPES.DATE: {
date = new Date(Date.parse(value));
date = new Date(value ? value + 'T00:00' : NaN);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this NaN is already what the initial Date.parse was doing. Basically we want to end up with an invalid date because that's how the implementation was initially done. So I'm just doing this to least divert from the previous behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically having this whole condition isn't necessary, because doing date of null + T00:00 will also return an invalid date, but I just wanted to be more explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand what you mean, the previous code seemed fine.

What do you mean by "least divert from the previous behavior"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, we need to add T00:00 to ensure we normalize.

The date.parse was redundant except when value was null in which case it would convert it to NaN which would then get converted to 'Invalid date' by the Date constructor. Because Date(null) actually is valid and creates todays day.

So I removed the Date.parse because it obfuscated a bit of things, and I know value will always be either null or a valid date string, so this seems like the most clear way of replicating the behavior.

I could also do new Date(Date.parse(value + 'T00:00')). But then I might as well just do new Date(value + 'T00:00')

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to better to add a explicit check for the type and not use this hacky solution of manually appending the time, like so:

typeof value === 'string' ? new Date(Date.parse(value)) : new Date(NaN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this will not work. The whole point of the change is to add the time so that we treat a date like 1996-11-20 as 1996-11-20T00:00+??:?? or whatever time, but we want to treat it as that day in my timezone.

The default behavior to parse 1996-11-20 is to treat it as 1996-11-20T00:00Z which is not the intent of a user, because for negative timezones that would mean the day before.

Copy link
Contributor Author

@Skaiir Skaiir Aug 14, 2024

Choose a reason for hiding this comment

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

So adding T00:00 isn't a hack, it's specifying the behavior. That part I'm sure on. We could also set it to 12:00 it doesn't matter but JS doesn't handle dates by themselves in any kind of way so to make sure the right day is picked, we need to add T00:00 (or any time really).

This below is the behavior for an American timezone.

new Date('2015-01-23')
Date Thu Jan 22 2015 14:00:00 GMT-1000 (Hawaii-Aleutian Standard Time)

new Date(Date.parse('2015-01-23'))
Date Thu Jan 22 2015 14:00:00 GMT-1000 (Hawaii-Aleutian Standard Time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And also, date.parse is redundant in the above example. new Date(Date.parse(validDate))) is the same as new Date(validDate)

Comment on lines 149 to 158
const year = date.getFullYear();
const month = String(date.getMonth() + 1).padStart(2, '0');
const day = String(date.getDate()).padStart(2, '0');
return `${year}-${month}-${day}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use toISOString() and split by T and get the first element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior is different, toISOString converts to UTC which would not extract the right date if the timezone difference is large enough.

@@ -86,7 +86,7 @@ export function Datetime(props) {

switch (subtype) {
case DATETIME_SUBTYPES.DATE: {
date = new Date(Date.parse(value));
date = new Date(value ? value + 'T00:00' : NaN);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I fully understand what you mean, the previous code seemed fine.

What do you mean by "least divert from the previous behavior"?

@Skaiir Skaiir force-pushed the 1223-date-endless-countdown branch from d089fbd to ccd0d7a Compare August 14, 2024 10:35
@github-actions github-actions bot temporarily deployed to demo-1223-date-endless-countdo August 14, 2024 10:35 Destroyed
@Skaiir Skaiir force-pushed the 1223-date-endless-countdown branch from ccd0d7a to fa63f97 Compare August 14, 2024 15:17
@github-actions github-actions bot temporarily deployed to demo-1223-date-endless-countdo August 14, 2024 15:17 Destroyed
@Skaiir Skaiir requested a review from vsgoulart August 14, 2024 15:18
@Skaiir Skaiir merged commit 15b5ba9 into main Aug 14, 2024
11 of 12 checks passed
@Skaiir Skaiir deleted the 1223-date-endless-countdown branch August 14, 2024 15:46
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants