moment / luxon

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

DateTime.now() string transform produces inequivalent DateTime #1254

Closed jzabinski-dolios closed 2 years ago

jzabinski-dolios commented 2 years ago

Describe the bug If a DateTime is generated using DateTime.now(), and is transformed to a string using toISO(), and new DateTimes are made with those strings, the resulting DateTimes are not seen as equal to the original when using DateTime.equals because they have differing metadata.

To Reproduce

  1. Generate a DateTime using DateTime.now(). Example: const now = DateTime.now();
  2. Produce the equivalent string using DateTime.toISO(). Example: const nowStr = now.toISO();
  3. Produce a second DateTime using DateTime.fromISO([string from #2], {zone: [#1.zoneName]}). Example: const now2 = DateTime.fromISO(nowStr, {zone: now.zoneName})
  4. Test for equivalency. Example: now.equals(now2).

Equivalency will fail. They fail because now's metadata includes zone as SystemZone. now2 has an IANAZone.

See this Stackblitz.

Actual vs Expected behavior On DateTimes defined using DateTime.now(), it is strange to me that zoneName refers to an IANAZone, while zone refers to 'system'. I would expect the zoneName and zone to match. In other words, when using DateTime.now(), either:

  1. zone is 'system', and zoneName is 'system', so that it is clear that the system's time zone was used to generate the time, or
  2. zone is explicitly defined as IANAZone {zoneName: [zoneName], valid: true}, and zoneName is [zoneName]

Desktop (please complete the following information):

Additional context I'm not sure if this is a bug, or an intentional limitation of DateTime.now(). The workaround seems to be to explicitly define the timezone by using DateTime.local({zone: [zoneName]}) instead of DateTime.now() when it is important to be able to have the ability to transform the DateTime to and from a string.

icambron commented 2 years ago

This isn't a bug. By default in Luxon, DateTimes are in the system zone. That's why the zone constructed by DateTime.now() is a the system zone:

DateTime.now().zone //=> SystemZone

When you're rebuilding the string, you're setting the zone explicitly to an IANA zone, and thus setting that zone property. Luxon DateTimes are only equivalent if they're in different zones:

const now = DateTime.now()
const iso = now.toISO()

DateTime.fromISO(iso).equals(now) // => true (both system zone) 
DateTime.fromISO(iso, { zone: "Europe/London" }).equals(now.setZone("Europe/London")) //=> true (both the same IANA zone)

Now there is one confusing thing, which is that you're passing in the zone you're already in, which makes it seem like it should work. But an IANA zone of "America/New_York" and a system zone that happens to be America/New_York are different things. The reason for this is performance: the native Date functionality is a lot faster than Luxon computing times in an explicit zone.

In principle, we could fix this by making Luxon aware that the System zone is equivalent to some particular IANA zone and treating those as the same for the purposes of comparison. But I think that also has some drawbacks that would make debugging Luxon harder in other scenarios.

You have a few workarounds to compare system zoned dates to IANA zoned dates:

  1. Always use an IANA zones when constructing dates. You can do this with Settings.defaultZone = SystemZone.instance.name. I don't recommend this.
  2. Same as 1, but do it ad-hoc on dates you plan to compare, as in your workaround, but without having to hardcode America/New_York