tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.35k stars 153 forks source link

withCalendar / withTimeZone #906

Closed sffc closed 4 years ago

sffc commented 4 years ago

Calendar and time zone have similar behavior. We have withCalendar but not withTimeZone. The primary reason for having withCalendar was because I believed that we wouldn't be able to come up with clean semantics on with({calendar}), but we now have that problem solved in #640. So I think we should revisit removing withCalendar, or adding withTimeZone for consistency.

ptomato commented 4 years ago

If we do decide to change something then I'm more in favour of removing withCalendar than adding withTimeZone.

justingrant commented 4 years ago

withTimeZone would be bad because it introduces subtle order-dependence issues. Copying from https://github.com/tc39/proposal-temporal/issues/640#issuecomment-686983494:

My strong preference is for Option 7, because it is consistent with how with({timeZone}) is handled in LocalDateTime, and with({timeZone}) is actually required for a specific case: if you're changing the time zone and other LDT fields at the same time, what you want is to apply DST disambiguation at the end after you've played the new fields on top of the new time zone. If you split up into two method calls, then you can end up with disambiguation applied to the intermediate state which may make the final state one hour different than what you expected.

Also, with({timeZone, calendar}) is helpful and ergonomic. as opposed to requiring two method calls.

So no matter what, I don't think we should add withTimeZone. I'm OK with removing withCalendar to encourage developers to discover with which is useful beyond calendars.

ptomato commented 4 years ago

Any objections to removing withCalendar on all types, and using with({ calendar }) instead?

ptomato commented 4 years ago

Meeting, Sept. 18: We cannot remove withCalendar(), and must add withTimeZone() to LocalDateTime, to address concerns around code like dateTime.with({ ...otherDate }), which (likely unexpectedly) copies the calendar from otherDate to dateTime. Therefore the decision is that calendar and timeZone are not set by with(). (They are taken into account in case they are needed in order to know how to interpret other fields of the property bag, but they don't directly affect the result.)

sffc commented 4 years ago

This change helps keep changes to time zone and calendar explicit, which is a good thing.

justingrant commented 4 years ago

I started implementing this change in ZDT and now have concerns around adopting the behavior proposed above for timeZone:

Let's discuss.

sffc commented 4 years ago
  1. No, it's not the only reason. Another compelling reason was so that you could do things like, "set the hour of this ZDT to 8pm in New York, but keep the current time zone", and also keep in mind that there was a desire to support other zoned types in user land. For example,
zdt = Temporal.ZonedDateTime.from("2020-10-01T12:00:00[America/Los_Angeles]");
zdt.with({ hour: 20, timeZone: "America/New_York" });
// zdt is now 2020-10-01T17:00:00[America/Los_Angeles]
  1. If we don't already do so, you should get an exception if you call .with() with a time zone and/or calendar but no other data fields. Would that help with learnability?
justingrant commented 4 years ago
  1. If we don't already do so, you should get an exception if you call .with() with a time zone and/or calendar but no other data fields. Would that help with learnability?

If we stick with the currently proposed behavior, then this seems like a good change to make.

Another compelling reason was so that you could do things like, "set the hour of this ZDT to 8pm in New York, but keep the current time zone", and also keep in mind that there was a desire to support other zoned types in user land.

IMHO after writing some code using the proposed behavior it still seemed surprising that explicitly specifying a timeZone field doesn't actually change the value of the field in this, unlike other fields like hour or day.

That said, I agree with the proposed behavior for calendar for several reasons:

TL;DR - The calendar behavior helps the ecosystem because it shields most developers from unexpected non-ISO calendar behavior and favors retaining the current calendar which is likely to be the safest behavior for the majority of developers who don't understand non-ISO calendars.

These benefits don't seem to apply (or apply much less) to time zones:

So I'd lean towards:

sffc commented 4 years ago

We could throw if either timeZone or calendar is present in the option bag. Plenary has been pretty clear that they don't see spreading operations as something we should encourage, so in most cases this would be a non-issue.

justingrant commented 4 years ago

We could throw if either timeZone or calendar is present in the option bag. Plenary has been pretty clear that they don't see spreading operations as something we should encourage, so in most cases this would be a non-issue.

Would that mean that .with(time) or with(date) would always throw if time and date were Temporal.Time or Temporal.Date instances? Or would we special-case Temporal instances and only throw for non-Temporal-instance property bags?

justingrant commented 4 years ago

Meeting 2020-10-15:

  1. We'll have a withTimeZone on ZDT and keep withCalendar on ZDT and other types
  2. Throw if no persistable properties are present in the input. This includes {} but also {calendar} or {timeZone} or {timeZone, calendar}.
  3. I'll investigate and try some code for the case of zdt.with(zonedTime) for a userland (or Temporal V2) ZonedTime class. A canonical use case: "What time in New York should I call my friend in London to wish him a happy new year?" Depending on the outcome of this investigation, I'll either: a) be OK with keeping the current behavior where the arguments are interpreted in the zonedTime's time zone but not persisted OR b) revisit the discussion above to either disallow or persist the timeZone in with.
