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 keeping time through tz to correctly handle being near DST boundaries #872

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mongoose700
Copy link

This is a fix for #871

This change simplifies the process by just using moment.tz(this.toArray(), name), so that it doesn't have to deal with offsets directly. I then also added logic to handle making it use the second occurrence of the time in the time zone if possible, if it was the second occurrence in the original time zone.

I'm not all that certain about the var zone = mom._z || moment.defaultZone || getZone(guess()); check. It seems to match the logic for how a moment with an unspecified time zone behaves, but I haven't been able to confirm it.

@jsf-clabot
Copy link

jsf-clabot commented Jul 13, 2020

CLA assistant check
All committers have signed the CLA.

@mongoose700 mongoose700 marked this pull request as ready for review July 13, 2020 01:06
@mongoose700 mongoose700 force-pushed the tz-keep-time-dst branch 3 times, most recently from a572677 to deb1a65 Compare July 13, 2020 03:22
@ichernev
Copy link
Contributor

@mongoose700 thanks for working on this!

Using toArray() seems like a very sensible thing to do. About adding all the additional code about repeated time, I'm less enthusiastic.

About the guess() being injected in there, I'm even less enthusiastic, need to see how things are expected to work across the board.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants