Should LocalDateTime.from accept a timezoneOffsetNanoseconds field?

[justingrant]

@ptomato in #700 (comment): What's the reason to accept timezoneOffsetNanoseconds in from() instead of always taking it from the time zone object?

Is it because timezoneOffsetNanoseconds is a property? If that's the only reason, I'd suggest it's not necessary. We could treat that property like hoursInDay, weekOfYear, etc.

There were a few reasons for accepting this field:

• Provides parity with the ISO string path of from, which does accept an offset
• Provides a way to construct a LocalDateTime from a DateTime, offset, and TimeZone without having to round-trip through an ISO string
• Provides a way to change the offset without changing other fields, and without having to round-trip through an Absolute or an ISO string
• In theory, helps developers understand what's happening "under the covers", e.g. more obvious JSON representation.

But looking through that list, I'm now questioning whether these reasons are so compelling that they justify the additional complexity of accepting timezoneOffsetNanoseconds in from and with (and emitting it in getFields). Do you think any of those reasons are enough to warrant having this be a persisted field?

If not, then I'm also inclined to do exactly what you recommend: keep the field but make it a non-persisted like hoursInDay. If it's removed, it will shave a lot of complexity from from and with.

[justingrant]

@ptomato replied:
I didn't understand these two reasons at first:

• Provides a way to construct a LocalDateTime from a DateTime, offset, and TimeZone without having to round-trip through an ISO string
• Provides a way to change the offset without changing other fields, and without having to round-trip through an Absolute or an ISO string

But after thinking about it, now I do. Do I understand the problem right? it's that you can't from() a DateTime of 2019-02-16T23:45 and TimeZone of America/Sao_Paulo together without some sort of disambiguation, nor can you with() your way from '2019-02-16T23:45-02:00[America/Sao_Paulo]' to '2019-02-16T23:45-03:00[America/Sao_Paulo]'.

Now I'm not sure anymore that the field can be calculated-only, either. If I understand correctly, the offset is not uniquely determined by the rest of the internal state of LocalDateTime, so it needs to be persisted.

In from() it seems like we could use disambiguation: 'earlier' | 'later' for that purpose, which might be easier for the caller since you don't need to go and calculate the offset nanoseconds yourself. But I don't know how to solve this for with().

[justingrant]

Do I understand the problem right? it's that you can't from() a DateTime of 2019-02-16T23:45 and TimeZone of America/Sao_Paulo together without some sort of disambiguation,

Yep, that's correct.

nor can you with() your way from '2019-02-16T23:45-02:00[America/Sao_Paulo]' to '2019-02-16T23:45-03:00[America/Sao_Paulo]'.

The only way to do it with with() would be to calculate a new absolute:

// Get a DateTime during the repeated-by-DST clock hour.
const dt = new Temporal.DateTime(2020, 11, 1, 1, 30);
// Convert to LocalDateTime. Default disambiguation will pick the earlier (-07:00) local time.
const ldt = new Temporal.LocalDateTime({...dt.getFields(), timeZone: 'America/Los_Angeles'}); 
// => "2020-11-01T01:30-07:00[America/Los_Angeles]"
// desired: one hour later in real world, but same local time
const newAbsolute = ldt.absolute.plus({nanoseconds: 3.6e12});
const newLocalDateTime = ldt.with({absolute: newAbsolute});
// => "2020-11-01T01:30-08:00[America/Los_Angeles]"

If I understand correctly, the offset is not uniquely determined by the rest of the internal state of LocalDateTime, so it needs to be persisted.

Nope, the offset is calculated fields as a function of (Absolute, TimeZone). The only required internal state is Absolute, TimeZone, and Calendar. The prototype also stores a DateTime in internal state because most LocalDateTime properties and methods require DateTime data, but this is really just an optional cache not part of the internal data model.

In from() it seems like we could use disambiguation: 'earlier' | 'later' for that purpose, which might be easier for the caller since you don't need to go and calculate the offset nanoseconds yourself. But I don't know how to solve this for with().