justingrant commented 4 years ago

After investigating (3) above, I think we should always throw if a non-undefined timeZone property is present in the args to ZonedDateTime.prototype.with. Reasons (with details below):

A) Consistency

Elsewhere in Temporal where we throw when there's ambiguity and there's no default behavior that we think will work to resolve the ambiguity for a large majority of use cases. Here's two examples of similar cases where we also throw:

zonedDateTime.with(zonedTime) is a similar case. It's ambiguous when reading code whether the result should have the time zone of other or this, and there's no obvious default behavior which would cover almost all use cases. Also, using with for this case seems relatively unusual compared to the preferred solution below, so it's not like we're handicapping a common case.

zonedTime.toZonedDateTime(date);

True, we could opt for consistency with calendar but most developers will never use a non-ISO calendar so this consistency won't really matter much for them because the thing it's consistent with is something they'll never notice. The 4 cases above are much more mainstream use cases, so if we want consistency, that's probably where to favor.

B) with is problematic for the canonical use case

The use case we discussed in the meeting, "What time in New York should I call my friend in London to wish him a happy new year?", isn't actually a good fit for zonedDateTime.with(zonedTime). The problem is that the date of the result is ambiguous. Unlike calendars where there's a neutral ISO calendar that can be used to evaluate any date and/or time, there's no equivalent lingua franca for time zones. For example:

zt = new ZonedTime('00:00[Europe/London]');
zdt = Temporal.now.zonedDateTimeISO('America/New_York');

callAt = zdt.with(zt);  // what should be the `date` of `callAt`?  

