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

Allow offset for timezone when creating internal dates for comparison #342

Closed
andrewplummer opened this Issue Jul 21, 2013 · 33 comments

Comments

Projects
None yet
4 participants
@andrewplummer
Owner

andrewplummer commented Jul 21, 2013

The gist of this one is that dates relative to the current date such as 1 week ago as well as Date.past('4pm') (which creates the current date internally to enforce the past requirement that the method name suggests) are making comparisons to a new Date. When attempting to simulate working in different timezones it's sometimes required to offset the date in question before making comparisons, so allow a global timezone offset that will be used internally if set.

Currently going with Date.SugarTimezoneOffset

This value will be in minutes, same as returned by the native getTimezoneOffset...

---- ASIDE ----

One potential problem that I was also thinking about here was DST (daylight savings time). I was toying with the idea of allowing SugarTimezoneOffset to be a function that could manipulate the newly created date, but I don't think that would address the issue that was raised. As SugarTimezoneOffset will always be an offset for newly created dates, it can be manipulated dynamically (say if you really needed precision, you could set an interval watching for the traversal into a new day and reset SugarTimezoneOffset appropriately).

The issue that was raised was instead if you have Date.create('2 weeks ago'), if that date traverses a DST threshold, then it may in fact not be 2 weeks ago... however this issue is independent of the one above as it is also a "problem" for local dates that don't have an offset at all to begin with. I put "problem" in quotes as I don't know if it actually is an issue as technically a date 2 weeks ago that (for example) shows a 1 hour discrepancy technically is 2 weeks ago by the strict definition of 2 * 7 * 24 * 60 * 60 * 1000, therefore I think it would be incorrect to mess with the date in an attempt to correct it. Instead, the developer should adjust it accordingly as necessary for the given app/context.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 21, 2013

Owner

Another question that just came up... should the offset be to the date itself... or should it be an absolute timezone that the date's timezone offset should be subtracted from?

In other words, if the current locale is PST -0800, then if SugarTimezoneOffset is -8 * 60 = -480, does it subtract 480 minutes from the date, or does it subtract nothing? I'm going with the latter for now, as I think it would much more likely be the case that the minute offset of the intended timezone is known (rather than for some reason knowing the number of minutes you want to subtract from the date) and modifying it to make the former work would be a hassle...

Or to put it another way, if you were working in a client side app and you knew the intended timezone was PST -0800 then you could simply set SugarTimezoneOffset to -480 and everything would just work.

Owner

andrewplummer commented Jul 21, 2013

Another question that just came up... should the offset be to the date itself... or should it be an absolute timezone that the date's timezone offset should be subtracted from?

In other words, if the current locale is PST -0800, then if SugarTimezoneOffset is -8 * 60 = -480, does it subtract 480 minutes from the date, or does it subtract nothing? I'm going with the latter for now, as I think it would much more likely be the case that the minute offset of the intended timezone is known (rather than for some reason knowing the number of minutes you want to subtract from the date) and modifying it to make the former work would be a hassle...

Or to put it another way, if you were working in a client side app and you knew the intended timezone was PST -0800 then you could simply set SugarTimezoneOffset to -480 and everything would just work.

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Jul 21, 2013

I think most callers are going to know offset from UTC in minutes (and be ignorant of local time, at least in the case where this code path is used). Given that, SugarTimezoneOffset == -480 for PST seems reasonable to me.

troy commented Jul 21, 2013

I think most callers are going to know offset from UTC in minutes (and be ignorant of local time, at least in the case where this code path is used). Given that, SugarTimezoneOffset == -480 for PST seems reasonable to me.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 21, 2013

Owner

I have to say that I already don't like how this is starting to seem like it might tread on the toes of the UTC based methods Date.utc.create() etc...

Owner

andrewplummer commented Jul 21, 2013

I have to say that I already don't like how this is starting to seem like it might tread on the toes of the UTC based methods Date.utc.create() etc...

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 21, 2013

Owner

Ok added this... if you pull down the latest build you can play with it!

Owner

andrewplummer commented Jul 21, 2013

Ok added this... if you pull down the latest build you can play with it!

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Jul 21, 2013

