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

Example where lack of object shape checks in ToTemporalTimeZone and ToCalendar can be confusing #2354

Closed anba closed 1 year ago

anba commented 2 years ago

Probably just a FYI, because the underlying issue was already reported in #1652, which in turn links to #300 and #1294.

I was recently writing this incorrect code:

var tz = new Temporal.TimeZone("UTC");
var cal = new Temporal.Calendar("iso8601");
var zdt = new Temporal.ZonedDateTime(0n, cal, tz);

timeZone and calendar were passed in the wrong order, which leads to an error only after some methods on zdt.[[TimeZone]] or zdt.[[Calendar]] are called:

var duration = Temporal.Duration.from("PT48H");

// Throws: TypeError: getOffsetNanosecondsFor property is not callable
console.log(duration.round({smallestUnit: "hours", relativeTo: zdt}).toString());

Directly catching this mistake when creating the ZonedDateTime would be nicer, but I'm not sure how to best detect this case, given that accepting any object for the time zone and calendar arguments was an intentional design choice.

ptomato commented 2 years ago

Yikes, that's an easy trap to fall into, for sure.

This is indeed an intentional design choice, but as I mentioned in the other issue: if this is causing problems in the implementation, especially with respect to optimizability, I think we should put it on the agenda to discuss in a Temporal champions meeting.

justingrant commented 2 years ago

Seems like a good thing to fix. Obvious bug that's easy to encounter.

ptomato commented 2 years ago

We had a bit of discussion on this in the Temporal champions meeting of 2022-10-13 and have a temperature check for some possible solutions:

  1. Do nothing — didn't sound to me like there was strong support for this, but it did sound like everyone could grudgingly live with it.
  2. Stricter check on object shapes at entry points where Temporal.TimeZone and Temporal.Calendar are passed in — we didn't do this in the past because these checks are observable, but if it's only HasProperty and not GetProperty, people seem OK with this. Some strong preference for it, others lukewarm. In the TimeZone case it's clear which checks should be done, but in the Calendar case it's less clear-cut.
  3. Use a bag with named arguments in the Temporal.ZonedDateTime constructor, instead of one required and one optional ordered argument — some strong preference for this, others think it decreases ergonomics of the common case in which no calendar is passed
  4. Throw when passing an object with [[InitializedTemporalCalendar]] to ToTemporalTimeZone and vice versa, accepting the potential for bugs when using (very rare, specialized) plain-object protocol implementations — some strong preference for this, others think it's a hack without precedent in the language

We'll discuss this again in the future.

ljharb commented 2 years ago

I continue to feel pretty strongly that required args should be positional whenever possible, which seems to conflict with option 3.

justingrant commented 2 years ago

Champions meeting 2022-10-27: We'll go with #4 above:

Throw when passing an object with [[InitializedTemporalCalendar]] to ToTemporalTimeZone and vice versa, accepting the potential for bugs when using (very rare, specialized) plain-object protocol implementations — some strong preference for this, others think it's a hack without precedent in the language