Proposal-temporal: Naming of Temporal.Duration fields

[sffc]

In Intl.DateTimeFormat, Intl.NumberFormat, Intl.DisplayNames, and even Temporal.DateTime, the date/time fields are singular: "second", "minute", "hour", "day", etc.

However, in Temporal.Duration, they are plural: "seconds", "minutes", etc.

I understand the reasoning, that it sounds more natural in English to say "seconds: 30" instead of "second: 30", but there are some downsides to the inconsistent naming:

1. Users may want to use the same key as input to both Temporal.Duration and one of the other classes
2. Non-native English speakers who don't care about language fluency have to learn to use singular in some cases and plural in others

Thoughts on making the Temporal.Duration field names singular?

[ljharb]

Alternatively, could they all be made plural? Singular only makes sense in english with a value of 1.

[sffc]

If we were doing this new, I'd say they should all be plural, but at least Intl.DateTimeFormat is already stable and uses singular names.

[gibson042]

This was the first open question of #120, so I guess not everything there was dealt with before it closed (and the rest should probably be audited as well). But as for this specific issue, I'm really not sure if it makes sense for Duration to be different... @sffc, can you provide an example where an author would want to use the same key as input to both Temporal.Duration and one of the other classes?

[littledan]

For RelativeTimeFormat, we decided to permit both singular and plural. In my personal opinion, plural makes sense here.

[ptomato]

Would anyone still like to discuss changing the field names or can we close this?

[sffc]

Maybe we could just accept both singular and plural everywhere, on input and output. Then you can't make a typo because both forms work the same.

[ryzokuken]

I too agree that we should use plural. I'm not convinced that we should allow both, as opposed to emitting better errors and better documentation.

[littledan]

The idea from here will be to ship the polyfill accepting just the plural form, and see if we get feedback asking for the single form. We'll revisit this issue before Stage 3.

[ptomato]

615 is feedback on this issue.

[justingrant]

Having different singular vs. plural is actually very helpful, because it lets Temporal throw exceptions when a developer passes a DateTime/Date/Time/etc instead of a Duration, or vice versa. Of course, Temporal would actually have to start throwing those errors which was my suggestion in #615.

Suggestion: can Temporal throw in cases when a singular field name is expected but the plural variant is supplied, or vice versa? Or when an XxxxLike object is passed to a Temporal object constructor?

The only thing I think we _shouldn't_ do is ship the current behavior, where only plurals are accepted but no exceptions are thrown if you pass the wrong type, so you get zeroed-out values without any complaints from Temporal. I wasted 10 minutes the other day wondering why I couldn't add minute: 10 before I noticed the missing "s". ;-)

Another benefit of different naming is that TypeScript users will catch the "pass wrong type" problem in the compiler or even right in the IDE, without having to wait to trigger the exception at runtime. So once we have TS types, my minute: 10 bug above would be an obvious in-IDE error.

[sffc]

We don't throw in 402 if there are unknown options in the options bag. There are pros and cons of doing it either way. However, the 402 options bag carries options, not data, which is an important distinction when we go about extending the options bags.

If we did throw here, would we specifically blacklist certain common misspellings, like "minute" instead of "minutes", or would we throw for any unrecognized option?

[justingrant]

I don't have enough context to know if there are valid use-cases for providing unknown field names in property bags.

My top priority would be to make sure that the plural vs. singular cases are caught because those errors will be painfully common.

We'd need to make sure that custom fields added by non-ISO calendars don't get flagged as typos.

[justingrant]

I thought about this more. I think passing "extra" properties seems relatively harmless (other than the obvious singular vs. plural cases above which I think should be blacklisted).

But passing a non-empty object with zero matching properties seems like it's almost guaranteed to be a mistake, e.g. passing a Time-like property bag when a Date-like is expected, or passing an Absolute when a DateTime property bag is expected. I've already written code with both of these bugs and they were frustrating to track down.

