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

Call updateOffset when creating moment #2054

Closed
wants to merge 2 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@elad
Contributor

elad commented Nov 17, 2014

Goes along with moment-timezone/pull/152.

@ichernev

This comment has been minimized.

Contributor

ichernev commented Nov 29, 2014

OK, that looks pretty good, I'll merge it in next release, we'll sync with moment-timezone.

@ichernev ichernev added this to the 2.9 milestone Nov 29, 2014

timrwood added a commit to moment/moment-timezone that referenced this pull request Dec 19, 2014

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 24, 2014

Merged in e2e4f4f

@ichernev ichernev closed this Dec 24, 2014

@elad

This comment has been minimized.

Contributor

elad commented Dec 24, 2014

Thanks! :) Can moment-timezone#15 be closed?

@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 26, 2014

This creates infinite recursion when the updateOffset function has to create or clone a moment object. I hit this in tests, so we should think about something else. It might not go out in 2.9 (unless you figure a decent fix very fast). I will think about it after 2.9.

Reverted in 6d3bc5f

@ichernev ichernev reopened this Dec 26, 2014

@elad

This comment has been minimized.

Contributor

elad commented Dec 26, 2014

Well that's unfortunate. Do you have a reproducible test case?

I haven't had a chance to look at the code yet (and in any case it will be easier with a test case), but: Can we fix the infinite recursion by introducing a state variable that a moment is being created and avoid calling updateOffset if it's set?

ichernev added a commit that referenced this pull request Dec 28, 2014

Merge pull request #2054 from elad:develop
Call updateOffset when creating moment

ichernev added a commit that referenced this pull request Dec 28, 2014

Revert "Merge pull request #2054 from elad:develop"
This reverts commit e2e4f4f, reversing
changes made to e9d9a71.

ichernev added a commit that referenced this pull request Dec 28, 2014

Merge pull request #2054 from elad:develop
Call updateOffset when creating moment
@ichernev

This comment has been minimized.

Contributor

ichernev commented Dec 28, 2014

I made sure (with a global boolean variable) that there would be no infinite loop. The way moment-timezone and moment are tied right now (with updateOffset) -- its too loose to assume that updateOffset sets _z property (it might not), so the only sane thing to do is to prevent the recursion.

What we really need is the notion of a zone (something with DST, a collection of utc offsets) in moment, and something (external) to tell it what is the offset in a zone of a particular UTC time. Then moment should handle the actual zone switching not some random function. We already to tricks in addOrSubtract (https://github.com/moment/moment/blob/ef27605/moment.js#L2492) because we can get recursive calls just from normal operation.

I'm actually working on https://github.com/ichernev/redate which would handle just that (proper zone switching). I'll see how it will play with moment (if at all) in the future.

Merged (with fixes) in ef27605

@ichernev ichernev closed this Dec 28, 2014

@elad

This comment has been minimized.

Contributor

elad commented Dec 28, 2014

Thanks for taking the time to fix it!

The _changeInProgress bit you pointed out is exactly what I had in mind. I agree that if existing code already has this type of issue then some restructuring is necessary to ensure it doesn't happen.

In any case, I'm very happy to hear the issue has been addressed and am looking forward to 2.9. :)

timmfin added a commit to HubSpot/moment-timezone that referenced this pull request Apr 23, 2015

Merge tag '0.3.1' into merge-with-0.3.x
0.3.1

* tag '0.3.1': (31 commits)
  Version 0.3.1
  time zone data: 2015a
  Remove references to tests/moment-timezone-utils/*. #174
  [Kashif] In meta data, filter coutries which don't have any timezones for them. eg: 'Bovet island'
  Add links to the changelog.
  Version 0.3.0.
  Add a hint when using an older version of moment with setDefault()
  Renable default zone tests now that moment@2.9.0 is released.
  Don't add an offset to timezone moments that were created from cloned moments.
  Remove needsOffset tests for cloned moments.
  Commenting out default tests until moment/moment#2054 lands
  Failing tests for #135
  Add temporary wrapper for moment#utcOffset until it lands in moment core
  Add tests for isSame() #127
  Fix for error "stderr: maxBuffer exceeded", while running `data-zdump` grunt task.
  Rebuild metadata for all 2014 releases according to the new schema
  Updates to the grunt task `data:meta`: 1. Pick "zone1970.tab" as its input source if available, defaults to zone.tab. (until 'tzdata2014f', only zone.tab was available, but now it's the deprecated version) 2. Output JSON of this task now has two hashes: "countries" and "zones" to facilitate consumers to deal much better with showing timezone choices. 3. Updated contribution docs to add info about "data-meta" grunt task.
  Update tests to use utcOffset instead of zone
  Prefer moment.fn.utcOffset to moment.fn.zone
  Version 0.2.5
  ...

Conflicts:
	package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment