moment / luxon

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

FixedOffsetZone constructor accepts bad inputs, but `isValid` returns true #1068

Open yuklai opened 2 years ago

yuklai commented 2 years ago

Describe the bug When I set timezone with a ISO string as 'America/Chicago', the DateTime was created properly. When I set timezone with a FixedOffsetZone object, the DateTime was created invalid.

To Reproduce Please share a minimal code example that triggers the problem:

> var zone = new luxon.FixedOffsetZone('CDT');
undefined
> zone.isValid
true
> var d1 = luxon.DateTime.fromISO('2021-11-03T13:00:00', { zone: 'America/Chicago'});
undefined
> d1
DateTime {
  ts: 1635962400000,
  _zone: IANAZone { zoneName: 'America/Chicago', valid: true },
  loc: Locale {
    locale: 'en-US',
    numberingSystem: null,
    outputCalendar: null,
    intl: 'en-US',
    weekdaysCache: { format: {}, standalone: {} },
    monthsCache: { format: {}, standalone: {} },
    meridiemCache: null,
    eraCache: {},
    specifiedLocale: null,
    fastNumbersCached: null
  },
  invalid: null,
  weekData: null,
  c: {
    year: 2021,
    month: 11,
    day: 3,
    hour: 13,
    minute: 0,
    second: 0,
    millisecond: 0
  },
  o: -300,
  isLuxonDateTime: true
}
> var d2 = luxon.DateTime.fromISO('2021-11-03T13:00:00', { zone: zone });
undefined
> d2
DateTime {
  ts: NaN,
  _zone: FixedOffsetZone { fixed: 'CDT' },
  loc: Locale {
    locale: 'en-US',
    numberingSystem: null,
    outputCalendar: null,
    intl: 'en-US',
    weekdaysCache: { format: {}, standalone: {} },
    monthsCache: { format: {}, standalone: {} },
    meridiemCache: null,
    eraCache: {},
    specifiedLocale: null,
    fastNumbersCached: null
  },
  invalid: Invalid { reason: 'invalid input', explanation: undefined },
  weekData: null,
  c: null,
  o: null,
  isLuxonDateTime: true
}

Actual vs Expected behavior I expect that d2 is valid because the timzone is valid and CDT and America/Chicago is basically the same.

Desktop (please complete the following information):

Additional context Add any other context about the problem here.

icambron commented 2 years ago

The issue here is that FixedOffsetZone isn't protecting itself from being instantiated with inputs it doesn't support. Looking at the code, I think what happened was that the whole class was originally private and used only internally, but I later made it public by request without privatizing the constructor, or following the conventions elsewhere about validity. Specifically, the way the class works is that it has factory methods like parseSpecifier(s) and instance(offset) that return null if they fail. Serves me right for not keeping them altogether private.

At any rate, the bug here is not the zone isn't working; it's that isValid should return false. "CDT" is not a valid input for building a zone. As the docs state (emphasis added):

A named offset is a time zone-specific name for an offset, such as Eastern Daylight Time. It expresses both the zone (America's EST roughly implies America/New_York) and the current offset (EST means -5). They are also confusing in that they overspecify the offset (e.g. for any given time it is unnecessary to specify EST vs EDT; it's always whichever one is right). They are also ambiguous (BST is both British Summer Time and Bangladesh Standard Time), unstandardized, and internationalized (what would a Frenchman call the US's EST?). For all these reasons, you should avoid them when specifying times programmatically. Luxon only supports their use in formatting.