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

s/zone/offset #1779

Closed
icambron opened this issue Jul 17, 2014 · 30 comments
Closed

s/zone/offset #1779

icambron opened this issue Jul 17, 2014 · 30 comments
Milestone

Comments

@icambron
Copy link
Member

While we're deprecating and renaming things, maybe it's time to be consistent with our use of "zone" and "offset"? I'm kind of tired of explaining that moment.zone() isn't quite what it says on the tin, and I'm almost sure @mj1856 is too. What do you think, @ichernev?

@ichernev
Copy link
Contributor

To be honest I don't know what is the correct definition of zone. It might be an offset, or a complete ruleset of offsets. When you say EET (Eastern European Time), that is considered a timezone (http://www.timeanddate.com/library/abbreviations/timezones/eu/eet.html), but in fact its an offset-zone -- It changes to EEST (Easter Europen Summer Time).

The rules for when to use which is not called a zone (http://www.timeanddate.com/time/change/greece/athens for example). Its more like when to change zones because of DST.

So I'd name them zone and DSTRules. What about other libraries?

Also moment-timezone handles just that -- DST for different countries. So it should be named moment-dst if anything :) Well it also has the information about different timezones (like EET), but that is not the main thing.

@icambron
Copy link
Member Author

I agree about the named-offset-vs-DST-rules ambiguity, but Moment doesn't support either of those, just a numerical offset. So it still seems like we should switch to the nice, unambiguous concept of "offset", whether perhaps you could really call it a "zone" or not.

The complication, I guess, is that it is actually also switching the zone DSTRuleset, since it switches to UTC DST rules (i.e. none). For big countries like the US, that actually matters, in the sense that it's reasonable to ask for a relative offset but keep the local DST rules, which in principle Moment proper can support. But whether or not that's actually a good idea, it makes it a lot easier to talk about when we frame it as " Moment supports two sets of rules, UTC and the one you're in. You can set the offset from UTC; we don't yet support offsets from the local rules". That nomenclature gets totally boned when we call the UTC offset method "zone".

The real answer is of course to get rid of DSTs.

@icambron
Copy link
Member Author

In fact, it might be wiser to rename zone to utcOffset to make it clear that it really means "switch to UTC rules and then apply this offset". Later if we want to add localOffset(), we can.

@timrwood
Copy link
Member

+1 for switching to offset.

@ichernev
Copy link
Contributor

So right now there are 3 functions for dealing with these. utc (switches to UTC zone), local (switches to local zone) and zone (switches to a fixed utc offset, 0 is the same as utc(), but that is an implementation detail).

So renaming zone to utcOffset or even fixedUTCOffset is good. Also we need getters, once and for all (not tell people to check _isUTC ... insanity). isLocal, isUTC, isUTCOffset, where we should decide how to handle utcOffset == 0 (maybe depending on how you set it).

@mattjohnsonpint
Copy link
Contributor

Yeah - I've probably been one of the more vocal ones on the interwebs about "Time Zone != Offset".... :)

From a technical perspective, I think the term "time zone" is a bit ambiguous. It could be used to describe any of:

  • Pacific Time (PT)
  • Pacific Standard Time or Pacific Daylight Time (PST / PDT)
  • UTC-8 or UTC-7
  • "America/Los_Angeles"

From a practical perspective though, I've been encouraging others to choose their words more carefully. I probably get this from DDD practices, where ubiquitous language is a key concept.

IMHO, the term "time zone" should be used for describing the entire geographic area, such that they apply without regard to any particular point in time. So "Pacific Time" or "PT" are appropriately called "time zones". So might "America/Los_Angeles" be called a "time zone", but also might be more specifically called a "time zone identifier".

I try to always call values like UTC-8 either an "offset", or a "time zone offset", and call out that a single "time zone" may have one or more "offsets" in their annual usage, or in their history.

Values like "Pacific Standard Time" (PST) are a bit trickier from a naming perspective. I think the best term I've heard for these is "time zone segment".

I do agree though - if you're making breaking changes, rename zone to offset. It's more accurate.

@icambron
Copy link
Member Author

Getting OT, but the terminology I prefer is that PST is a "named offset"; i.e. it's just an alias for UTC-8 with a somewhat useful name that also implies that you're in PT. I agree that PT is a zone for the same reason you do, but of course it's a) an ambiguous name and b) not amenable to change (what do we call it when SF decides to have its own time zone?). America/Los_Angeles as "time zone identifier" seems fine, but I also don't mind just calling it a zone, since it's the unambiguous name for a set of rules.

Edit: another way to put that is that PT is a "common name" for America/Lost_Angeles.

@timrwood
Copy link
Member

This would also be the perfect time to fix this issue. moment/moment-timezone#56 (comment)

Because .offset will be a new method, anyone still using .zone can get a deprecation message noting that the new method and that the sign would now be consistent between passing a string or a number.

@mattjohnsonpint
Copy link
Contributor

