Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

size-suggestions: Consider removing all toXxx conversion functions #2848

Closed
justingrant opened this issue May 20, 2024 · 11 comments
Closed

size-suggestions: Consider removing all toXxx conversion functions #2848

justingrant opened this issue May 20, 2024 · 11 comments
Assignees
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@justingrant
Copy link
Collaborator

justingrant commented May 20, 2024

Temporal contains 21 prototype methods that allow ergonomic conversion between one Temporal type and another type. Should we consider removing these methods below, and instead require callers to use from?

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.

To understand the impact of this change, here's a representative use case that illustrates both subtractive and additive conversions: "For a particular date and time in my local time zone, what is the date in Tokyo?"

// current
const tokyoDate = Temporal.PlainDateTime.from('2020-05-20T11:35')
  .toZonedDateTime(Temporal.Now.timeZoneId())
  .withTimeZone('Asia/Tokyo')
  .toPlainDate();
// => 2020-05-21

// proposed
const pdt = Temporal.PlainDateTime.from('2020-05-20T11:35');
// get pdt fields by manual deconstruction
  const { year, month, day, hour, minute, second, millisecond, microsecond, nanosecond } = pdt;
  const localFields = { year, month, day, hour, minute, second, millisecond, microsecond, nanosecond };
// ...or by mapping (there really should be an easier way!)
  const localFields = Object.fromEntries(Object.entries(pdt.getISOFields()).map(([name, value]) =>
    [name.replace(/^iso([A-Z])/, (_, first) => first.toLowerCase()), value]
  ));
const tokyoDate = Temporal.PlainDate.from(
  Temporal.ZonedDateTime.from({ timeZone: Temporal.Now.timeZoneId(), ...localFields })
  .withTimeZone('Asia/Tokyo')
);
// => 2020-05-21

Here's the list of methods.

Temporal.Instant.prototype.toZonedDateTimeISO ( timeZone )
Temporal.Instant.prototype.toZonedDateTime ( item )
Temporal.PlainDate.prototype.toPlainDateTime ( [ temporalTime ] )
Temporal.PlainDate.prototype.toPlainMonthDay ( )
Temporal.PlainDate.prototype.toPlainYearMonth ( )
Temporal.PlainDate.prototype.toZonedDateTime ( item )
Temporal.PlainDateTime.prototype.toPlainDate ( )
Temporal.PlainDateTime.prototype.toPlainMonthDay ( )
Temporal.PlainDateTime.prototype.toPlainTime ( )
Temporal.PlainDateTime.prototype.toPlainYearMonth ( )
Temporal.PlainDateTime.prototype.toZonedDateTime ( temporalTimeZoneLike [ , options ] )
Temporal.PlainMonthDay.prototype.toPlainDate ( item )
Temporal.PlainTime.prototype.toPlainDateTime ( temporalDate )
Temporal.PlainTime.prototype.toZonedDateTime ( item )
Temporal.PlainYearMonth.prototype.toPlainDate ( item )
Temporal.ZonedDateTime.prototype.toInstant ( )
Temporal.ZonedDateTime.prototype.toPlainDate ( )
Temporal.ZonedDateTime.prototype.toPlainDateTime ( )
Temporal.ZonedDateTime.prototype.toPlainMonthDay ( )
Temporal.ZonedDateTime.prototype.toPlainTime ( )
Temporal.ZonedDateTime.prototype.toPlainYearMonth ( )

@gibson042 gibson042 added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 20, 2024
@FrankYFTang
Copy link
Contributor

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

As what we have in the spec today, if we do not change our proposal, is there functionality that

 X.from(t, opt)

cannot produce but

t.toX(opt) 

can?

@FrankYFTang
Copy link
Contributor

I simply do not understand why we need both a X.from and a toX methods in the propoal. And if these two right now provide different functionality, that will be even more confusing and difficult for developers to choose which one to use.

@gibson042
Copy link
Collaborator