// no ambiguity via explicit conversion
callAt = zt.toZonedDateTime(zdt.date).withTimeZone(zdt.timeZone).time;
// OR (if it's before midnight where I am)
callAt = zt.toZonedDateTime(zdt.date.add({days: 1}).withTimeZone(zdt.timeZone).time;

This seems similar to the ambiguous cases listed in (A), so I think with should throw to force the user to be explicit about their intended date.

BTW, this behavior may also problematic for calendar, not for with(date) but for cases like japaneseDateTime.with({calendar: 'hebrew', month: 2}) where a partial date is provided. The result won't only change its month, it will also change the day and maybe even the year, because months are different lengths between calendars. This calendar case seems harder to resolve than timezone case where we could simply throw if a timeZone is present in the input, so we may want to try to come to consensus on the timezone case first and then figure out the harder calendar case.

sffc commented 4 years ago

Time-wrapping is a problem, but it's not specific to timeZone/calendar. Given the cyclic nature of most time units, I think it would be useful if .with() had an option like { resolution: 'nearest' | 'absolute' }, where 'nearest' goes to the nearest match, and 'absolute' sets the field directly. Like this:

let dt = Temporal.PlainDateTime.from("2020-10-01T23:00:00");
dt.with({ hour: 0, resolution: "absolute" });  // 2020-10-01T00:00:00
dt.with({ hour: 0, resolution: "nearest" });  // 2020-10-02T00:00:00

I'm not necessarily convinced by the consistency argument. Across Temporal, we generally make a best attempt effort to interpret arguments in a way that makes sense. The cases where we throw exceptions are cases where there is not a sensible interpretation.

justingrant commented 4 years ago

I'm not necessarily convinced by the consistency argument. Across Temporal, we generally make a best attempt effort to interpret arguments in a way that makes sense. The cases where we throw exceptions are cases where there is not a sensible interpretation.

My understanding is a little different. We've chosen to throw in cases where we think that callers are likely to assume a different default behavior, even if we could come up with a sensible default. For example, we decided to throw for code like this:

zdt.difference(zdtInOtherTimeZone, {largestUnit: 'days'})

We could have defined this method to always use this.timeZone to calculate the difference. That'd be a sensible default. But enough users might not anticipate this behavior that we opted to throw instead.

I see the zdt.with({timeZone, hour}) case as similar. It's not that we can't come up with a sensible default, it's that naive users are likely to have trouble anticipating our sensible default, which makes throwing the most sensible option. Another ingredient is whether throwing would foreclose an important ergonomic workflow. In this case it wouldn't, unless I'm missing some important use case.

Given the cyclic nature of most time units, I think it would be useful if .with() had an option like { resolution: 'nearest' | 'absolute' }, where 'nearest' goes to the nearest match, and 'absolute' sets the field directly.

I think this would be a great cookbook example. But I don't think it's necessarily wise to add this to with, because today with offers simple and easy-to-understand behavior: it only changes the properties provided in the input bag and doesn't change any others. This makes with really easy to reason about. The only possible exception I found is a rare case that may never happen in real life:

In this issue we're discussing whether to add another exception which is for "partial" cases like japaneseDateTime.with({calendar: 'hebrew', month: 2}) or zdt.with({timeZone: 'America/Los_Angeles', hour: 2}). If we disallow these cases then we'll retain the current simplicity of with, which I think would be best.

Also, FWIW in the past we removed options from with (e.g. the ability to balance to wrap around to a new month for example) in order to achieve with simplicity.

ptomato commented 4 years ago

This is to be discussed tomorrow in the overflow meeting, but I want to emphasize that we did already reach a consensus on this, so the default will be that we stick with that decision. We can change decisions if there is new information, but if the new information is not convincing to everyone then we cannot keep discussing it endlessly.

sffc commented 4 years ago

This is to be discussed tomorrow in the overflow meeting, but I want to emphasize that we did already reach a consensus on this, so the default will be that we stick with that decision. We can change decisions if there is new information, but if the new information is not convincing to everyone then we cannot keep discussing it endlessly.

I agree, although there is actually one open question on which we've never gotten consensus: exactly how to resolve dates and times in different time zones and calendars. The current plan of action is that the timeZone and calendar properties to .with() are used to interpret the other properties in that option bag, but we haven't landed on the exact algorithm.

Algorithm 1

The algorithm I proposed was:

  1. Transform the receiver into the calendar system and time zone of the argument.
  2. Apply the new fields.
  3. Transform back into the original calendar system and time zone.

Algorithm 2

@pipobscure proposed instead:

  1. Create a new ZonedDateTime using the argument's time zone and calendar system, filling in missing fields with default values.
  2. Transform the ZonedDateTime into the receiver's calendar system and time zone.
  3. Go back through the non-undefined fields in the argument and copy them into the receiver.

Example 1: Date Flip

This example illustrates a downside of Algorithm 1.

// 11:11pm on Thursday, October 22, in America/Denver
const a = Temporal.ZonedDateTime.from("2020-10-22T23:11:00-06:00[America/Phoenix]");

// Set hour to 8pm in America/New_York
const b = a.with({
  hour: 20,
  timeZone: "America/New_York",
});

Algorithm 1:

  1. Transform the input into America/New_York: 2020-10-23T01:11:00-04:00[America/New_York]
  2. Apply hour: 2020-10-23T20:11:00-04:00[America/New_York]
  3. Transform back into America/Denver: 2020-10-23T18:11:00-06:00[America/Denver]

Algorithm 2:

  1. Create an intermediate ZonedDateTime by filling in defaults: 2020-10-22T20:00:00-04:00[America/New_York]
  2. Transform the ZonedDateTime to America/Denver: 2020-10-22T18:00:00-06:00[America/Denver]
  3. Apply the hour to the original ZonedDateTime: 2020-10-22T18:11:00-06:00[America/Denver]

Note that Algorithm 1 changes the date as well as the time.

Example 2: Time Zone Transition

This example illustrates a downside of Algorithm 2.

// 11:11pm on Sunday, November 1, in America/Phoenix
const a = Temporal.ZonedDateTime.from("2020-11-01T23:11:00-07:00[America/Phoenix]");

// Set hour to midnight in America/Halifax
const b = a.with({
  hour: 0,
  timeZone: "America/Halifax",
});

Algorithm 1:

  1. Transform the input into America/Halifax: 2020-11-02T02:11:00-04:00[America/Halifax]
  2. Apply the hour: 2020-11-02T00:11:00-04:00[America/Halifax]
  3. Transform back to America/Phoenix: 2020-11-01T21:11:00-07:00[America/Phoenix]

Algorithm 2:

  1. Create an intermediate ZonedDateTime by filling in defaults: 2020-11-01T00:00:00-03:00[America/Halifax] (note the jump across a DST transition)
  2. Transform the ZonedDateTime to America/Phoenix: 2020-10-30T20:00:00-07:00[America/Phoenix]
  3. Apply the hour to the original ZonedDateTime: 2020-11-01T20:11:00-07:00[America/Phoenix]

Note that Algorithm 2 produces a ZonedDateTime that is not actually at midnight in America/Halifax due to a daylight savings time transition.

justingrant commented 4 years ago

Yep, what @sffc said above was what I was (poorly) trying to convey in the meeting: that we hadn't actually made a decision on whether with can change scalar fields (i.e. not Calendar or TimeZone fields) that were not present in the input bag.

I apologize because I didn't do a good job explaining this concern in the meeting! Clearly I woke up on the wrong side of the bed this morning.

Shane, just to make sure I'm 100% understanding your points, I'll try to recapitulate what you're saying with another example. Does below accurately model the decision we have to make?

My understanding was that @sffc assumed that last week's decision was "yes, with can change other fields", while @pipobscure assumed that last week's decision was "no, with can only change scalar fields in the input". Shane and Philipp, did I understand your positions correctly?

To illustrate the difference, assume this code: (EDIT: Earlier versions of the code below were buggy. Fixed now.)

date = Temporal.Date.from('2020-01-01');  // in ISO calendar
date.with({ calendar: 'hebrew', month: 3 });
// FYI: the instant below in Tokyo is 3:00PM on Thursday, Jan 1 2020 
zdt = Temporal.ZonedDateTime.from('2019-12-31T22:00[America/Los_Angeles]');
zdt.with( {timeZone: 'Asia/Tokyo', hour: 22 }); // same local time, but in Tokyo

Which of the following will it act like?

Option A (@pipobscure): with only changes scalar fields in its input (nothing else)

date.with({
  month: date.withCalendar('hebrew').with({ month: 3 })
});
zdt.with({
  hour: zdt.withTimeZone('Asia/Tokyo').with({ hour: 22 }).withTimeZone(zdt.timeZone).hour
});  // result is 5AM Weds Dec 31 2019 (17 hours earlier than original instant)

Option B (@sffc): with will change other fields too

// result may change day and year as well as month
date.withCalendar('hebrew').with({month: 3}).withCalendar('iso8601')

// result is 5AM Weds Jan 1 2020 (7 hours later than original instant)
zdt.withTimeZone('Asia/Tokyo').with({ hour: 22 }).withTimeZone('America/Los_Angeles');
justingrant commented 4 years ago

Suggestion for tomorrow's discussion: before settling on a solution, could we do some quick on-screen coding of a real use case(s) for whatever solution(s) are being proposed? Having been through these problems a few times, I've consistently been surprised when solutions that seemed great on paper were problematic while coding real use cases.

Here's a few simple cases we could try:

Nothing fancy, but something like below so the plan is really precise and clear. Examples:

zdt.withTimeZone('Asia/Tokyo').with({ hour: 22 }).withTimeZone('America/Los_Angeles');
zdt.with({
  hour: zdt.withTimeZone('Asia/Tokyo').with({ hour: 22 }).withTimeZone(zdt.timeZone).hour
}); 
justingrant commented 4 years ago

Algorithm 2

@pipobscure proposed instead:

  1. Create a new ZonedDateTime using the argument's time zone and calendar system, filling in missing fields with default values.
  2. Transform the ZonedDateTime into the receiver's calendar system and time zone.
  3. Go back through the non-undefined fields in the argument and copy them into the receiver.

@sffc could you explain what you mean by "default values"? Is that fields from this? Or literally default values like zeroes for times, and 1 & 1 for month and day? I assumed the former, but are you assuming the latter?

sffc commented 4 years ago

@sffc could you explain what you mean by "default values"? Is that fields from this? Or literally default values like zeroes for times, and 1 & 1 for month and day? I assumed the former, but are you assuming the latter?

My understanding from what @pipobscure said is that the date values default to the dates values from the receiver, and the time values default to zero, but @pipobscure would be better to comment on that.

justingrant commented 4 years ago

@pipobscure - could you clarify what your expected output would be for the following cases?

date = Temporal.Date.from('2020-01-01');  // in ISO calendar
date.with({ calendar: 'hebrew', month: 3 });

// FYI: the instant below in India is 12:15PM on Thursday, Jan 1 2020 
zdt = Temporal.ZonedDateTime.from('2019-12-31T22:45[America/Los_Angeles]');
zdt.with({ timeZone: 'Asia/Kolkata', hour: 22 }); 
// EDIT: if behavior differs between time units and date units, then how will be following code behave? 
zdt.with({ timeZone: 'Asia/Kolkata', month: 3, hour: 22 }); 
zdt.with({ timeZone: 'Asia/Kolkata', month: 3 }); 
justingrant commented 4 years ago

Sounds like there are (at least) four different options to consider. Here's the options as I understand them. If folks have time before tomorrow's meeting, could you weigh in with your preferred option? (I'd hoped to be able to get to this earlier so we wouldn't need meeting time for this, but it's been a busy week.)

Here's some userland code

date = Temporal.Date.from('2020-01-01');  // in ISO calendar
date.with({ calendar: 'hebrew', month: 3 });

// FYI: the instant below in India is 12:15PM on Thursday, Jan 1 2020 
zdt = Temporal.ZonedDateTime.from('2019-12-31T22:45[America/Los_Angeles]');
zdt.with( {timeZone: 'Asia/Kolkata', hour: 22 });
// These next 2 can be ignored unless the option has different behavior for time units vs. date units
zdt.with( {timeZone: 'Asia/Kolkata', month: 3, hour: 22 }); 
zdt.with( {timeZone: 'Asia/Kolkata', month: 3 }); 

How should that code behave?

Option A) Replace only fields that are present in the input

// Only month field will change. 
// Is the result meaningful, given that Hebrew & ISO months don't start on the same ISO dates?
date.with({
  month: date.withCalendar('hebrew').with({ month: 3 }).withCalendar('iso8601').month
});

// Result is 14 hours earlier (same calendar day) than original instant
zdt.with({
  hour: zdt.withTimeZone('Asia/Kolkata').with({ hour: 22 }).withTimeZone(zdt.timeZone).hour
});  // =>  2019-12-31T08:45-08:00[America/Los_Angeles]

Option B) Update any changed fields, including fields not present in the input

