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

Implement timezone interface #3134

Closed
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
4 participants
@timrwood
Member

timrwood commented Apr 22, 2016

This is the initial pass at implementing moment/moment-rfcs#1

This introduces an interface for a TimeZone in moment core. Moment would provide 2 classes that implement the TimeZone interface, LocalTimeZone and FixedOffsetTimeZone. Libraries like moment-timezone could provide their own TZDBTimeZone class that implements the TimeZone interface.

Each moment created now has a mandatory time zone. Calls to moment() would have a LocalTimeZone instance, calls to moment.utc() would have a FixedOffsetTimeZone instance, and calls to moment.tz() would have a TZDBTimeZone instance.

To support this, we will no longer be using the local timezone Date.prototype getters and setters. All calls to the underlying date object use the setUTC* and getUTC* methods, and parsing is done with Date.UTC(). We will continue to store the _offset property on a moment to determine what the offset is.

Public API Additions

moment.withTimeZone(timeZone)

This is the public api for moment-timezone and other libraries to construct a moment in a timezone. It returns a function that can be used just like moment() or moment.utc().

var create = moment.withTimeZone(chicagoTimeZone);
var nowInChicago = create();
var decemberInChicago = create("2016 décembre", "YYYY MMMM", "fr");

Private API Removals

moment.updateOffset hook.

This was build as an undocumented api for use by moment-timezone. We are moving the code for manually adjusting the offset into moment, so there is no need for an external library to handle this logic.

moment#_useUTC

This is used by the parser to determine whether to use new Date(y, m, d...) or Date.UTC(y, m, d) when constructing the Date. We are always using Date.UTC(y, m, d), so this is no longer needed.

moment#_isUTC

This was used to switch between calls to Date#getMonth and Date#getUTCMonth, but we are always using Date#getUTCMonth, so this is unneeded.

moment.momentProperties

This added for moment timezone to copy the _z property when cloning a moment, but now that timezones are a core concept for moment, a moment-timezone moment will behave the same as all other moments.

Public API Deprecations

moment#isLocal, moment#isUtc, moment#isUtcOffset

These methods don't really match the semantics of the new timezone interface. They were added because we had internal flags that toggled between different modes. A better interface would be something like moment#timezone() === 'local'.

Open Questions

This adds moment.withTimeZone as partial application for the timezone, should we expand that to format, locale, and strict parsing?

var create = moment.withDefaults({
  zone: chicagoTimeZone,
  locale: "fr",
  format: "YYYY MMMM",
  strict: true
});
var novemberInChicago = create("2016 novembre");
var decemberInChicago = create("2016 décembre");

Should we add a getter/setter for timezones? Should it publicly reference timezones with strings instead of objects?

moment().timezone('local');
moment().timezone('UTC+1');
moment().timezone('America/Los_Angeles');
moment().timezone(); // 'local'
moment.withTimeZone('America/Los_Angeles').timezone(); // 'America/Los_Angeles'
moment.utc().timezone(); // 'UTC'

To support this, we could provide an api to register callbacks to be used to resolve the timezone string.

moment.defineTimezone(function (input) {
  if (input === 'local') {
    return new LocalTimeZone();
  }
});
moment.defineTimezone(function (input) {
  if (matchesFixedOffsetFormat(input)) {
    return new FixedOffsetTimeZone(input);
  }
});
// moment timezone
moment.defineTimezone(function (input) {
  if (zones[input]) {
    return zones[input];
  }
});

The timezone resolver would just loop through all the callbacks, returning the first timezone that matched.

@icambron

This comment has been minimized.

Member

icambron commented on src/lib/timezone/local.js in 3a126c1 Apr 21, 2016

So, I think this interface should be function(year, month, day, hour, minute, second, millisecond). The part that confuses me is where the timestamp being passed in is, like, not actually the timestamp we're computing offset for (it's the timestamp that has the same UTC year/month/etc as the time we're actually using). So I think it's more clear if Moment calls this with:

someTZ.parse(_d.getUTCFullYear(), _d.getUTCMonth(), ...)

and then this thing just does:

return getTimezoneOffset(new Date(year, month, day, hour, minute, second, millisecond))

That also saves you one date creation, which is nice.

This comment has been minimized.

Member

timrwood replied Apr 22, 2016

For the local timezone, it is easier to use the date parts (year, month, day...) to calculate an offset, but for moment timezone, it is easier to use a UTC timestamp. I'll update the RFC and then this PR with a better api for parsing one of the two.

I do agree that this un-offset timestamp is confusing to explain...I'll try to describe it better in the RFC

@maggiepint

This comment has been minimized.

Member

maggiepint commented May 30, 2016

You know what, I'm putting a release tag on this. We can't sit on it forever, it's a positive change, and those who patched private variables on a library can learn.
Let me know if there are any objections. Obviously, Tim has to get moment timezone ready for this change.

@mj1856 mj1856 added this to the 2.15.0 milestone Jul 5, 2016

@mj1856 mj1856 self-assigned this Aug 5, 2016

@mj1856 mj1856 modified the milestones: 3.0, 2.15.0 Aug 12, 2016

@mj1856

This comment has been minimized.

Member

mj1856 commented Aug 12, 2016

If we merge this now, we'll break moment-timezone. So staging for v3 until we can figure out when to do them both together.

@icambron

This comment has been minimized.

Member

icambron commented Mar 5, 2017

See rebased version in #3807.

@icambron icambron closed this Mar 5, 2017

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