Agreed about #56.

@ichernev
Copy link
Contributor

OK, so how about:

  • do not touch the existing zone behavior, just make sure its deprecated :)
  • introduce utcOffset getter and setter. It would not invert as zone used to. It should also handle minute, hour and string offsets (utcOffset(1) === utcOffset(60) === utcOffset("+1:00"))
  • introduce isLocal, isUtc and isUtcOffset. isUtc is a shorthand for isUtcOffset() && utcOffset() === 0.
  • [later] if we put named-utc-offset information in moment (to handle Date.toString() parsing), we can support utcOffset("PDT"), or even have moment.parseUtcOffset() that just takes offset input and canonicalize it (to minutes).

Note: Not sure if we should use UTC of Utc, but the existing methods are called utc, not UTC, so it makes sense to capitalize Utc ... ugh.

@timrwood
Copy link
Member

@ichernev, that all looks good to me.

We may want to fn.isUTC = fn.isUtc for developer friendliness.

EST as a named alias for -04:00 might be a little trickier as Australia also has an Eastern Standard Time and Eastern Saving Time.

@ichernev
Copy link
Contributor

According to this there is no EST in Australia.

@timrwood
Copy link
Member

See moment/moment-timezone#108 and eggert/tz@62df86e#diff-7a413a97538e4a2a88ec6c08dd067830R869.

They are changing, but that's just one example of a name not being unique to a offset. I haven't checked other named offsets, but I'm guessing it happens in other places as well.

@icambron
Copy link
Member Author

I agree with @timrwood - I don't think we can depend on these being unique. See e.g. BST and BST. There are a bunch CSTs (the US's, Cuba's, China's); just scan this list. It's not even clear to me that there's a real governing standard for this stuff.

@ichernev
Copy link
Contributor

Close in favor of #2074

KATT added a commit to KATT/node-cron that referenced this issue Jan 20, 2015
get rid of deprecation warning `Deprecation warning: moment().zone is deprecated, use moment().utcOffset instead. moment/moment#1779
KATT added a commit to KATT/node-cron that referenced this issue Jan 20, 2015
get rid of deprecation warning "Deprecation warning: moment().zone is deprecated, use moment().utcOffset instead. moment/moment#1779"
@RumataEstor
Copy link

Now we have moment().utcOffset() == - moment.tz.zone( <your local zone id> ).offset(moment()).

YidingW pushed a commit to YidingW/bootstrap-daterangepicker that referenced this issue Feb 5, 2015
Update moment.js to the latest;
According to moment/moment#1779, change zone()
to utcOffset()
marqu3s added a commit to marqu3s/fullcalendar that referenced this issue Jun 12, 2015
Remove Deprecation warning: moment().zone is deprecated, use moment().utcOffset instead.
moment/moment#1779
@samholmes
Copy link

Deprecation warning: moment().zone is deprecated, use moment().utcOffset instead. #1779

This is the message I get when running my app. I don't use the zone method at all in my app, so why am I getting a warning about it?

@mattjohnsonpint
Copy link
Contributor

@samholmes - hard to say without seeing an example. Perhaps you have some other dependency that uses it? If you can post a jsFiddle or similar that reproduces the message, I'd be happy to investigate. Otherwise, dive in to your browser's debugging tools and check the stack trace to see what's making the call to that function.

@samholmes
Copy link

$ npm list moment
SECRET@0.0.0 /Users/holmes/Code/SECRET/app
├─┬ cron@1.0.6
│ └─┬ moment-timezone@0.2.4
│   └── moment@2.9.0 
└── moment@2.9.0 

Is this a 2.9.0 problem?

@mattjohnsonpint
Copy link
Contributor

@samholmes - This looks like the same as moment/moment-timezone/issues/228. I see you're using moment-timezone 0.2.4. I can also see that cron 1.0.6 took a hard dependency on moment-timezone version 0.2.4, but in the current version it asks for "~0.3.0" - which I understand to mean >= 0.3.0 and < 1.0.0.

Therefore, updating to the latest cron and moment versions should fix the issue.

@robertdumitrescu
Copy link

I changed the the zone to utcOffset in my implementation but I still get the deprecation error. What can I do to avoid that. I made sure that aren't any .zone() calls in my implementatioin.

@mattjohnsonpint
Copy link
Contributor

@robertdumitrescu search your code. Or put it somewhere we can search and give a URL. Not sure what else I could suggest.

patrickm68 added a commit to patrickm68/node-cron-types that referenced this issue Sep 14, 2023
get rid of deprecation warning "Deprecation warning: moment().zone is deprecated, use moment().utcOffset instead. moment/moment#1779"
patrickm68 added a commit to patrickm68/node-cron-types that referenced this issue Sep 14, 2023
`cron` was using `.tz` which was recently deprecated in moment/moment#1779.
This commit fixes the deprecation notice that `moment` was throwing.
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

10 participants