Developers will choose based on the value they're starting from—like document.querySelectorAll(selector).forEach(…) vs. Array.from(document.querySelectorAll(selector)).map(…).forEach(…).

@justingrant
Copy link
Collaborator Author

The toXxx methods provide chainable, concise, very-easy-to-discover (especially via IDE & browser dev-tools autocomplete) ways to perform one of the most common operations in Temporal, which is converting one Temporal type to another, either "narrowing" conversions like ZonedDateTime=>PlainDate or "expanding" conversions like Instant => ZonedDateTime.

Having used the Temporal API intensively for several years, I've found find that these conversions are very, very common. It's rare for a non-trivial Temporal use case that doesn't do any conversion.

The code sample I added to the OP (and that @gibson042 edited a bit) is a good illustration in the difference in usability, complexity, and code size between using chainable toXxx conversions and needing to use from.

Another advantage of the toXxx conversions are that they don't lose the calendar, because most developers will not think to copy the calendar from one object to another because most developers are unaware of non-ISO calendars.

That said, some conversion paths are more important than others. In particular, some conversions (like ZDT or PDT => PlainMonthDay or PlainYearMonth) are both uncommonly used and have easy two-step chained workarounds, so I'd be OK to remove the following conversions:

  • Temporal.PlainDateTime.prototype.toPlainMonthDay ( ) - replaced with pdt.toPlainDate().toPlainMonthDay()
  • Temporal.PlainDateTime.prototype.toPlainYearMonth ( ) - replaced with pdt.toPlainDate().toPlainYearMonth()
  • Temporal.ZonedDateTime.prototype.toPlainMonthDay ( ) - replaced with zdt.toPlainDate().toPlainMonthDay()
  • Temporal.ZonedDateTime.prototype.toPlainYearMonth ( ) - replaced with zdt.toPlainDate().toPlainYearMonth()

I would also be willing to discuss removing these two conversions because they've always been ergonomically awkward. Also, because both of them only transfer two properties (three if there's a calendar), the alternative of using from isn't as bad as all other conversions, like PDT where conversions can avoid writing up to 9 properties!

  • Temporal.PlainMonthDay.prototype.toPlainDate ( item )
  • Temporal.PlainYearMonth.prototype.toPlainDate ( item )