Just to play Devil's advocate, does Date.utc.create() put the offset abstraction in the right place? UTC is unique because it's so frequently used, but otherwise it's yet another non-local timezone. In a sense, it should wrap Date.create() with SugarTimezoneOffset set to counteract the local offset.

(I'm sure not arguing for changing Date.utc, but one would not expect a Date.pst or Date.cest to exist, which makes me wonder whether Date.utc is as elegant as it appears.)

troy commented Jul 21, 2013

Just to play Devil's advocate, does Date.utc.create() put the offset abstraction in the right place? UTC is unique because it's so frequently used, but otherwise it's yet another non-local timezone. In a sense, it should wrap Date.create() with SugarTimezoneOffset set to counteract the local offset.

(I'm sure not arguing for changing Date.utc, but one would not expect a Date.pst or Date.cest to exist, which makes me wonder whether Date.utc is as elegant as it appears.)

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 21, 2013

Owner

Date.utc methods do much more than this though. They set an internal flag on the date so that it can be manipulated as UTC from that point forward, return the correct get values, etc.

Owner

andrewplummer commented Jul 21, 2013

Date.utc methods do much more than this though. They set an internal flag on the date so that it can be manipulated as UTC from that point forward, return the correct get values, etc.

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Jul 21, 2013

I totally agree, not to mention that the JS date classes lend themselves to UTC much more than to arbitrary timezones.

Thanks for the new build, too. I'll pull the latest build and give it a whirl.

troy commented Jul 21, 2013

I totally agree, not to mention that the JS date classes lend themselves to UTC much more than to arbitrary timezones.

Thanks for the new build, too. I'll pull the latest build and give it a whirl.

@eric

This comment has been minimized.

Show comment
Hide comment
@eric

eric Jul 23, 2013

One thing that could be helpful is if there were a factory that was used for creating the dates.

Being able to change what Date object that is created (so that a timezone-aware date could be substituted for the standard Date object) would allow someone in the future to solve the timezone problem fully.

eric commented Jul 23, 2013

One thing that could be helpful is if there were a factory that was used for creating the dates.

Being able to change what Date object that is created (so that a timezone-aware date could be substituted for the standard Date object) would allow someone in the future to solve the timezone problem fully.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 23, 2013

Owner

What's a "factory"?

Timezone-aware dates are something that are definitely outside the scope of Sugar. It's a very tricky issue that is best left to a separate lib. I haven't tried yet either, but if I were to attempt it I don't think it should be a subclass of Date either, due to the complex internals, but instead use dates under the hood.

Owner

andrewplummer commented Jul 23, 2013

What's a "factory"?

Timezone-aware dates are something that are definitely outside the scope of Sugar. It's a very tricky issue that is best left to a separate lib. I haven't tried yet either, but if I were to attempt it I don't think it should be a subclass of Date either, due to the complex internals, but instead use dates under the hood.

@eric

This comment has been minimized.

Show comment
Hide comment
@eric

eric Jul 23, 2013

Sorry, I'm letting programming design patterns sneak into my javascript. :)

Basically, I was just saying, if Sugar provided a way that I could provide a function that returned the current data, I could at some later time return a TimezoneJS.Date date instead of a standard one.

If there were a way I could override the implementation of getNewDate() by setting a property somewhere, that would be what I am looking for.

eric commented Jul 23, 2013

Sorry, I'm letting programming design patterns sneak into my javascript. :)

Basically, I was just saying, if Sugar provided a way that I could provide a function that returned the current data, I could at some later time return a TimezoneJS.Date date instead of a standard one.

If there were a way I could override the implementation of getNewDate() by setting a property somewhere, that would be what I am looking for.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 23, 2013

Owner

Hm... without looking into it much further yet, I think this would have to be an either/or thing. I don't want SugarTimezoneOffset making the situation more confused or vice-versa.

Owner

andrewplummer commented Jul 23, 2013

Hm... without looking into it much further yet, I think this would have to be an either/or thing. I don't want SugarTimezoneOffset making the situation more confused or vice-versa.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 23, 2013

Owner

I have to say however, this strikes me as a very good idea.

Owner

andrewplummer commented Jul 23, 2013

I have to say however, this strikes me as a very good idea.

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Jul 23, 2013

It's worth noting that using Timezone.JS is a lot more complex than using SugarTimezoneOffset. The biggest reason is that it depends on distributing Olson timezone data files to browsers (see its README). SugarTimezoneOffset solves most cases with far fewer moving parts.

troy commented Jul 23, 2013

It's worth noting that using Timezone.JS is a lot more complex than using SugarTimezoneOffset. The biggest reason is that it depends on distributing Olson timezone data files to browsers (see its README). SugarTimezoneOffset solves most cases with far fewer moving parts.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Jul 23, 2013

Owner

Ya that's definitely true...but I still like the idea of opening up the internal constructor (instead of SugarTimezoneOffset). That way you can do whatever you want. If you want to use timezoneJS you can, or simply create a new date and subtract your own offset as needed... As long as your custom function returns either a date object or something that speaks the same language....

Owner

andrewplummer commented Jul 23, 2013

Ya that's definitely true...but I still like the idea of opening up the internal constructor (instead of SugarTimezoneOffset). That way you can do whatever you want. If you want to use timezoneJS you can, or simply create a new date and subtract your own offset as needed... As long as your custom function returns either a date object or something that speaks the same language....

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 4, 2013

Owner

Having another look at this, it's going to be tricky. For the idea of a "factory" to work, every method that might possibly fetch a new date via getNewDate can no longer use methods that are common to Sugar because they might not be dealing with a Sugar enhanced date any longer. This means that much of the enhanced functionality will now be unavailable internally. I still like the idea, but it's not going to be easy.

In any case I'm still leaning in that direction, with the idea that this ability to override the internal constructor is going to replace SugarTimezoneOffset. Keep in mind that this doesn't mean that you have to do anything as tricky as using timezoneJS (which is what would cause the above problem). You could just as easily simply advance the date directly.

Owner

andrewplummer commented Aug 4, 2013

Having another look at this, it's going to be tricky. For the idea of a "factory" to work, every method that might possibly fetch a new date via getNewDate can no longer use methods that are common to Sugar because they might not be dealing with a Sugar enhanced date any longer. This means that much of the enhanced functionality will now be unavailable internally. I still like the idea, but it's not going to be easy.

In any case I'm still leaning in that direction, with the idea that this ability to override the internal constructor is going to replace SugarTimezoneOffset. Keep in mind that this doesn't mean that you have to do anything as tricky as using timezoneJS (which is what would cause the above problem). You could just as easily simply advance the date directly.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 4, 2013

Owner

btw part of the problem here is that timezoneJS doesn't subclass from Date directly. The problems listed above wouldn't exist if it did, but I guess that's too much to ask.

Owner

andrewplummer commented Aug 4, 2013

btw part of the problem here is that timezoneJS doesn't subclass from Date directly. The problems listed above wouldn't exist if it did, but I guess that's too much to ask.

@eric

This comment has been minimized.

Show comment
Hide comment
@eric

eric Aug 6, 2013

What about "enhancing" the date that is returned from getNewDate() with the Sugar methods?

eric commented Aug 6, 2013

What about "enhancing" the date that is returned from getNewDate() with the Sugar methods?

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 7, 2013

Owner

Hmm... that doesn't really feel right either.. I think the best would be maybe to create a Date subclass facade/wrapper for timezone-js ... I'll probably do this myself if I take this route ... gonna give this a bit more thought though...

Owner

andrewplummer commented Aug 7, 2013

Hmm... that doesn't really feel right either.. I think the best would be maybe to create a Date subclass facade/wrapper for timezone-js ... I'll probably do this myself if I take this route ... gonna give this a bit more thought though...

@eric

This comment has been minimized.

Show comment
Hide comment
@eric

eric Aug 7, 2013

I think in general it would be helpful if you provided a way that I could pass in a Date-like object and have Sugar extend it like it does the normal Date object.

eric commented Aug 7, 2013

I think in general it would be helpful if you provided a way that I could pass in a Date-like object and have Sugar extend it like it does the normal Date object.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 8, 2013

Owner

I don't think I'll be going down that route as Sugar extends prototypes, not objects, but actually there is a way this can be done as Sugar exposes its methods, which you can use to map them on:

function extendObjectWithDateMethods (obj) {
  Object.each(Date.SugarMethods, function (name, m) { 
    obj[name] = m.method;
  });
}
Owner

andrewplummer commented Aug 8, 2013

I don't think I'll be going down that route as Sugar extends prototypes, not objects, but actually there is a way this can be done as Sugar exposes its methods, which you can use to map them on:

function extendObjectWithDateMethods (obj) {
  Object.each(Date.SugarMethods, function (name, m) { 
    obj[name] = m.method;
  });
}
@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 8, 2013

Owner

Actually SugarMethods has both class and instance methods mixed in. There's a way you can discern the two m.instance === true in the above example, but at the moment they're being run through the minifier ... will fix that though.

Owner

andrewplummer commented Aug 8, 2013

Actually SugarMethods has both class and instance methods mixed in. There's a way you can discern the two m.instance === true in the above example, but at the moment they're being run through the minifier ... will fix that though.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 8, 2013

Owner

Ok actually I spoke too soon. There is already a (undocumented) mechanism to do this, and I just changed it so that it can be leveraged to something like what you want. sugarRestore exists to restore control back to Sugar from external scripts that have overwritten Sugar methods. This can actually be used to map Sugar methods onto anything you want like this:

Date.sugarRestore.call(Foo);

There's only one catch: you remember how I said that SugarMethods has class methods and instance methods mixed in? Well part of sugarRestore is to discern between the two, so you can't call it on an instance itself, but instead the constructor. In other words, the methods won't be mapped onto the object directly but instead the prototype of whatever class you're passing in (again, better practice this way).

The one caveat is that Sugar methods also call native methods, so in the above example, Foo would ALSO have to implement the native Date interface in order to work properly (otherwise calls to Sugar methods mapped onto Foo's prototype may fail)...

FORTUNATELY guess what does implement the native Date interface?? timezonejs!
This means that creating a subclass of timezone's interface and mapping Sugar methods on should (I still have to test this) now be a breeze!

Owner

andrewplummer commented Aug 8, 2013

Ok actually I spoke too soon. There is already a (undocumented) mechanism to do this, and I just changed it so that it can be leveraged to something like what you want. sugarRestore exists to restore control back to Sugar from external scripts that have overwritten Sugar methods. This can actually be used to map Sugar methods onto anything you want like this:

Date.sugarRestore.call(Foo);

There's only one catch: you remember how I said that SugarMethods has class methods and instance methods mixed in? Well part of sugarRestore is to discern between the two, so you can't call it on an instance itself, but instead the constructor. In other words, the methods won't be mapped onto the object directly but instead the prototype of whatever class you're passing in (again, better practice this way).

The one caveat is that Sugar methods also call native methods, so in the above example, Foo would ALSO have to implement the native Date interface in order to work properly (otherwise calls to Sugar methods mapped onto Foo's prototype may fail)...

FORTUNATELY guess what does implement the native Date interface?? timezonejs!
This means that creating a subclass of timezone's interface and mapping Sugar methods on should (I still have to test this) now be a breeze!

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Aug 8, 2013

TimezoneJS is great for those who want to distribute Olson timezone files to end users. For those who don't, did you decide between a custom offset function during instantiation or using SugarTimezoneOffset?

And thanks, too! It's neat to see this happen as a thoughtful discussion. I appreciate the effort.

troy commented Aug 8, 2013

TimezoneJS is great for those who want to distribute Olson timezone files to end users. For those who don't, did you decide between a custom offset function during instantiation or using SugarTimezoneOffset?

And thanks, too! It's neat to see this happen as a thoughtful discussion. I appreciate the effort.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 8, 2013

Owner

Well at this point I'm set on opening up the internal constructor for people who need maximum support with timezoneJS (also going to write a facade and put it in the readme so that people can get up and running with this quickly.. might even integrate timezonejs into the unit tests as well)... now of course many people may not need that level of support and I don't want to step on their toes either. But if they have full manual control I would really rather not introduce ANOTHER artificial timezone metaphor with SugarTimezoneOffset (remember there's already utc in there that makes dates call internal UTC-based methodss so it's already kind of like a timezone offset... it would be hard to know when to use what) -- I'd rather have them just do it themselves something like this:

Date.SugarInternalGetNewDate = function () {
  var d = new Date();
  d.addMinutes(MY_SUPER_COOL_SERVER_TIMEZONE_OFFSET);
  return d;
};

That said SugarInternalGetNewDate really sounds dumb to me so I gotta think about that one.

Owner

andrewplummer commented Aug 8, 2013

Well at this point I'm set on opening up the internal constructor for people who need maximum support with timezoneJS (also going to write a facade and put it in the readme so that people can get up and running with this quickly.. might even integrate timezonejs into the unit tests as well)... now of course many people may not need that level of support and I don't want to step on their toes either. But if they have full manual control I would really rather not introduce ANOTHER artificial timezone metaphor with SugarTimezoneOffset (remember there's already utc in there that makes dates call internal UTC-based methodss so it's already kind of like a timezone offset... it would be hard to know when to use what) -- I'd rather have them just do it themselves something like this:

Date.SugarInternalGetNewDate = function () {
  var d = new Date();
  d.addMinutes(MY_SUPER_COOL_SERVER_TIMEZONE_OFFSET);
  return d;
};

That said SugarInternalGetNewDate really sounds dumb to me so I gotta think about that one.

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Aug 8, 2013

TimeZone.js has some known issues, especially around daylight savings transitions. See here for example.

You might want to consider one of the other implementations. In particular, I think either BigEasy/TimeZone or WallTime-js would be a good match for Sugar.

I might also add that before you head down this path, make sure you understand the nature of time zones. In particular, read the timezone tag wiki on StackOverflow. The "Time Zone != Offset" section and the parts about the IANA database are especially pertinent.

As an example of where it could get tricky, say I provide an offset of -420. By that alone, you don't know if I am in MST or PDT. Even if I tell you it is "MST" - you don't know if I mean Arizona which is permanently on MST, or US Mountain Time which alternates between MST and MDT. And that's just in the US. Worldwide, it is much harder.

mj1856 commented Aug 8, 2013

TimeZone.js has some known issues, especially around daylight savings transitions. See here for example.

You might want to consider one of the other implementations. In particular, I think either BigEasy/TimeZone or WallTime-js would be a good match for Sugar.

I might also add that before you head down this path, make sure you understand the nature of time zones. In particular, read the timezone tag wiki on StackOverflow. The "Time Zone != Offset" section and the parts about the IANA database are especially pertinent.

As an example of where it could get tricky, say I provide an offset of -420. By that alone, you don't know if I am in MST or PDT. Even if I tell you it is "MST" - you don't know if I mean Arizona which is permanently on MST, or US Mountain Time which alternates between MST and MDT. And that's just in the US. Worldwide, it is much harder.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 9, 2013

Owner

TimeZone.js has some known issues, especially around daylight savings transitions.

Ok well that's good to know! For Sugar I'm of course trying to find an agnostic solution that will work with any library. But that's especially good to know for the timezonejs wrapper that was going to be an "extra".

I might also add that before you head down this path, make sure you understand the nature of time zones.

Yes, to be sure timezone traversal in Sugar's date-shifting methods as well as intense unit testing in various OS locales have given me a new perspective on timezones and their intricacies. I totally get the fact that timezones are not the same as offsets and it's one of the reasons why SugarTimezoneOffset doesn't appeal to me as a solution. At this point I'm definitely leaning toward exposing the internal date constructor instead to allow users to have the choice to 1. Not care about timezones. 2. Simply allow offsets to stand in for time zones and not care about the difference. 3. Deal with them themselves (or allow one of the other libraries you mentioned to do it for them).

Owner

andrewplummer commented Aug 9, 2013

TimeZone.js has some known issues, especially around daylight savings transitions.

Ok well that's good to know! For Sugar I'm of course trying to find an agnostic solution that will work with any library. But that's especially good to know for the timezonejs wrapper that was going to be an "extra".

I might also add that before you head down this path, make sure you understand the nature of time zones.

Yes, to be sure timezone traversal in Sugar's date-shifting methods as well as intense unit testing in various OS locales have given me a new perspective on timezones and their intricacies. I totally get the fact that timezones are not the same as offsets and it's one of the reasons why SugarTimezoneOffset doesn't appeal to me as a solution. At this point I'm definitely leaning toward exposing the internal date constructor instead to allow users to have the choice to 1. Not care about timezones. 2. Simply allow offsets to stand in for time zones and not care about the difference. 3. Deal with them themselves (or allow one of the other libraries you mentioned to do it for them).

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Aug 9, 2013

As far as I can see, there's no reason why you shouldn't be able to use Sugar dates with any of the timezone libraries already. Admittedly, I need to dive deeper into Sugar to verify this. I've been wrapped up in moment.js lately. :)

I can see that Sugar has more comprehensive natural language parsing than other libs (like moment). That is probably where you would need to focus on getting timezones working. Something like these:

Date.create('the last day of March 2013 in America/New_York')

Date.create('3:00 PM in America/New_York as Europe/London')

or if you wanted to dig into the tzdb yourself, perhaps:

Date.create('10:00 AM today in Los Angeles, in London time')

That might be a stretch though. ;-)

If it's not going to involve the parser, then I don't see why you need to do anything special at all.

mj1856 commented Aug 9, 2013

As far as I can see, there's no reason why you shouldn't be able to use Sugar dates with any of the timezone libraries already. Admittedly, I need to dive deeper into Sugar to verify this. I've been wrapped up in moment.js lately. :)

I can see that Sugar has more comprehensive natural language parsing than other libs (like moment). That is probably where you would need to focus on getting timezones working. Something like these:

Date.create('the last day of March 2013 in America/New_York')

Date.create('3:00 PM in America/New_York as Europe/London')

or if you wanted to dig into the tzdb yourself, perhaps:

Date.create('10:00 AM today in Los Angeles, in London time')

That might be a stretch though. ;-)

If it's not going to involve the parser, then I don't see why you need to do anything special at all.

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 9, 2013

Owner

If it's not going to involve the parser, then I don't see why you need to do anything special at all.

So, the original ticket was about Sugar's internally created dates. Right now it is now doing a simple new Date() but there are various reasons that that date needs to be pre-processed to handle timezone offsets, etc. One example of why this is an issue is that if Sugar is comparing a date to "one week ago", then "one week ago" cannot be relative to the local time if you're trying to keep everything in a different timezone (or pseudo-timezone offset). This is why the internal constructor needs to be opened up.

I can see that Sugar has more comprehensive natural language parsing than other libs (like moment). That is probably where you would need to focus on getting timezones working.

No, that's definitely outside the scope of this ticket, and probably also of Sugar itself. Instead it would be the domain of the lib in question, although it would be nice to have a bridge between the two, but this would easily be accomplished in a different way such as:

SomeDateSubclassWIthTimezone.create('the last day of march 2013').in('America/New_York')

... or something similar

Owner

andrewplummer commented Aug 9, 2013

If it's not going to involve the parser, then I don't see why you need to do anything special at all.

So, the original ticket was about Sugar's internally created dates. Right now it is now doing a simple new Date() but there are various reasons that that date needs to be pre-processed to handle timezone offsets, etc. One example of why this is an issue is that if Sugar is comparing a date to "one week ago", then "one week ago" cannot be relative to the local time if you're trying to keep everything in a different timezone (or pseudo-timezone offset). This is why the internal constructor needs to be opened up.

I can see that Sugar has more comprehensive natural language parsing than other libs (like moment). That is probably where you would need to focus on getting timezones working.

No, that's definitely outside the scope of this ticket, and probably also of Sugar itself. Instead it would be the domain of the lib in question, although it would be nice to have a bridge between the two, but this would easily be accomplished in a different way such as:

SomeDateSubclassWIthTimezone.create('the last day of march 2013').in('America/New_York')

... or something similar

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Aug 9, 2013

Ok, I think I know what you are talking about now. Perhaps your are thinking in similar lines to the .zone() setter in moment.js. It's misnamed in my opinion, but basically, when you pass a specific offset in the zone setter, such as .zone(-420) or .zone('-07:00'), that is persisted in a separate internal variable. Then every time you use one of the output functions like .format(), it starts with the UTC timestamp of the internal Date, and applies the zone you specified, thus bypassing the local time zone. Sugar could do something similar, which you would see the effects when using .format(), .short(), .long(), etc.

With regards to comparing to "one week ago" - that gets tricky. The naive implementation would subtract (7 * 24 * 60 * 60 * 1000) from the start time milliseconds and compare against that. But that is assuming that a week is exactly 7 standard days. Of course in reality you might cross a DST transition, making one of those days either 23, 23.5, 24.5 or 25 hours in length. So it's a difficult problem to solve without knowing where the transitions are.

One approach is to work strictly from the calendar dates. So you would subtract 7 whole days from the local date, not by calculating milliseconds, but by manipulating the day, month, year manually. Any DST transition would be absorbed by that logic. Of course the problem is that you are still using the DST rules from the local time zone - not the target time zone. But you can't do anything about that without a timezone database.

BTW - just offering advice here. I'm not super invested in the issue either way. I hope it's helpful for you.

mj1856 commented Aug 9, 2013

Ok, I think I know what you are talking about now. Perhaps your are thinking in similar lines to the .zone() setter in moment.js. It's misnamed in my opinion, but basically, when you pass a specific offset in the zone setter, such as .zone(-420) or .zone('-07:00'), that is persisted in a separate internal variable. Then every time you use one of the output functions like .format(), it starts with the UTC timestamp of the internal Date, and applies the zone you specified, thus bypassing the local time zone. Sugar could do something similar, which you would see the effects when using .format(), .short(), .long(), etc.

With regards to comparing to "one week ago" - that gets tricky. The naive implementation would subtract (7 * 24 * 60 * 60 * 1000) from the start time milliseconds and compare against that. But that is assuming that a week is exactly 7 standard days. Of course in reality you might cross a DST transition, making one of those days either 23, 23.5, 24.5 or 25 hours in length. So it's a difficult problem to solve without knowing where the transitions are.

One approach is to work strictly from the calendar dates. So you would subtract 7 whole days from the local date, not by calculating milliseconds, but by manipulating the day, month, year manually. Any DST transition would be absorbed by that logic. Of course the problem is that you are still using the DST rules from the local time zone - not the target time zone. But you can't do anything about that without a timezone database.

BTW - just offering advice here. I'm not super invested in the issue either way. I hope it's helpful for you.

@mj1856

This comment has been minimized.

Show comment
Hide comment
@mj1856

mj1856 Aug 9, 2013

Just ran a few simple tests, moving by days across DST boundaries with your advance and rewind methods. They appear to already be doing what I described. :)

mj1856 commented Aug 9, 2013

Just ran a few simple tests, moving by days across DST boundaries with your advance and rewind methods. They appear to already be doing what I described. :)

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 9, 2013

