moment / luxon

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

Can't format DateTime to ISO if zone is undefined #1477

Closed Gazzia closed 1 year ago

Gazzia commented 1 year ago

Describe the bug For a reason I don't understand (maybe the fault of a deep merge of a parent object or whatever), some of my DateTimes seem to lose their zone property. When I then try to stringify the parent objet, Luxon tries to converting it to JSON by converting it to ISO first, but its method toISOTime is calling the DateTime's getter isOffsetFixed, as seen here: image but this getter depends on the existence of a zone, and there is no nullity check as long as the DateTime is considered valid. image

Actual vs Expected behavior Currently, a JS error is thrown when that happens : image

Maybe if a zone isn't found in the getter, it could use the system's zone to get the information ?

Desktop (please complete the following information):

diesieben07 commented 1 year ago

How are you creating this DateTime object?

Gazzia commented 1 year ago

At the start, from a DateTime.fromISO(), in an interceptor for all responses coming from our backend: image (It's not pretty, but it works and all ISO string dates received end up as DateTimes)

diesieben07 commented 1 year ago

I can't reproduce this, DateTime.fromISO("2023-08-01T09:58:00+00:00", {zone: 'UTC+1'}).toISO() works fine. Can you produce code that reproduces this issue?

Gazzia commented 1 year ago

I really have no clue as to where and how these DateTimes lose their zone/are initialized with no zones (but it's only some of them, and it seems pretty random as to when as I tried debugging for a full day without finding why, if I find out more I'll let you know), but the DateTime is considered valid at this point, even though the rest of the line depend on the zoneproperty that is not present. image (in DateTime> get isOffsetFixed()) This throws a standard js exception, and crashes the rest of the events (in our case, the stringification of a request body containing the DateTime, thus aborting the request and leaving the app in a frozen state)

diesieben07 commented 1 year ago

This code is in the Luxon constructor: https://github.com/moment/luxon/blob/69f8c1e6aac877769aa0401b0dcd2ca51a262b02/src/datetime.js#L456-L459

The only way for zone to be null after this if something modifies DateTime#_zone after the constructor has run, because your DateTime is not invalid, therefor it must get to the zone.isValid call in the constructor - which would throw if zone was null. DateTime#_zone is not part of the public API and must not be used.

I don't think this is something Luxon is doing itself, so unless you can provide some sort of way to reproduce this (can you share the project in which it occurs, or an excerpt?) I don't think I can help much more here.

Gazzia commented 1 year ago

Ok I finally found where the problem resides.. I'll explain it for posterity, even if it's not relative to Luxon and is quite a niche problem. We have an ugly interceptor for outgoing requests that recursively analyses the body objects and suppresses private properties that (I suppose) are not necessary for our back-end treatment. But of course DateTime objects need to keep their private properties (such as _zone) to be parsed to JSON just after. so here is the fix : image

I didn't think Luxon was faulty for losing the zone, but now I don't need an alternative toISO() treatment Sorry for the inconvenience, and thanks for your help !

diesieben07 commented 1 year ago

Glad you figured it out!