Also, the PlainDate => wider type conversion is much more commonly needed than the PlainTime => wider type conversion, so we could perhaps remove these two conversions by pushing users to convert from dates => wider by adding times, instead of from times => wider by adding dates. This would remove two more functions:

  • Temporal.PlainTime.prototype.toZonedDateTime ( item ) - replaced by item.plainDate.toZonedDateTime({plainTime, item.timeZone}
  • Temporal.PlainTime.prototype.toPlainDateTime ( temporalDate ) - replaced by calling temporalDate.toPlainDateTime

Finally, I'd be OK to remove Temporal.Instant.prototype.toZonedDateTime ( item ) and rename toZonedDateTimeISO=>toZonedDateTime. The replacement code for non-ISO calendars would be a chainable set of two calls: first call toZonedDateTime to get the ISO calendar value, then call .withCalendar(calendar) on the result. If we did this, we'd also want to change Temporal.PlainDate.prototype.toZonedDateTime from accepting a {timeZone, plainTime?} object to instead accept positional parameters of (timeZone, plainTime?).

The suggestions here in this comment would remove 9 of the 21 conversion methods, but without significantly degrading the large usability and IDE-discoverability benefit that the remaining 12 chainable conversions below provide:

  • Temporal.Instant.prototype.toZonedDateTime ( timeZone )
  • Temporal.PlainDate.prototype.toPlainDateTime ( [ temporalTime ] )
  • Temporal.PlainDate.prototype.toPlainMonthDay ( )
  • Temporal.PlainDate.prototype.toPlainYearMonth ( )
  • Temporal.PlainDate.prototype.toZonedDateTime ( timeZone [, temporalTime ] )
  • Temporal.PlainDateTime.prototype.toPlainDate ( )
  • Temporal.PlainDateTime.prototype.toPlainTime ( )
  • Temporal.PlainDateTime.prototype.toZonedDateTime ( timeZone [ , options ] )
  • Temporal.ZonedDateTime.prototype.toInstant ( )
  • Temporal.ZonedDateTime.prototype.toPlainDate ( )
  • Temporal.ZonedDateTime.prototype.toPlainDateTime ( )
  • Temporal.ZonedDateTime.prototype.toPlainTime ( )

I would be strongly against removing these 12 conversions.

@FrankYFTang
Copy link
Contributor

Here are all the toX in ECMA262

$ egrep "<h1" ecma262/spec.html|egrep -v "Properties|Constructor|Object|\. \. \.|range|@@toStringTag"|cut  -d'>' -f2|cut -d'<' -f1|sort -u|egrep  -v "(constructor|prototype)$" |egrep "\(|\.|get "|egrep -v ":|Operator|Functions|@@" |egrep "\.to"
Array.prototype.toLocaleString ( [ _reserved1_ [ , _reserved2_ ] ] )
Array.prototype.toReversed ( )
Array.prototype.toSorted ( _comparefn_ )
Array.prototype.toSpliced ( _start_, _skipCount_, ..._items_ )
Array.prototype.toString ( )
BigInt.prototype.toLocaleString ( [ _reserved1_ [ , _reserved2_ ] ] )
BigInt.prototype.toString ( [ _radix_ ] )
Boolean.prototype.toString ( )
Date.prototype.toDateString ( )
Date.prototype.toGMTString ( )
Date.prototype.toISOString ( )
Date.prototype.toJSON ( _key_ )
Date.prototype.toLocaleDateString ( [ _reserved1_ [ , _reserved2_ ] ] )
Date.prototype.toLocaleString ( [ _reserved1_ [ , _reserved2_ ] ] )
Date.prototype.toLocaleTimeString ( [ _reserved1_ [ , _reserved2_ ] ] )
Date.prototype.toString ( )
Date.prototype.toTimeString ( )
Date.prototype.toUTCString ( )
Error.prototype.toString ( )
Function.prototype.toString ( )
Number.prototype.toExponential ( _fractionDigits_ )
Number.prototype.toFixed ( _fractionDigits_ )
Number.prototype.toLocaleString ( [ _reserved1_ [ , _reserved2_ ] ] )
Number.prototype.toPrecision ( _precision_ )
Number.prototype.toString ( [ _radix_ ] )
RegExp.prototype.toString ( )
String.prototype.toLocaleLowerCase ( [ _reserved1_ [ , _reserved2_ ] ] )
String.prototype.toLocaleUpperCase ( [ _reserved1_ [ , _reserved2_ ] ] )
String.prototype.toLowerCase ( )
String.prototype.toString ( )
String.prototype.toUpperCase ( )
String.prototype.toWellFormed ( )
Symbol.prototype.toString ( )
Symbol.toPrimitive
Symbol.toStringTag
%TypedArray%.prototype.toLocaleString ( [ _reserved1_ [ , _reserved2_ ] ] )
%TypedArray%.prototype.toReversed ( )
%TypedArray%.prototype.toSorted ( _comparefn_ )
%TypedArray%.prototype.toString ( )

Most of them are toString and toLocaleString. I do not think inside the current ECMA262 , we has a precedent of the toDataType() for naming convention, right?

@sffc
Copy link
Collaborator

sffc commented May 20, 2024

It seems like a good idea to create multiple slates of these toXxx functions so that we can discuss them separately. The impact on developer ergonomics is not the same across all of the proposed functions.

@ljharb
Copy link
Member

ljharb commented May 21, 2024

"toString" and "toLocaleString" are an existing "toDataType" naming convention.

@justingrant
Copy link
Collaborator Author

justingrant commented May 24, 2024

Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose removing the following conversion methods:

  • PMD/PYM direct conversions from PDT/ZDT, replaced with two-method chains via PlainDate
    • PlainDateTime.p.toPlainMonthDay - replaced with pdt.toPlainDate().toPlainMonthDay()
    • PlainDateTime.p.toPlainYearMonth - replaced with pdt.toPlainDate().toPlainYearMonth()
    • ZonedDateTime.p.toPlainMonthDay - replaced with zdt.toPlainDate().toPlainMonthDay()
    • ZonedDateTime.p.toPlainYearMonth - replaced with zdt.toPlainDate().toPlainYearMonth()
  • PlainTime => PDT/ZDT conversions, replaced by going through PlainDate instead:
    • PlainTime.p.toZonedDateTime( item ) - replaced by item.plainDate.toZonedDateTime(timeZone | {plainTime, timeZone})
    • PlainTime.p.toPlainDateTime ( temporalDate ) - replaced by temporalDate.toPlainDateTime(plainTime)
  • Remove Temporal.Instant.p.toZonedDateTime ( item ) as part of size-suggestion: Consider merging Temporal.Now.xISO() and Temporal.Now.x() #2846. Please see that issue for details.

We think that this is OK for the following reasons:

  • PMD/PYM have long been acknowledged to be "weird" types that don't need the same level of ergonomics as other types, and the workarounds are easily discoverable.
  • PlainTime + PlainDate combinations can be done either way, and using the PlainDate as the receiver is a more common use case, so we'll keep that one and the less common case can swap receiver and argument to achieve the same result.

@sffc
Copy link
Collaborator

sffc commented May 24, 2024

Can we remove all the functions that can be done via chaining?

For example, date.toZonedDateTime({ plainTime, timeZone }) can be written as date.toPlainDateTime(plainTime).toZonedDateTime(timeZone)

@justingrant
Copy link
Collaborator Author

justingrant commented May 24, 2024

We can do anything. :-). But my strong opinion is that we should NOT remove the Date=>ZDT 1-step conversion path.