Should Temporal throw if zero fields in the bag match a whitelist of expected fields, including custom calendar fields? And if yes, would we be able to prevent throwing for {} in contexts where {} might be valid, like duration.with({}, {disambiguation: 'balance'})?

[justingrant]

Update: looks like Duration.prototype.with in the polyfill throws for {} and {foo: 1}, while other Duration methods don't throw. Seems like there should be consistent behavior unless there's a reason not to.

duration = Temporal.Duration.from('PT10H54M')
duration.plus({}).toString()
// => "PT10H54M"
duration.minus({}, {disambiguation: 'balance'}).toString()
// => "PT10H54M"
Temporal.Duration.from({}).toString()
// => "PT0S"
duration.with({}, {disambiguation: 'balance'}).toString()
// throws: Uncaught RangeError: invalid duration-like

[ptomato]

The reason was that from() should have the same optional/required semantics as the constructor arguments (and in the case of Temporal.Duration, all constructor arguments are optional) whereas in with(), it's probably an error if you passed an empty object.

I think rather than duration.with({}, {disambiguation: 'balance'}) a better way would be Temporal.Duration.from(duration, {disambiguation: 'balance'}).

[sffc]

Related: does it matter if the misspelled properties exist on the prototype but aren't owned properties?

const a = { hours: 2, minute: 30 };
const b = Object.create(a);
const d = Temporal.Duration.from(b);  // what should this be?

[justingrant]

@ptomato whereas in with(), it's probably an error if you passed an empty object.

Why is it an error for with but not for plus and minus?

I think rather than duration.with({}, {disambiguation: 'balance'}) a better way would be Temporal.Duration.from(duration, {disambiguation: 'balance'}).

The challenge is that you need to go back to the start of a chain of methods to insert Temporal.Duration.from:

• Ergonomics: it's many more keystrokes, including both the method call and the the cursor movement to go to the start of the chain and back again to insert the options bag.
• If a method chain is long, it can be hard to figure out exactly where to cut the chain to insert the Temporal.Duration.from, especially if the chain is interleaved with non-Duration methods.
• When patching existing code, adding Temporal.Duration.from requires inserting code in two places, instead of appending to a chain which puts all new code in one place. Chaining -> cleaner patches.
• With from, IDE users can't use autocomplete to discover the next chained method to use. With with, it's easier: the user can scroll through autocomplete until they find the correct method to use.

All that said, I think _both_ with and from are bad if all that's needed is balancing. And the current chainable solution, .minus({}, {disambiguation: 'balance'}, feels like an awful hack. IMHO a better solution (discussing in #645) could be to make balancing easier & more consistent so that there'd never be a need to pass an empty object with, plus, or minus so all three could throw.

[justingrant]

@sffc Related: does it matter if the misspelled properties exist on the prototype but aren't owned properties?

Non-owned properties for property bag objects are a pretty advanced use case that almost all developers will not use, so I don't have a strong opinion on this one way or the other.

[gibson042]

I would propose throwing an error when both no expected fields are present _and_ one or more cross-type fields _are_ present, and I don't think own vs. prototype position should matter:

1. Let _result_ be a new Record with all the internal slots given in the Internal Slot column in \\.
2. Let _empty_ be true.
3. For each row of \\, except the header row, in table order, do
   
   
   
   1. Let _slotName_ be the Internal Slot value of the current row.
   
   
   2. Let _fieldName_ be the Property value of the current row.
   
   
   3. If ? HasProperty(_temporalDurationLike_, _fieldName_) is true, then
      
      
      
      
      
      1. Set _empty_ to false.
      
      
      
      2. Let _val_ be ? Get(_temporalDurationLike_, _fieldName_).
      
      
      
      3. Set _result_.[[\<_slotName_>]] to ? ToInteger(_val_).
      
      
      
   
   