// Result may change day and year as well as month
date.withCalendar('hebrew').with({month: 3}).withCalendar('iso8601')

// Result is 10 hours later (next calendar day) than original instant
zdt.withTimeZone('Asia/Kolkata').with({ hour: 22 }).withTimeZone('America/Los_Angeles');
// => 2020-01-01T08:45-08:00[America/Los_Angeles]

Option C) Default time units to zero (but only if 1+ time units are present in the input?)

I took a guess below for time zone behavior in the simplest case below, but this option needs clarification from @pipobscure about expected behavior. Specifically:

// Needs clarification about how this should behave
// date.withCalendar('hebrew').with({month: 3});
// TODO: show behavior

// result is 14 hours and 15 minutes earlier (same calendar day) than original instant
intermediate = zdt.withTimeZone('Asia/Kolkata')
    .with({ hour: 22, minute: 0, second: 0, millisecond: 0, microsecond: 0, nanosecond: 0 })
    .withTimeZone(zdt.timeZone);
const {hour, minute, second, millisecond, microsecond, nanosecond} = intermediate.getFields();
zdt.with({hour, minute, second, millisecond, microsecond, nanosecond});
// => 2019-12-31T08:30-08:00[America/Los_Angeles]

// If behavior differs between time units and date units, then how will be following code behave? 
// zdt.with({ timeZone: 'Asia/Kolkata', month: 3, hour: 22 }); 
// TODO: show behavior

