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 differenceInDays across DST (#533) #1630

Merged
merged 3 commits into from Apr 9, 2020
Merged

fix differenceInDays across DST (#533) #1630

merged 3 commits into from Apr 9, 2020

Conversation

dcousens
Copy link
Contributor

@dcousens dcousens commented Feb 5, 2020

Resolves #533

I'm happy to make changes where necessary, this was simply the simplest solution for the primitives.

@dcousens dcousens changed the title fix differenceInDays across DST (#533) fix differenceInDays across DST (#533) - WIP Feb 5, 2020
@dcousens dcousens changed the title fix differenceInDays across DST (#533) - WIP fix differenceInDays across DST (#533) Feb 5, 2020
@dcousens dcousens requested review from leshakoss and kossnocorp and removed request for leshakoss February 5, 2020 02:14
@dcousens
Copy link
Contributor Author

dcousens commented Feb 5, 2020

For reference, BEFORE THIS PATCH the differenceInDays function returned different results depending on your timezone.

Please note, I have used ISO-8601 timestamps to avoid ambiguity.

const { differenceInDays } = require('date-fns')

const a = new Date('2019-10-05T15:00:00.000Z') // for TZ=Australia/Sydney, this is 1 hour before DST
const b = new Date('2019-10-06T14:00:00.000Z') // for TZ=Australia/Sydney, this is 1 day  after DST
const c = new Date('2019-10-07T14:00:00.000Z') // for TZ=Australia/Sydney, this is 2 days after DST

console.log(differenceInDays(c, b)) 
console.log(differenceInDays(b, a))
console.log(differenceInDays(c, a))

NOTE: These results are from before this patch.

For TZ=Australia/Sydney

1
1
2

For TZ=UTC

1
0
1

@dcousens
Copy link
Contributor Author

dcousens commented Mar 3, 2020

@kossnocorp anything I need to do to help this?

Copy link
Contributor

@dkozickis dkozickis left a comment

Choose a reason for hiding this comment

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

Would be cool if you can add a bit info about the case fixed in this PR to the function description?

@@ -42,15 +40,9 @@ export default function differenceInDays(dirtyDateLeft, dirtyDateRight) {
var dateLeft = toDate(dirtyDateLeft)
var dateRight = toDate(dirtyDateRight)

var sign = compareAsc(dateLeft, dateRight)
var difference = Math.abs(differenceInCalendarDays(dateLeft, dateRight))
var result = (dateLeft - dateRight) / 86400000
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move 86400000 into a var MILLISECONDS_IN_DAY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dkozickis moved

@dcousens
Copy link
Contributor Author

dcousens commented Mar 30, 2020

Would be cool if you can add a bit info about the case fixed in this PR to the function description?

What about

This function returns the difference in days as an integer whole number of 24 hour periods between two timestamps,  and thereby ignores DST changes.

@dkozickis
Copy link
Contributor

dkozickis commented Mar 31, 2020

@dcousens sounds logical to me

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.

Incorrect result across DST in differenceInDays
3 participants