4. If _empty_ is true, then
   
   
   
   1. For each row of \\, except the header row, in table order, do
      
      
      
      
      
      1. Let _fieldName_ be the Property value of the current row.
      
      
      
      2. If ? HasProperty(_temporalDurationLike_, _fieldName_) is true, then
         
         
         
         
         
         
         
         1. Throw a TypeError exception.
         
         
         
         
      
      
      
   
   

This way Temporal.Duration.from({}) will generate a zero duration but Temporal.Duration.from({hour: 1}) will fail.

[justingrant]

@gibson042 I would propose throwing an error when both no expected fields are present _and_ one or more cross-type fields _are_ present, and I don't think own vs. prototype position should matter

What about Temporal.Duration.from({hours: 1, minute: 1})? My inclination would be to throw in that case.

[gibson042]

Hmm, so that would be more like "throw if you see any Temporal.DateTime fields"? Yeah, that can work. But I don't think I'd go pairwise, where Temporal.Duration.from({hours: 1, minutes: 1, minute: 0}) doesn't throw.

[justingrant]

But I don't think I'd go pairwise, where Temporal.Duration.from({hours: 1, minutes: 1, minute: 0}) doesn't throw.

Sounds good to me, although that seems like an obscure case so I think it'd also be OK to throw in the case you listed, which might make the blacklist easier for browsers to implement. But I think either way would be OK as long as the common single-typo case (Temporal.Duration.from({hours: 1, minute: 1})) throws.

[sffc]

Did we eliminate adopting the Intl.RelativeTimeFormat behavior of allowing the keys to be either singular or plural? I only see this comment from @justingrant, which is valid, but I don't know if it eliminates the option:

Having different singular vs. plural is actually very helpful, because it lets Temporal throw exceptions when a developer passes a DateTime/Date/Time/etc instead of a Duration, or vice versa. Of course, Temporal would actually have to start throwing those errors which was my suggestion in #615.

---

Non-owned properties for property bag objects are a pretty advanced use case that almost all developers will not use, so I don't have a strong opinion on this one way or the other.

No developer would actually write the code I posted, but checking for properties up the prototype chain is absolutely something that can and will happen in the real world. For example, maybe someone writes a third-party duration class for interacting with their database. The fields like "years" and "days" might be getters living on the prototype.

---

If we go with the rejection approach, do we scope the problem to singular/non-singular, or do we also handle misspellings, like hors or minnutes or wekks?

[justingrant]

If we go with the rejection approach, do we scope the problem to singular/non-singular, or do we also handle misspellings, like hors or minnutes or wekks?

The former only. The goal isn't to catch every typo, it's to avoid cases where developers accidentally/mistakenly use one set of fields when they should be using the other.

[justingrant]

My current opinion:

• Accept both singular and plural everywhere units are accepted, like Intl.RelativeTimeFormat https://github.com/tc39/proposal-temporal/issues/325#issuecomment-638557552
• Emit (and document!) plural for duration fields and singular everywhere else
• Temporal methods should always throw if a Duration instance is passed where a non-Duration object is expected, or if a non-Duration instance is passed where a DurationLike object is expected. This might already be the current behavior; not sure.

[ptomato]

@justingrant How do you feel about the TypeScript bindings? Should they accept both? My feeling is that they should not.

I agree with what you proposed there. It does leave a weird case where you can do
dateTime.plus(otherDateTime.getFields()) or dateTime.difference(duration.getFields()) but as far as I can tell, there is no way to prevent that and still support both singular and plural fields.

[justingrant]

How do you feel about the TypeScript bindings? Should they accept both? My feeling is that they should not.

@ptomato Hmm, good question. The more I researched the TS implications, it's made me open to changing my opinion about what we should do here. The problem is that TS doesn't seem to have a concept of "allowed but not preferred". AFAIK there's no ability to control, for example, whether the TS compiler emits a warning or an error. Nor does there seem to be a way in VSCode to prevent it from suggesting all allowed variants of a property name. So if you want to prevent date.plus({hour: 1}) from causing a TS compiler error, then I don't think you can also prevent VSCode from showing both variants to the user in autocomplete:

