deprecate isDSTShifted #3160

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@maggiepint
Member
maggiepint commented Apr 30, 2016 edited

Not sure if the group will agree on this one, but I think we should just deprecate isDSTShifted.

It doesn't work for moments that have been mutated (#2942), it doesn't work with moments using moment timezone (moment/moment-timezone#131), because it relies on browser behavior, it can't be unit tested, and it's confusing: http://stackoverflow.com/questions/36939900/moment-timezone-utc-offset-difference

IIRC @mj1856 and I have both looked at this function and tried to come up with a way to fix it. I think we both concluded that due to issues with overflow, it wasn't really feasible. Or maybe Matt looked at something else related to DST - I'm not sure.

It's possible there's a way to fix it, I'm open to suggestion. But if nobody is going to fix it, and it doesn't work, it doesn't make sense to keep it.

I know I added a link to a webpage that doesn't exist in that deprecation warning. If we decide to merge this I'll update docs.

@ichernev
Contributor

I think it was added for newly created moment objects. I'm pretty sure moment-timezone could provide the same information.

So if it could be fixed for newly made moments for both regular and moment timezone I prefer to keep it, with proper docs. If not we can deprecate if of course.

@icambron
Member
icambron commented May 5, 2016 edited

Definitely, when I wrote this function, I had in mind the parsing case. I think that I thought that because the .add() type stuff did a good job on this stuff internally, that we didn't need to think about mutations. I probably just failed to consider .hour(2) type mutations that would throws this off.

This is the kind of thing that makes me want immutability. That said, Moment is definitely mutable and if we can't make it work for mutated moments, I'm +1 on removing it. I think the it-works-except-when-you-do-these-otherwise-supported-things thing is too subtle and documenting it doesn't really fix the issue.

I do think this is fixable, though! Specifically, @timrwood's TZ interface changes provide the answer because _d handles the overflows but not the DSTs. Specifically, if _d has a different hour than its local version (i.e. new Date(_d.getYears(), _d.getMonths(), ....), a transformation that happens anyway to compute the offset) then it must be in a DST shift. That's really just a cleaner version of the array comparison stuff we're doing now, but it handles mutation just fine. I'm not sure how that applies to moment-timezone, but it seems like it would work the same way.

@mj1856
Member
mj1856 commented May 10, 2016

Yeah, agreed. It's too easily confused with isDST. In fact, the deprecation message should probably state "you probably meant isDST" or something to that end.

The idea is nice, and is similar in concept to TimeZoneInfo.IsAmbiguousTime in .NET (and it's cousin IsInvalidTime). However, those only work because DateTime can represent local time in it's native state. Since a moment object (like the JS Date object) is always mapped to a specific timestamp on the instantaneous timeline, there's no real representation of local time occurring, and thus detecting invalid and ambiguous time doesn't work.

@mj1856 mj1856 added Deprecate todo and removed todo labels May 10, 2016
@mj1856
Member
mj1856 commented May 10, 2016

Actually, the current PR looks fine to me, assuming the guides page matches the URL listed.

@ichernev
Contributor

Merged in 3227fff

@ichernev ichernev closed this Jun 14, 2016
@mj1856 mj1856 added this to the 2.14.0 milestone Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment