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

Creating a moment-tz after the DST missing hour, can add 1 hour. #90

Closed
stevehof opened this issue Jun 10, 2014 · 6 comments
Closed

Creating a moment-tz after the DST missing hour, can add 1 hour. #90

stevehof opened this issue Jun 10, 2014 · 6 comments

Comments

@stevehof
Copy link

Hi

I've found that if the a datetime passed into the moment.tz function will add 1 hour. Ive been able to write a test to help debug, here it is:

t.equal(moment.tz("2014-03-09 03:45:00","America/Chicago").format(),"2014-03-09T03:45:00-05:00", "2014-03-09T03:45:00 in America/Chicago should be 2014-03-09T03:45:00-05:00");

Some investigation showed me that at first it subtracts the timezone data and then adds it back on. However when it subtracts it doesn't update the Offset and so isn't aware it crossed a missing hour, and then when it adds the data back it is aware it crossed the missing hour, and adds it on.

@timrwood
Copy link
Member

Yeah, it is a known issue unfortunately. See #27.

Prior to the major refactor/rewrite in #82, I spent 16+ hours working on it with no luck. However, I think the new architecture will make it much easier to solve, I just haven't had the time to tackle it yet.

@stevehof
Copy link
Author

Ah thanks couldn't find a similar ticket. I can sink some time in to it if you'd like. Is the new arch fairly complete on the development branch?

@timrwood
Copy link
Member

Yeah, architecture should be pretty solid on the develop branch. You may want to grab the failing tests from #27.

The way I was thinking of implementing this was something along these lines.

// pseudocode
function parse (args..., zoneName) {
  var output = moment.utc(...args); // try to parse into utc
  var zone = getZone(zoneName);

  // We only need to add the offset if `args` is an array
  // or a string without an offset.
  // You can use the `_i` property on the output moment to check this.
  if (needsOffset(output)) {
    output.add('m', zone.parse(+output));
  }

  output.tz(zoneName);
  return output;
}

zone.parse should take a timestamp and loop until untils[i] + (offsets[i] * 60000) is no longer greater than the timestamp. Something kinda like zone._index.

That's the general route that I was going to take, but I may have gotten something backwards, so take it with a grain of salt.

@stevehof
Copy link
Author

So i looked at it a different way. It 's related to how it shifts the object around, and forgetting to skip the missing hour when it subtracts the tz data in moment.js:2198 and then when it adds the tz difference back on in moment-timezone.js:214 it takes the missing hour into account and increments the time. Hence the phantom 1 hour.

I've pushed some changes and some tests to a branch on my forked repo. Let me know what you think and i can submit a pull request if you like it.

Not sure if it fixes all the errors in #27, haven't included those tests yet, but it does pass all the tests and the ones i included.

stevehof/moment-timezone@develop...dst_changeover_fixes

@stevehof
Copy link
Author

Whoops, sorry my tests were off slightly, however correct for me... The UTC ambig/missing tests are inaccurate because they're configure using my local tz by default. I'll adjust them so that a utc based moment is created

@timrwood
Copy link
Member

I was able to find some time this weekend and added this in #93.

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

No branches or pull requests

2 participants