Owner

Sugar could do something similar, which you would see the effects when using .format(), .short(), .long()

Those formatting methods don't use timezones anyway, except for format, which allows an offset token and does apply the UTC override when set.

Owner

andrewplummer commented Aug 9, 2013

Sugar could do something similar, which you would see the effects when using .format(), .short(), .long()

Those formatting methods don't use timezones anyway, except for format, which allows an offset token and does apply the UTC override when set.

andrewplummer added a commit that referenced this issue Aug 10, 2013

going with Date.SugarNewDate instead of Date.SugarTimezoneOffset for
handling customizing internally created dates. Changed updateDate to
better avoid infinite recursion #342

andrewplummer added a commit that referenced this issue Aug 10, 2013

@andrewplummer

This comment has been minimized.

Show comment
Hide comment
@andrewplummer

andrewplummer Aug 10, 2013

Owner

Ok, added a check for Date.SugarNewDate. This function will be used to return a new date internally if it exists.

Additionally I wrote a shim that will allow timezoneJS objects to be used in place of native date objects. It is available in lib/extras and I'll put some stuff in the readme about it. It subclasses timezoneJS.Date and mixes in Sugar methods.

Also note that I take back what I said about "the proper way would be for them to subclass Date"... This isn't possible in JS and I always forget that it's not. Here's an example of why:

