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

Behavior of invalid local time adjustment #99

Closed
mattjohnsonpint opened this issue Jun 25, 2014 · 12 comments
Closed

Behavior of invalid local time adjustment #99

mattjohnsonpint opened this issue Jun 25, 2014 · 12 comments
Milestone

Comments

@mattjohnsonpint
Copy link
Contributor

In the new moment-timezone docs, there's a paragraph under Parsing Ambiguities which states:

The result is that any time between 1:59:59 and 3:00:00 never actually happened. Moment Timezone accounts for this. If you try to parse a time that never existed, it will round down an hour.

In my experience - this is rarely the right thing to do. It would be best if the developer could specify the behavior they wanted, but if you must assume a behavior then you should have a common use case in mind.

The most common case I can think of is when one schedules a local-time recurrence pattern like "Daily at 2:00 AM Pacific Time". It works every day, except on the day of the spring-forward transition when 2:00 AM doesn't exist. So in this case, would you expect the job to run an hour early at 1:00 AM? Probably not. Most of the time, it's more reasonable to run at 3:00 AM, because that's 1 minute after 1:59 AM on this day.

Another way to think about it is: What if this was a physical clock and the person just forgot to move the time forward? It would run at 2:00 AM as scheduled, which is the same instant as should be 3:00 AM. And yet another way to think about it: 2:00 still falls between 1:59 and 3:00, so running at 1:00 doesn't make much sense.

Regarding the fall-back transition, I think the default of using the earlier instance is the usually desired behavior.

But again, perhaps this could be configurable. Even better, you could let the user define a "resolver" function to handle it whichever way they like. Noda Time does this.

@timrwood
Copy link
Member

Bah, looking back at the original issue I see that we already agreed on that and I didn't update the tests.

I'll write up some tests for this and try to make these configurable as well. Maybe something like the following would work? Default to what is seen below, but allow them to be changed?

moment.tz.moveInvalidForward = true;
moment.tz.moveAmbiguousForward = false;

@timrwood timrwood added this to the 0.2.0 milestone Jun 25, 2014
@timrwood
Copy link
Member

As a side note, I tested this out in a couple browsers to see how they handled it and the results were inconsistent. Chrome rolled back, and Firefox and Safari rolled forward.

new Date(2014, 2, 9, 2).getHours() // 1 = back, 3 = forward

@mattjohnsonpint
Copy link
Contributor Author

Yes - browsers are inconsistent in this regard. Here's a slide from my upcoming Pluralsight course, where I touch on this.

image

@mattjohnsonpint
Copy link
Contributor Author

With regard to the API - that would probably be good enough for most folks. There are some more advanced use cases that might require a function definition instead of just a boolean, but I'm not sure if that's something you want to support or not.

As an example of one of these more advanced scenarios - I was recently working on a system that received data from proprietary time-clock devices. Even though those devices would adjust for DST automatically, they didn't include an offset or DST flag in the data they sent. So during the ambiguous period, I would use the local clock of the server and the known time zone rules about where the device was running in order to make the determination of whether to treat the incoming value as daylight time or standard time. That kind of advanced rule isn't something I would expect to be built-in to any API - but since I was writing this in .Net and using Noda Time, I was able to write my own custom resolver function and plug it in.

I'm thinking that if anyone has custom needs like this, they'll probably be using moment from a node.js app, rather than from the browser.

@mattjohnsonpint
Copy link
Contributor Author

Also, just realized there's an inconsistency with Firefox between what you mentioned and how I built the graph in my slide. I wonder if there's a difference between OS or FF versions. I'll do some more research. I assume you are on the latest FF. On a Mac, right?

@timrwood
Copy link
Member

Yes, FF 30.0 on OSX 10.9.3.

@mattjohnsonpint
Copy link
Contributor Author

Checked FF25 through 30 and all skip forward to 3:00 in the spring. I must have made a mistake when testing last time around. I'll have to update my slides...

What's interesting though, is that in the fall, it does indeed choose the second instance. So that makes FireFox the only one with that particular combination of the two behaviors.

At any rate - the whole point is to get consistency regardless of browser - which we should have since we rely on the tz data and not on the Date object. Right?

@mattjohnsonpint
Copy link
Contributor Author

For the record....

image

@timrwood
Copy link
Member

Yep, this should be consistent across all browsers.

With moment#utc(), moment#zone(val), and moment#tz(val), we always use the Date#getUTCX getters and setters instead of Date#getX so we never rely on DST calculations from the Date object.

@neverfox
Copy link

neverfox commented Jul 5, 2014

@mj1856, I was just about to post an issue for this. I had the same thought when I read the new docs. Thanks for bringing it up.

@timrwood
Copy link
Member

timrwood commented Jul 7, 2014

Fixed and merged in #101.

@timrwood timrwood closed this as completed Jul 7, 2014
@timrwood timrwood modified the milestones: 0.1.1, 0.2.0 Jul 7, 2014
@timrwood
Copy link
Member

This is published as of moment-timezone@0.2.0.

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

No branches or pull requests

3 participants