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(#391): timezones #404

Closed
wants to merge 1 commit into from
Closed

Conversation

spurreiter
Copy link

This fixes the issue described in #391

rrule time base is UTC whereas Luxon requires dates in local format to
calculate the correct time offset.

New tests where introduced to prove correct behavior in different
system timezones. Run those with npm run test:tz

rrule time base is UTC whereas Luxon requires dates in local format to
calculate the correct time offset.

New tests where introduced to prove correct behaviour in different
system timezones. Run those with `npm run test:tz`
@greenreign
Copy link

Any chance to get this merged in? We've been using the branch and it seemed to fix all our timezone issues.

@egarkavy
Copy link

@greenreign May be still actual. I also needed that fix so I published this package to NPM registry with this PR merged
https://www.npmjs.com/package/rrule-community-edition

Copy link
Collaborator

@davidgoli davidgoli left a comment

Choose a reason for hiding this comment

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

I appreciate your PR & the thoughtful work that went into this, but unfortunately, it seems to stem from a fundamental misunderstanding of the API. Please see my comment in the linked issue. Thanks!

"test:tz": "npm run test:tz1 && npm run test:tz2 && npm run test:tz3",
"test:tz1": "TZ=Asia/Tokyo npm t",
"test:tz2": "TZ=Europe/Madrid npm t",
"test:tz3": "TZ=America/Chicago npm t"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This project uses yarn, please change these to reference yarn instead of npm

@@ -37,8 +37,9 @@ describe('rezonedDate', () => {
const currentLocalDate = DateTime.local(2000, 2, 6, 1, 0, 0)
setMockDate(currentLocalDate.toJSDate())

const d = DateTime.fromISO('20101005T110000').toJSDate()
const d = DateTime.fromISO('20101005T110000Z').toJSDate()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The purpose of this test is not to test the ability to use UTC (which already works). It's to test conversion from local time into the expected timezone.

@davidgoli davidgoli closed this Oct 31, 2021
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.

None yet

5 participants