function DateWithTimezone () {};
DateWithTimezone.prototype = new Date();
new DateWithTimezone().getTime();  // --> Errors with "not a Date Object"

So JS natives internally perform checks that defeat the purpose of duck typing, making subclassing impossible... (thanks a lot, JS).

At any rate, the method of instead mixing in methods seems to work great. In fact, I even hooked up the Sugar date unit tests to ALL use timezoneJS objects instead of native dates, and nearly all of them passed. Of the failures, I've accounted for some errors with the timing of loading in the Olson files via timezoneJS's loading mechanism.

In addition to that it appears that the Sugar unit tests exposed some problems with the timezoneJS handling of negative timestamps:

new timezoneJS.Date('8/25/50', 'Asia/Tokyo').getTime() !== new Date('8/25/50').getTime()

(note this is my internal timezone) ... so I can account for failures there.

As @mj1856 implied, possible bugs aside, after working with timezoneJS, it doesn't really leave me with a very good impression, FYI. Among oddities is internally requiring an ajax transport layer to load the timezone files. I actually got a "jQuery not defined" error when trying to load too early... yeesh. There must be a better way.

So that said I may have a look at making other shims for some others. (walltime looks particularly good despite the name)

Owner

andrewplummer commented Aug 10, 2013

Ok, added a check for Date.SugarNewDate. This function will be used to return a new date internally if it exists.