Currently in the prototype, both from and with accept disambiguation: 'earlier' | 'later' | 'reject' for exactly this reason. The offset (if it's present in the property bag) overrides the disambiguation setting. Here's part of the current with implementation:

    // Deal with the rest of the fields. If there's a change in tz offset, it'll
    // be handled by `from`. Also, unless we're changing the `absolute` then
    // omit it so that it won't conflict with any other fields that we're adding
    // here.
    const { absolute: baseAbsolute, timeZoneOffsetNanoseconds: baseOffset, ...fields } = base.getFields();
    if (updateAbsolute) (fields as LocalDateTimeFields).absolute = baseAbsolute;
    if (updateOffset) (fields as LocalDateTimeFields).timeZoneOffsetNanoseconds = baseOffset;
    const merged = { ...fields, ...localDateTimeLike };
    return LocalDateTime.from(merged, options);

So you can probably see why I'm on the fence about supporting this field or not. There are some clear use cases why it'd be helpful, but those are also relatively obscure and adding the field adds some complexity.

[ptomato]

Ah, right, it's Absolute+TimeZone, not DateTime+TimeZone. So I didn't understand it correctly. In that case I'd agree it's not necessary to make this field persistent or support it in with() or from(). But on the other hand it does seem like we need to think more about what with() and from() should actually mean for this type.

[justingrant]

Yep. After writing a lot of sample code using LocalDateTime, here's what I've observed about with() and from():

1. The fields that work well (where "well" means easy to code and easy to deal with conflicts/ambiguity) are: DateTime fields, absolute, timeZone, and calendar. That said, @sffc has concerns (see LocalDateTime fields in from and with #706) about DateTime and Calendar fields.
2. I'm on the fence about timeZoneOffsetNanoseconds per discussion above.
3. You've raised the question about whether we need a more ergonomic way to merge together multiple Temporal objects into one LocalDateTime.

// current implementation
Temporal.LocalDateTime.from({...date.getFields(), ...time.getFields(), timeZone: 'Europe/Paris'});

// Alternate idea could be to accept fake fields: `date`, `time`, `monthDay`, `yearMonth`.
// These would not be emitted via getFields(). 
// If there's a conflict (e.g. `date` vs. `year`) then always throw.
// If we did this here, we'd also want to do it in `DateTime.from` and `Date.from`.
Temporal.LocalDateTime.from({date, time, timeZone: 'Europe/Paris'});

I think that all these questions boil down to one core issue: Are from and with always doing explicit, 1:1 programming of the internal fields of each type? Or are these methods simply ergonomic ways to manipulate internal state, knowing that some of those manipulations might involve multiple fields at once (e.g. .with({date}) if we offered it), might change existing fields (e.g. DateTime.prototype.with({calendar})) and/or might involve calculated fields (e.g. with({year}) that changes the Absolute in internal LocalDateTime state).

After spending a lot of time writing code with Temporal, I come down squarely in the latter, ergonomics-first camp. I care more about the quack of the duck than its internal organs. The fact that LocalDateTime's internal data model is based on Absolute is a helpful implementation detail to understand, but in practice I suspect developers will care much more about how the type acts than what it stores. If code can set the year property and later call the year property and the same value comes back, then most developers probably won't care about exactly how the persistence works.

I think this is related to what @pipobscure was saying here:

This is significantly different than either ZonedAbsolute or ZonedDateTime. It seems to be more like a ZonedAbsoluteDateTime which attempts to contain and harmonise all the types Timezone, Absolute and DateTime.

[justingrant]

Here's another use case that's related to this issue: "Truncate minutes and smaller units"

// "second" 1:15AM on a day that DST ends
const ldt = LocalDateTime.from({
    year: 2020, 
    month: 11, 
    day: 1,
    hour: 1, 
    minutes: 15,
    seconds: 30,
    milliseconds: 123,
    microseconds: 456,
    nanoseconds: 789
  }, {disambiguation: 'later');

// should this return a time in the earlier or later 1-2AM hour?
ldt.with({minutes:0, seconds: 0, milliseconds: 0, microseconds: 0, nanoseconds: 0}).toString();

There are two options:

1. Assume that the caller's intent is to keep hours and larger units constant, so the timezone offset shouldn't change.
2. You're making a change to a DateTime unit, so you should expect that disambiguation will happen on the resulting DateTime. You can get the "second hour" behavior but you'll need to opt into it via {disambiguation: 'later'}

My sense is that this is a corner case and we should do the easiest thing (Option 2).

But it's an example of how a timeZoneOffsetNanoseconds property could be used to enable Option 1 behavior in a reasonably discoverable way.

ldt.with({
    minutes: 0,
    seconds: 0, 
    milliseconds: 0, 
    microseconds: 0, 
    nanoseconds: 0, 
    timeZoneOffsetNanoseconds: ldt.timeZoneOffsetNanoseconds
  }).toString();

That said, this behavior is still available without timeZoneOffsetNanoseconds, it's just harder and less discoverable:

function getCurrentDisambiguation(ldt) {
  const possibleAbsolutes = ldt.timeZone.getPossibleAbsolutesFor(ldt.toDateTime());
  if (possibleAbsolutes.length < 2) return 'compatible';
  return possibleAbsolutes[0].equals(ldt.absolute) ? 'earlier' : 'later';
}
ldt.with({
    minutes: 0,
    seconds: 0, 
    milliseconds: 0,
    microseconds: 0, 
    nanoseconds: 0
  }, {disambiguation: getCurrentDisambiguation(ldt)).toString();

So even though removing timeZoneOffsetNanoseconds makes this harder, it doesn't make it that much harder. So I'm still not convinced that this property is needed in from and with.

Also, it's arguable that the "harder" approach is actually more correct too, because if a DST transition happens in the middle of a clock hour or isn't a full hour of offset change, then the "Option 1" code could throw if the old timeZoneOffsetNanoseconds would be invalid for the new DateTime. This failure case isn't possible with a disambiguation-based solution.

So in the end I'm still leaning towards removing this prop from from and with. I'll make this change later this week unless anyone objects.

[justingrant]

I realized that there's a potential compromise position: remove timeZoneOffsetNanoseconds from getFields(). This removes two areas of user-facing complexity that I've been concerned about:

• a weird property showing up in getFields().
• hard to switch the time zone when you want to leave local time unchanged

Here's an example that illustrates both problems:

function otherTimeZoneSameClockTime(ldt, newTimeZone) {
  const { timeZoneOffsetNanoseconds, ...rest } = ldt.getFields();
  return Temporal.LocalDateTime.from({...rest, timeZone: newTimeZone});
}

With this change, it'd be simpler:

function otherTimeZoneSameClockTime(ldt, newTimeZone) {
  return Temporal.LocalDateTime.from({...ldt.getFields(), timeZone: newTimeZone});
}

Another problem I've been concerned about (which doesn't go away if we take this prop out of with and from) is inconsistent behavior in string and object variants of from. If timeZoneOffsetNanoseconds vanishes from getFields, then both string and object from can behave exactly the same.

What I'm proposing here seems closer in spirit to the other two "advanced users only" features in Option 3 of #706 (comment), because it'd be completely invisible to novice users who only use defaults.

Anyway, I'm planning to make this initial change today or tomorrow. Then we can decide on the larger change of removing this prop from from and with in context of the Option 2 vs. Option 3 discussion in #706. If we decide on Option 3 ("Full Superset") then we can keep it, otherwise can remove.

Sound OK?

There are two options:

1. Assume that the caller's intent is to keep hours and larger units constant, so the timezone offset shouldn't change.
2. You're making a change to a DateTime unit, so you should expect that disambiguation will happen on the resulting DateTime. You can get the "second hour" behavior but you'll need to opt into it via {disambiguation: 'later'}

My sense is that this is a corner case and we should do the easiest thing (Option 2).

BTW, I found a simpler and more consistent way to get the former behavior: if the resulting DateTime is ambiguous AND prefer==='offset' AND one of the possible Absolutes has the same offset as this, then use that Absolute. Otherwise, use disambiguation to choose the Absolute. This seems closest to the "keep things the same wherever possible" spirit of with and avoids the complexity of needing to monkey around with disambiguation in almost all cases. This would also reduce the need to use timeZoneOffsetNanoseconds in with which seems like a good thing, especially if we might remove it later.

I'm still not 100% sure that this will work--gotta try a few more use cases-- but so far it looks promising.

[justingrant]

Changes described above are in db8a8db. In addition to better DX, the new from logic is much simpler, 50+ LOC shorter, and is now mostly shared code between object and string variants. The new with behavior described above is in too.

[ptomato]

If this is done in the proof-of-concept code, can we close this issue?

(By the way, it seems to me that otherTimeZoneSameClockTime could be implemented in a way that corresponds more to the associated mental operation, by converting from LocalDateTime to DateTime and back)

[justingrant]

If this is done in the proof-of-concept code, can we close this issue?

Yep. Closing.

(By the way, it seems to me that otherTimeZoneSameClockTime could be implemented in a way that corresponds more to the associated mental operation, by converting from LocalDateTime to DateTime and back)

Yep. Here's the canonical samples excerpted from LocalDateTime JSDoc:

const sameInstantInOtherTz = ldt.with({timeZone: 'Europe/London'});
const newTzSameLocalTime = ldt.toDateTime().toLocalDateTime('Europe/London');