moment / luxon

⏱ A library for working with dates and times in JS
https://moment.github.io/luxon
MIT License
15.42k stars 729 forks source link

Support daylight savings (`dst`) option #1278

Closed davispuh closed 2 years ago

davispuh commented 2 years ago

Is your feature request related to a problem? Please describe. Currently it's not possible to create correct local time when DST happens. For example, I want to parse 2022-11-06 02:30 after daylight saving change in New York So I can use DateTime.fromFormat('yyyy-MM-dd HH:mm', { zone: 'America/New_York' }) But I don't have any way to specify that this is after DST and not before. This matters when I want to convert it to UTC as currently it will be wrong.

Describe the solution you'd like Accept dst: true/false parameter in local time creating methods. eg. DateTime.local(), DateTime.fromObject(), DateTime.fromFormat()

Describe alternatives you've considered I guess the only way currently is to wrap Luxon DateTime in your own DateTimeDST object that keeps track of this and adds hour when converting to UTC.

icambron commented 2 years ago

Luxon can't support this. An in-depth explanation in #914

davispuh commented 2 years ago

That doesn't explain why it can't. You don't need support from timezone to tell you when is DST and when it's not. User would specify it himself knowing that and it would allow to fix this implementation:

https://github.com/moment/luxon/blob/e0c8f874304cd4ecc3944bdcff3d8f8c27102a18/src/datetime.js#L1194

by just returning this DST value. You just need to track this extra DST variable. By default it always can be undefined (current behavior) and only if user sets it then we can use it to correctly implement fixOffset() by using either one offset or other.

icambron commented 2 years ago

That's just not how it works. At the core of Luxon is a timestamp -- the number of milliseconds UTC that the time represents, regardless of zone, or DST or anything else. When you construct a datetime from local() or fromObject(), or whatever, what you're doing is asking Luxon to compute that timestamp from your inputs, which builds the Luxon DateTIme object around that timestamp. "It's (not) in DST" doesn't help Luxon do that; what Luxon needs to know is the offset to use in making that computation, and without probing times or having direct access to the rules, it can't do that. The best we could ever do is allow the user to specify the offset directly, which isn't a very good interface.

It's even worse than that: DST is not even a well-defined concepts. Some zones some years had multiple shifts, or just a permanent shift, and so on, which makes the boolean not even really make sense. I regret having added isInDST() in the first place for this reason. This is why proposals regarding ambiguous local times tend to allow the user to specify which side of the ambiguity they want, not a higher-level, mushier concept like DST.

davispuh commented 2 years ago

asking Luxon to compute that timestamp from your inputs, which builds the Luxon DateTIme object around that timestamp

And this is the limitation we're currently hitting, even when we're passing TimeZone, we can't tell if it's before DST or after DST thus Luxon can't compute the correct timestamp.

The best we could ever do is allow the user to specify the offset directly, which isn't a very good interface.

Then this should be solution because current state is broken for DST change times and applications can't do anything about it because DST does happen and users want Local time clocks working even then :P For local time 02:10-30mins can be 02:40 and that's expected how it should work.

Some zones some years had multiple shifts, or just a permanent shift, and so on, which makes the boolean not even really make sense

That's true, I didn't think about this so yeah boolean wouldn't work indeed. But then solution is having an offset parameter.

icambron commented 2 years ago

From the linked issue above:

The best we could do here is allow the caller to specify their own default offset, which would tell Luxon to "try" that offset first instead of defaulting to now()'s offset. That would effectively determine which side of the DST ambiguous local times would be interpreted as. Something like DateTime.local(2021, 5, 22, { defaultOffset: -420}), but I haven't thought too hard about it. We'd also need to figure out what happens if you specify something bogus there.

I'd accept that as a PR.