I asked a question in Stack Overflow to see if there's a way to hide the "wrong" variant from autocomplete but not trigger a compiler error either if the wrong variant is used. https://stackoverflow.com/questions/63837660/in-typescript-and-or-jsdoc-how-to-indicate-that-some-property-names-in-a-record

If we go the route that you're recommending and TS will show a compiler error if the wrong variants are used, is this OK? If we do this then should we just prohibit the wrong variants at runtime too so that runtime and TS won't disagree on what's valid code?

I agree with what you proposed there. It does leave a weird case where you can do
dateTime.plus(otherDateTime.getFields()) or dateTime.difference(duration.getFields()) but as far as I can tell, there is no way to prevent that and still support both singular and plural fields.

There is one case I think this would be a feature not a bug: converting Duration to Time and vice versa, which is a fairly common use case.

const time = Temporal.Time.from('00:15');
const duration = Temporal.Duration.from(time.getFields());
const timeRoundTrip = Temporal.Time.from(duration.getFields());

[ptomato]

I think it's OK for compile time and runtime to disagree as long as compile time is stricter. (But as I'm not an experienced TypeScripter I don't know if this is usually done differently.)

I can think of another example where this happens. For example in Temporal.Date.from, the type of the first argument is Temporal.Date | DateLike | string, but at runtime it is actually any. If you pass a boolean, number, symbol, bigint, etc., it's converted to a string and then the string is parsed as an ISO string. I don't think it would be useful for the type to actually be any in the .d.ts file.

I don't think it would be a great loss if the TypeScript compiler were to flag the conversion between Duration.from(time) and Time.from(duration)`. What are the use cases you had in mind? I can only think of ones where it seems like the Time should already have been a Duration in the first place.

[justingrant]

I'm mostly worried about the case where valid JS code breaks for a mainstream use case when it's run through the TS compiler. If people get used to being lazy about pluralization, it will put us in a bind: do we expand the TS types to include the "wrong" variants (and degrade autocomplete usability) or do we accept that lots of valid JS code won't be valid when run through the TS compiler.

So I guess it depends on whether we think a lot of users will start depending on this behavior. It's a slippery slope, and the hard part is that if people start relying on the "wrong" variants then there's no good way to put the TS horse back in the barn.

So now I'm back to my previous opinion, rolling up other feedback above too:

1. In methods accepting a duration, throw if any property name matches a singular unit name
2. In methods accepting a duration, throw if a non-Duration temporal type instance is provided.
3. In methods accepting a non-duration, throw if any property name matches a plural unit name.
4. In methods accepting a non-duration, throw if a Duration temporal type instance is provided.
5. Only use a fixed list of known units for the ISO calendar. Non-ISO fields are exempted from these checks.
6. Don't bother with any other checks (mis-spelling, etc.)
7. Handle non-owned properties however we think will be best, which per @sffc above I think means that the checks above should include non-owned properties.

[justingrant]

Decision 2020-09-11:

Property Names

• All methods in Temporal that accept a TimeLike or a DurationLike property bag must include at least one correctly-spelled property.
• Empty objects will throw, as will objects that only contain the wrong pluralization and/or mis-spelling.
• Cases like {years: 1, hour: 1} won't throw because it's obscure and obvious to solve via debugging.

Unit names in options values (e.g., smallestUnit)

• Allow both variants.
• Use JSDoc @deprecated for the "wrong" unit (the one differing from the type's field name) in TypeScript types.
• Ideally, we'd have an ESLint rule to catch the "wrong" variant, but that's out of scope.

[ptomato]

As far as I can tell, the APIs affected by this change are:

• from, with, add, subtract of Date, DateTime, Duration, Time, YearMonth
• Temporal.Duration.prototype.round (via relativeTo)
• Temporal.MonthDay.from
• Temporal.MonthDay.prototype.with
• Temporal.Instant.prototype.add
• Temporal.Instant.prototype.subtract

I'd also propose disallowing "week" since there is no property named that anywhere.