TL;DR - we want more developers going direct from PlainDate=>ZDT without going through PDT in the middle, to prevent DST bugs across the ecosystem.

Details are below:

First, some context: most date-time data in real-world business workflows (and therefore software) are dates (aka PlainDate), times (PlainTime), and "when exactly something happened" which can be timezone-irrelevant (Instant) or timezone-relevant (ZDT). PDT is less common than these 4.

Furthermore, much of the usage of PDT-like data in computing is probably incorrectly ignoring the time zone and DST, because it is vulnerable to DST bugs.

So in general I don't think we should be encouraging users to use PDT unless they specifically opt into using it. The "easy path" should nudge users to get a DST-safe result from a PlainDate value.

WIth that context in mind, here's a few reasons to leave this conversion in place:

  • Users who don't know any better might stop at PDT, which is dangerous because it seems like the right type to a novice developer, but most of those cases really should have a timezone attached to avoid DST bugs.
  • PlainDate.p.toZonedDateTime is an IDE-autocomplete hint that you can go direct from PD to ZDT. Without that, you'd need to teach users about both PDT and ZDT and the conversions required to make the jump. That's a lot of learning just to get a timezone-safe date.
  • PD=>ZDT and Instant=>ZDT will be the most common ZDT conversions, so ergonomics and discoverability matter more for those than others.
  • We don't really want developers using PDT "accidentally" without explicitly opting into it.
  • PDT will usually be Temporal's most expensive type to construct and store and it has the most internal slots. It'd be a bummer to have to pay to construct an unnecessary PDT for a very common conversion path.

@ptomato
Copy link
Collaborator

ptomato commented May 27, 2024

There is an implementation of the conclusion documented in #2848 (comment) available: here and here.

@ptomato ptomato self-assigned this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns
Projects
None yet
Development

No branches or pull requests

6 participants