// zdt.with({ timeZone: 'Asia/Kolkata', month: 3 }); 
// TODO: show behavior

Option D) Throw if calendar and/or time zone doesn't match this

with would throw if either of the following are true:

To use with across calendars and/or time zones, the caller must be explicit about their intent. Either they can convert this or other to the opposite calendar and/or time zone, or they can remove calendar and timeZone from the input.

The same behavior exists elsewhere in Temporal where we throw if there's a potentially confusing interaction across calendars and/or time zones:

(EDIT: we captured the options below in the meeting discussion, so adding them here to close the loop)

Option E) Throw if calendar and/or time zone are present in the input

Same as (D), except with would throw if calendar or timeZone is present in the input, regardless of the current timeZone or calendar properties of `this.

Option F) Throw if time zone doesn't match this or if calendar.id's don't match and both are non-ISO

Same as (D), except unmatched calendars wouldn't throw if one of them were ISO.

Option G) calendar and/or timeZone in the input act like withCalendar and/or withTimeZone, respectively

This was the previous plan of record before we started this GH issue: if a timeZone and/or calendar is present in the input, then the result will have that calendar and/or time zone. The challenge with this plan is that it works fine for time zones, but for calendars it makes it too easy to inadvertently change the calendar of this, e.g. zdt.with('10:00') would change the calendar of ZDT to ISO which is probably unexpected.

// result has a new calendar. Day and year are unchanged after the calendar is changed. 
date.withCalendar('hebrew').with ({ month: 3 });

// Result is in a new time zone and is is 10 hours later than original instant
zdt.withTimeZone('Asia/Kolkata').with({ hour: 22 });
// => 2020-01-01T22:15+05:30[Asia/Calcutta]

My preference is (D) because:

justingrant commented 4 years ago

Meeting 2020-10-29:

The conclusion: (A) seems like the "least bad" outcome, and there's at least one use case (albeit a kind-of-buggy one that is not a best practice) where it could reasonably be seen as useful. Specific consensus was:

  1. Use option (A) as defined above
  2. Retain the other decisions made previously:
    • We'll have a withTimeZone on ZDT and keep withCalendar on ZDT and other types
    • Throw if no persistable properties are present in the input. This includes {} but also {calendar} or {timeZone} or {timeZone, calendar}.
  3. Document this heavily!
sffc commented 4 years ago

I discussed this offline with @justingrant, but I increasingly think that Option A is just garbage and we cannot use it in Temporal.

Fields Cannot Be Equated Between Calendars

For example, the following code is a perfectly reasonable use case.

const jpnDateTime = Temporal.PlainDate.from("2020-05-05[u-ca=japanese]");
jpnDateTime.with("2015-01-01")

I expect that line to set my Japanese date (as fields: { era: "reiwa", year: 2, month: 5, day: 5 }) to the Julian day corresponding to 2015-01-01 in the ISO calendar. The above line is equivalent to:

jpnDateTime.with({
  calendar: "iso8601",
  year: 2015,
  month: 1,
  day: 1,
})

Based on Justin's explanation to me of Option A, the above code results in the nonsensical date 2055-01-01[u-ca=japanese]. Why?

  1. First, we project the receiver into ISO: 2020-05-05
  2. We apply the requested fields: 2015-01-01
  3. We transform the result of step 2 into Japanese: 2015-01-01[u-ca=japanese]. As fields: { calendar: "japanese", era: "heisei", year: 35, month: 1, day: 1 }
  4. We copy the input fields, which are year, month, and day, into the receiver. We get: { calendar: "japanese", era: "reiwa", year: 35, month: 1, day: 1 }. As string: 2055-01-01[u-ca=japanese].

In other words, Option A returns complete garbage for this legitimate use case.

The root cause of this problem is that it is not possible to equate the fields from one calendar into the fields of another. To illustrate: One calendar could represent dates in years, months, and days. Another could represent them in rainbows, leprechauns, and pots of gold. However, .with() should still work with those calendars as it works with any other calendar.

The Halifax Problem

I listed this example earlier, but I'll reproduce it here:

// 11:11pm on Sunday, November 1, in America/Phoenix
const a = Temporal.ZonedDateTime.from("2020-11-01T23:11:00-07:00[America/Phoenix]");

// Set hour to midnight in America/Halifax
const b = a.with({
  hour: 0,
  timeZone: "America/Halifax",
});

Option B returns results that are guaranteed to uphold the following invariant: the return value is guaranteed to represent the requested date/time in the argument's time zone / calendar system. In the above example, you are guaranteed to get back a datetime that is 12am in America/Halifax, even if it is a different date than when you started.

Option A just returns garbage. It returns a datetime that may or may not be 12am in America/Halifax. Justin justified this by saying that it is an "approximation". I honestly don't buy that. I can't think of any use case where you would actually want a garbage approximation like this.

Temporal.with() can already change other fields

The only downside I've seen so far of Option B is that with() can change fields that were not requested. However, with() already does exactly this:

Temporal.PlainDate.from("2020-01-31").with({ month: 2 });  // 2020-02-29

Even though I didn't ask for the day to change, it changed nonetheless. I claim this is no different than the case when I pass a different calendar system or time zone as an argument: Temporal retains the right to adjust other fields in order to retain certain invariants.

justingrant commented 4 years ago

@sffc came up with a counter-proposal in #1085 that IMHO works better than my own proposal in #1084 and any of the other proposed behaviors above. Here's what's proposed. @pipobscure: let us know what you think of this proposal below.

1. Add withPlainDate and withPlainTime methods to PlainDateTime and ZonedDateTime

2. Rename with to withFields and throw if calendar or timeZone fields are present

3. Solution for partial dates (including PlainMonthDay / PlainYearMonth)

  • If a MonthDay/YearMonth method is problematic (e.g. for calendar reasons) the bias is to remove it and rely on Date and docs instead. Don't bend over backwards to shoehorn functionality into these types if a less ergonomic Date+docs solution is OK.
  • Don't make usability worse for other Temporal types just because a pattern doesn't work for MonthDay/YearMonth. These types are weird and making them different is OK.
justingrant commented 4 years ago

Hi @pipobscure - Are you OK with the solution described in the comment above? We're coming down to the wire to get ready for Friday's workshop and we're waiting on your review. Thanks!

ptomato commented 4 years ago

I really hope you three can come to agreement on this. withPlainDate and withPlainTime, and throwing on time zone or calendar in with, sound like a good way to solve the problem to me. I'm less happy about renaming with to withFields as it seems like a step backwards in API clarity and brevity, and furthermore it seems like another big last-minute rename with lots of busywork for dubious gain. The solution seems better or at least equally good if we don't rename with.

justingrant commented 4 years ago

If we're not worried that developers will be surprised by with('10:00') or with(Temporal.Date.from('2020-01-01')) always throwing, then I'm OK with leaving the with name and using docs to explain why those patterns throw.

@pipobscure - Last call for feedback! We need to get this implemented ASAP.

pipobscure commented 4 years ago

I think the suggested solutions are even worse than just throwing!

Let’s keep things simple and not add surface. Instead just rename to withFields and throw when supplied with a calendar/timezone in god’s name.

And document that putting in temporal types is illegal (because it would throw).

pipobscure commented 4 years ago

Instead of withPlainDate/withPlainTime yoh can just destructure into an object and supply those values ti withFields

justingrant commented 4 years ago

So plainDateTime.with('10:00') should throw?

pipobscure commented 4 years ago

We’ve always treated ISO strings like property bags, so no.

We’ve used Time.from to standardize this but not as that that’s used. So the string is treated just as much as it would in PlainTime.from.

What would throw is withFields(‘10:00[America/New_York]’)

justingrant commented 4 years ago

PlainTime.from returns an object with a calendar property, which would cause withFields to throw. Or are you suggesting a different behavior where the withFields implementation would not directly call PlainTime.from with the string, but instead would consider a string without a calendar annotation to implicitly adopt the calendar of the receiver?

pipobscure commented 4 years ago

We never said we would USE .from only that we would treat ISO-Strings LIKE property bags.

For most cases using .from is a nice shortcut, but because of the throwing behaviour it’s NOT identical

pipobscure commented 4 years ago

We treat these strings/property-bags identical in all functions and we schemaed it for .from

But in .from the calendar can come from the property-bag or be defaulted. And the timezone just has to be there with ZonedFateTime.

pipobscure commented 4 years ago

If we throw in withFields when calendar/timezone is present, we should do that everywhere (so withFields in PlainDate/PlainDateTime/... should act the same way)

justingrant commented 4 years ago

I'd like to clarify what you're proposing so I can be sure I understand the proposal. Assume the following code below. Which with calls do you think should throw, if any?

jpnDateTime = Temporal.PlainDateTime.from(`2020-01-01`).withCalendar('japanese');
jpnDateTime.with('10:00'); 
jpnDateTime.with(Temporal.Time.from('10:00'));
jpnDateTime.with(Temporal.Time.from('10:00').getFields());
pipobscure commented 4 years ago

Only the third one.

(If I recall correctly getFields does not return calendar/timezone; right? if it dis it would also throw)

But please RENAME to withFields

justingrant commented 4 years ago

(If I recall correctly getFields does not return calendar/timezone; right? if it dis it would also throw)

getFields returns all fields, otherwise it couldn't be used for round-trip object/JSON serialization.

So if I understand you correctly, your proposal is:

  1. Rename with to withFields
  2. Throw if a calendar or timeZone property is passed to withFields (EDIT: fixed typo)
  3. Consider strings passed to with without a calendar annotation to implicitly adopt the calendar of the receiver
  4. DON'T add withTime and withDate methods.
  5. Missing time units are defaulted to zero with string arguments, but not for property bag arguments. So plainDateTime.withFields({hour: 10}) would not change the value of minute in the result.

Is that your proposal?

justingrant commented 4 years ago

I fixed a typo in (2) above.

justingrant commented 4 years ago

And in (5). ;-)

pipobscure commented 4 years ago

Pretty much (except for the typo in 2 where I think you meant withFields).

And if getFields contains timezone/calendar then your example line 4 would also throw.

pipobscure commented 4 years ago

LoL you were too quick with your typo fixes 😀

justingrant commented 4 years ago

OK, then it sounds like we're all in agreement on most of the proposal. Here's the only places where I think the proposals differ:

Can we narrow the disagreement? Anyone open to go over to the other side on any of these three questions? It feels like we're very close to a resolution, especially on the most important part which is that with/withFields should throw if a timezone or calendar is present in the input.

Here's an attempt at brokering compromise: @pipobscure - if we renamed to withFields as you proposed, but also added withDate and withTime per @sffc's proposal, and if we limited withFields to just property bags (no strings) would you be OK with that solution? If so then @ptomato would you be OK with that?

justingrant commented 4 years ago

BTW, given that we all agree that with should throw for Temporal objects or property bags with (non-undefined) calendar or timezone properties, @ptomato I think this unblocks almost all of the with implementation in ZonedDateTime. String support and a new name could be layered on top depending on what we decide above. Is that helpful?

pipobscure commented 4 years ago

I think we should explicitly NOT have withDate and withTime! (consider this strong opposition)

I’d much prefer withFields, since the name excludes temporal objects and therefore lends clarity. But I’m not going to stress too much about it. Suffice it to say that clarity trumps brevity 100% of the time.

As to string values, I’d prefer to allow them by a country mile. But given that it would be something that’s possible to add in v2 (since it doesn’t break anything else) I’d accept leaving it out i. the interest of time.

ptomato commented 4 years ago

A - Why spend extra effort, this late in the game, to rename this tried-and-true API into something more verbose and less intuitive? I disagree that it is an improvement in clarity. It seems like lots of last-minute work for dubious gain. "with() takes a property bag" is a pattern I've encountered elsewhere, but withFields() is unfamiliar and causes developers to wonder what is meant by "fields". As I said above, the solution seems better or at least equally good if we don't rename with().

B - I don't care, as I believe that passing other Temporal objects to with() is only marginally useful. Although, it was explicitly mentioned in the last plenary that the use case covered by withDate/withTime was desired, which is the sole reason that I'm OK to add this API at this point. I would actually prefer it to be left for a subsequent proposal, but I accept that it's a sticking point for others.

C - I don't care about this either, since I also believe passing strings to with() is only marginally useful. But since with() already does accept strings, I'm fine with not doing the work to remove it, or I'm fine to remove it and introduce it in a subsequent proposal.

I agree on the common points, and that is indeed what I have started to implement in ZonedDateTime.

In addition I'm really disappointed to see that this is dragging on into discussions about big renames and more API surface. I don't understand why this is happening, because I thought we were all in for an attempt to go to Stage 3 in January, and this is just putting that more and more at risk.

pipobscure commented 4 years ago

Ok, scree the rename

justingrant commented 4 years ago

OK, here's a lowest-common-denominator, future-compatible proposal to resolve the remaining open issues. Is everyone OK with this? @sffc you're the only one who hasn't weighed in today, so object now or hold your peace until V2!

  1. No rename of with
  2. No withDate nor withTime methods. Could reconsider adding in a V2.
  3. Don't accept strings in with. Only accept property bags. Could reconsider adding in a V2.

Combined with what we already agreed with earlier:

  1. Throw if a calendar or timeZone property is passed to with. This means that Temporal objects are not allowed as arguments to with because they all have calendars.
  2. Missing time units are NOT defaulted to zero (for property bags, which are the only arguments now allowed). So plainDateTime.withFields({hour: 10}) would not change the value of minute in the result.
sffc commented 4 years ago

I'm not particularly happy with this neutered result. I think it harms ergonomics with no clear reason. However, I don't have a strong objection to it.

A - I'm a 3/5 in supporting with() being renamed to withFields() if it only accepts a property bag and nothing else. But I won't block on that.

B - I'm a 4/5 in favor of withTime() and withDate(). I think it covers the ergonomics of several use cases very nicely. I haven't seen any compelling argument against them. @pipobscure says he's fairly strongly opposed, but I don't see why, other than: "Let’s keep things simple and not add surface"

C - I think we should have withTime() and withDate() that accept strings. If we really don't want those methods, though, then I guess we could accept ISO-calendar strings and copy in the result on ISO fields.

Use Cases

I think we're straying away from the use cases Justin listed in #1084. Let's look at how we cover the use cases with this solution:

Top Priority (optimize ergonomics for these)

  1. Setting fields from a single Temporal object (esp. PlainDate, PlainTime)
  2. Setting fields from a single string
  3. Setting one or more date and time fields (without any timezone and/or calendar in input)

3 is covered. 2 is only covered if we specifically allow strings.

1 is NOT covered. There is no viable workaround, either. If you call .getFields() on a PlainDate or PlainTime, you get an object with a calendar or timeZone property. Without any other options, users might be tempted to delete the calendar field from the return value of getFields(), which is a very risky outcome.

Next Priority (must support, but OK if ergonomics isn't perfect)

  1. Using a property bag to set all fields from a Temporal object, esp all date fields, all time fields, or all date/time fields (for ZDT only). This is (1), but using property bags.
  2. Setting fields from a single userland ZonedTime or ZonedDate object

There is no nice way to cover 4. If I want to set the time of my PlainDateTime to 12:00 noon, with this proposal, I will need to write one of these monstrosities:

// Full property bag
plainDateTime.with({ hour: 12, minute: 0, second: 0, millisecond: 0, microsecond: 0, nanosecond: 0 })

// Hack with getFields()
const noon = Temporal.Time.from("12:00").getFields();
delete noon.calendar;  // OUCH
plainDateTime.with(noon);

There is no solution for 5, either. Everything is manual.

Low Priority (if needed, can move developers to other APIs for these cases)

  1. Setting all time properties and some date properties, e.g. { day: 1, ...time.getFields() }

Same problems as above: you can't spread time.getFields() without deleting the calendar.

Not a Priority (these are use cases that are actively bad)

  1. Setting partial date or partial time in a property bag with a time zone and/or calendar, e.g. { month: 2, calendar: 'islamic' } where the result is guaranteed to be confusing and is at best an approximation of the right answer.
  2. Setting timezone and/or calendar using with. We have withTimeZone and withCalendar for this purpose.
  3. Applying defaults to missing units in the property bag syntax. {hour: 12} shouldn't set minutes, seconds, etc. to zero. If the user wants this behavior, use a string or Time.from.

These are fine.

Inverting call sites to cover use case 1

Just a thought exercise: since the proposal explicitly does not cover use case 1, can we invert the call site?

// Proposal from #1085
plainDateTime.withPlainDate(plainDate)
zonedDateTime.withPlainDate(plainDate)

// Inverted call site
plainDate.toPlainDateTime(plainDateTime.getTime())
plainDate.toZonedDateTime({ time: plainDateTime.getTime(), timeZone: plainDateTime.getTimeZone() });

Is there another way to write that code that's any more concise?

Caveats: