tc39 / proposal-temporal

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

size-suggestion: Fold some `withX()` functions into `with()` functions #2847

Closed justingrant closed 4 months ago

justingrant commented 5 months ago

PlainDate, PlainDateTime, and ZonedDateTime have with() functions as well as withPlainTime, withPlainDate, withCalendar, and withTiimeZone functions, each expecting a different kind of input. Should we consider only having with?

This issue is adapted from suggestions made by @FrankYFTang to help reduce the size of Temporal to address concerns from implementers (especially Android and Apple Watch) that Temporal's install size and/or in-memory size is too large.

There are three cases to consider:

ptomato commented 5 months ago

Yeah, these are just not redundant with each other even if they are similarly named.

I'd be willing to reluctantly consider removing withPlainDate but everything else is a hard no for me.

FrankYFTang commented 5 months ago

My origional suggestion could be found in https://docs.google.com/document/d/16Rd43XB8z6iLC2c9AfF--unDFg_t476tI1PFe7PbbcM/

would be very confusing for callers because it's not clear if the result should be This could be addressed by an additional parameter in the option bag to indicate what kind of the variant of the with is instead of by adding a new prototype function, so instead of

a.withCalendar(x)
b.withTimeZone(y)

you can have

a.with(x, {what: 'calendar'})
b.with(y, {what: 'timeZone'})

where what need to be a better name to indicate what kind of object the with() is, just as what was indicated by the suffix in the method.

justingrant commented 5 months ago

I'd be willing to reluctantly consider removing withPlainDate but everything else is a hard no for me.

Agreed.

you can have

a.with(x, {what: 'calendar'})
b.with(y, {what: 'timeZone'})

We could do something like that (or require that with either includes only a time zone, only a calendar, or neither of these, but either of these options would be a lot less discoverable than the current API.

sffc commented 5 months ago

From a readability and ergonomics point of view, this seems harmless:

// Current:
.withPlainDate("2024-05-20")
.withPlainTime("00:00")

// Proposed:
.with(Temporal.PlainDate.from("2024-05-20"))
.with(Temporal.PlainTime.from("00:00"))

// Alternative:
.with({ date: "2024-05-20" })
.with({ time: "00:00" })

As far as withCalendar and withTimeZone, I do remember litigating the ordering question a lot, which is why we decided to have separate functions. I wouldn't be opposed to merging them back into one function but throwing an exception if you mix types of things.

// Current:
.withTimeZone("America/Chicago")
.withCalendar("buddhist")

// Proposed:
.with(Temporal.TimeZone.from("America/Chicago"))
.with(Temporal.Calendar.from("buddhist"))

// Alternative:
.with({ timeZone: "America/Chicago" })
.with({ calendar: "buddhist" })

All of the following would just throw an exception:

.with({ date: "2024-05-20", monthCode: "M06" })
.with({ time: "00:00", hour: 12 })
.with({ timeZone: "America/Chicago", hour: 12 })
.with({ calendar: "buddhist", monthCode: "M05" })
justingrant commented 5 months ago

merging them back into one function but throwing an exception if you mix types of things.

We could do this, but it would make a confusing API even more confusing. The current with* family of APIs took months to hash out and the final result is very IDE-discoverable and makes it really obvious to users why you can't put a calendar or a time zone in your with call: because there are dedicated APIs sitting right in your autocomplete list!

The exception approach would move this discoverability to runtime and would generate data-driven exceptions that we try to avoid Temporal-wide.

So I would not be inclined to support this change.

gilmoreorless commented 5 months ago

From a readability and ergonomics point of view, this seems harmless:

// Current:
.withPlainDate("2024-05-20")
.withPlainTime("00:00")

// Proposed:
.with(Temporal.PlainDate.from("2024-05-20"))
.with(Temporal.PlainTime.from("00:00"))

// Alternative:
.with({ date: "2024-05-20" })
.with({ time: "00:00" })

I think it's worth re-reading the arguments put forward in #592, where the current string syntax was proposed and discussed. .with(Temporal.PlainTime.from("00:00")) may look fine on its own, but is a big step backwards in terms of readability within the context of a lot of other API calls in a codebase.

It's also worth remembering this comment from @justingrant:

This small-seeming change may be the biggest ergonomic improvement we could have made in Temporal and it addresses the #1 feedback item: more brevity.

(Yes, I'm biased as the one who proposed the shortcut string arguments, but AFAIK Justin's comments still stand.)

justingrant commented 5 months ago

I agree with @gilmoreorless, especially with regards to this pattern .withPlainTime("00:00") that I've found to be very, very common in real-world Temporal code.

I think we could, if we need to, live without .withPlainDate("2024-05-20"), both because it's less common and because you can always go in the reverse direction and add a time to a date-having receiver instead of the other direction.

sffc commented 5 months ago

Based on today's discussion I agree that we shouldn't allow .with({ date: "2024-05-20" }) or .with({ time: "00:00" }) because they allow passing conflicting options into the options bag.

However, I'm not convinced that .withPlainDate("2024-05-20") is substantially better than .with(Temporal.PlainDate.from("2024-05-20")) (or even .with(PlainDate.from("2024-05-20")) if you import all items in the Temporal namespace).

@ptomato pointed out that there is a potential calendar conflict, but the calendar conflict already exists in .withPlainDate and should be resolved in the same way. This seems like a non-issue.

This would allow us to remove the following functions:

justingrant commented 5 months ago

Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose removing ZonedDateTime.p.withPlainDate and PlainDateTime.p.withPlainDate.

We think that this is OK for the following reasons:

I'll open a new issue to discuss withPlainTime per @sffc's comment above.

ptomato commented 5 months ago

Removal of withPlainDate is implemented here.