Additionally I wrote a shim that will allow timezoneJS objects to be used in place of native date objects. It is available in lib/extras and I'll put some stuff in the readme about it. It subclasses timezoneJS.Date and mixes in Sugar methods.

Also note that I take back what I said about "the proper way would be for them to subclass Date"... This isn't possible in JS and I always forget that it's not. Here's an example of why:

function DateWithTimezone () {};
DateWithTimezone.prototype = new Date();
new DateWithTimezone().getTime();  // --> Errors with "not a Date Object"

So JS natives internally perform checks that defeat the purpose of duck typing, making subclassing impossible... (thanks a lot, JS).

At any rate, the method of instead mixing in methods seems to work great. In fact, I even hooked up the Sugar date unit tests to ALL use timezoneJS objects instead of native dates, and nearly all of them passed. Of the failures, I've accounted for some errors with the timing of loading in the Olson files via timezoneJS's loading mechanism.

In addition to that it appears that the Sugar unit tests exposed some problems with the timezoneJS handling of negative timestamps:

new timezoneJS.Date('8/25/50', 'Asia/Tokyo').getTime() !== new Date('8/25/50').getTime()

(note this is my internal timezone) ... so I can account for failures there.

As @mj1856 implied, possible bugs aside, after working with timezoneJS, it doesn't really leave me with a very good impression, FYI. Among oddities is internally requiring an ajax transport layer to load the timezone files. I actually got a "jQuery not defined" error when trying to load too early... yeesh. There must be a better way.

So that said I may have a look at making other shims for some others. (walltime looks particularly good despite the name)

@troy

This comment has been minimized.

Show comment
Hide comment
@troy

troy Aug 11, 2013

This seems like a great conclusion, @andrewplummer.

I have a similar opinion of TimezoneJS, not as a fault as much as I think that it's overkill for most situations. It was written by a company that markets themselves as Twitter for businesses, and I can see them needing more thorough timezones than most.

Thank you for your work on this.

troy commented Aug 11, 2013

This seems like a great conclusion, @andrewplummer.

I have a similar opinion of TimezoneJS, not as a fault as much as I think that it's overkill for most situations. It was written by a company that markets themselves as Twitter for businesses, and I can see them needing more thorough timezones than most.

Thank you for your work on this.

andrewplummer added a commit that referenced this issue Aug 4, 2015

going with Date.SugarNewDate instead of Date.SugarTimezoneOffset for
handling customizing internally created dates. Changed updateDate to
better avoid infinite recursion #342

andrewplummer added a commit that referenced this issue Aug 4, 2015

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