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-suggestion: Consider removing some epoch* getters and fromEpoch* methods #2849

Closed
justingrant opened this issue May 20, 2024 · 13 comments
Closed
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.Instant contains static fromEpochSeconds, fromEpochMilliseconds,fromEpochMicroseconds, and fromEpochNanoseconds methods to construct new instances (the latter of which duplicates the constructor's functionality), as well as epochSeconds, epochMilliseconds,epochMicroseconds, and epochNanoseconds getters. The same four getters are also present on Temporal.ZonedDateTime. Are all these needed?

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.

For context, here's a quick explanation of why each of these methods exist. Some of them are definitely higher priority than others. The higher-priority ones are noted in the first list:

  • Temporal.Instant ( _epochNanoseconds_ ) - obviously required because it's a constructor
  • Temporal.ZonedDateTime ( _epochNanoseconds_, _timeZoneLike_ [ , _calendarLike_ ] ) - obviously required because it's a constructor
  • get Temporal.Instant.prototype.epochMilliseconds - this is required for interop with legacy Date, i.e. new Date(instant.epochMilliseconds)
  • get Temporal.ZonedDateTime.prototype.epochMilliseconds - allows converting between ZDT and Date without having to convert to a Temporal.Instant first.
  • Temporal.Instant.fromEpochMilliseconds ( _epochMilliseconds_ ) - this is used for Date interop in the other direction: when you have a milliseconds value you got from a Date and want to turn it into a Temporal.Instant without having to convert it into a Date first so that you can call Date.p.toTemporalInstant().
  • get Temporal.Instant.prototype.epochNanoseconds - this exposes the BigInt value of the instant in its full resolution
  • Temporal.Instant.fromEpochNanoseconds ( _epochNanoseconds_ ) - Across Temporal, we try to train users to avoid constructors and instead use from methods because from methods accept a wider set of types and are more likely for the user to be successful. This method on Instant, although it duplicates the constructor, helps reinforce this pattern, and aligns with the other common way to construct an instant which is fromEpochMilliseconds.

The methods below for seconds and microseconds are admittedly lower priority, because they're not required for Date interop nor are required for full-resolution nanoseconds. We could consider deferring them for a few years until the added size of a few methods is less of a big deal for Android and Apple Watch:

  • Temporal.Instant.fromEpochMicroseconds ( _epochMicroseconds_ )
  • Temporal.Instant.fromEpochSeconds ( _epochSeconds_ )
  • get Temporal.Instant.prototype.epochMicroseconds
  • get Temporal.Instant.prototype.epochSeconds
  • get Temporal.ZonedDateTime.prototype.epochMicroseconds
  • get Temporal.ZonedDateTime.prototype.epochSeconds
@justingrant justingrant added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 20, 2024
@anba
Copy link
Contributor

anba commented May 20, 2024

The proposal mentions removing the milliseconds and microseconds functions, while keeping the seconds and nanoseconds functions. So a different set of functions than the ones written up here.

IMO milliseconds and nanoseconds functions are useful for the reasons outlined in #2849 (comment). But if we additionally keep the seconds functions, I don't see much reason to only omit the microseconds functions.

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2024

I'm neutral on this one. Probably microseconds and seconds aren't going to be used very much.

However, I disagree with the assertion given in the proposal that every developer knows that you can multiply or divide by 1000 to get from one to the other. I'd say that in fact many developers don't have the SI prefixes and their magnitudes at their fingertips, and Temporal should be accessible to those developers too.

@FrankYFTang
Copy link
Contributor

I'd say that in fact many developers don't have the SI prefixes and their magnitudes at their fingertips, and Temporal should be accessible to those developers too.

then for this group of develpers, how could they have Temporal at their fingertips?

If they do not know the relationship between seconds, milliseconds, microseconds, and nanoseconds, how could you expect they will call the right methods? The very same developer could call the microseconds one while they should call the nanoseconds one, right? and providing 4 will make their life even harder than only provideing then 1 or 2 since now they have to learn the differences between these four SI prefixes and their magnitudes which are not in their fingertips...

@ljharb
Copy link
Member

ljharb commented May 20, 2024

autocomplete can teach them of the presence of different methods, and grow their understanding, quite successfully - in that respect, more methods is an education improvement.

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2024

If they do not know the relationship between seconds, milliseconds, microseconds, and nanoseconds, how could you expect they will call the right methods?

They have microseconds from a data source, or need microseconds to put into another API, so they call the method that has "microseconds" in its name. That's how.

@justingrant
Copy link
Collaborator Author

Meeting 2024-05-23: At the upcoming June 2024 TC39 plenary meeting, we'll propose removing the following 6 functions for the seconds and microseconds units:

  • Temporal.Instant.fromEpochMicroseconds ( _epochMicroseconds_ )
  • Temporal.Instant.fromEpochSeconds ( _epochSeconds_ )
  • get Temporal.Instant.prototype.epochMicroseconds
  • get Temporal.Instant.prototype.epochSeconds
  • get Temporal.ZonedDateTime.prototype.epochMicroseconds
  • get Temporal.ZonedDateTime.prototype.epochSeconds

For decades, milliseconds has been (thanks to Date) the primary time unit in ECMAScript, and nanoseconds is required because it's the full-resolution unit of Temporal. So we believe that highly discoverable and ergonomic access to those is important.

But we believe that the other two are less important, so can be omitted to save space. (From history: microseconds was only added to these APIs because it was weird to have 3 out of 4 units present, so if we remove seconds then removing microseconds too is a no-brainer.)

@ljharb
Copy link
Member

ljharb commented May 24, 2024

Are there ever any edge cases where ms * 1e3 won't be microseconds, or ms / 1e3 won't be seconds?

@justingrant
Copy link
Collaborator Author

Are there ever any edge cases where ms * 1e3 won't be microseconds

Well, in the current API epochMicroseconds is a bigint because precision, so there is possible precision loss in millseconds => microseconds using number multiplication. But that's what epochNanoseconds is for, because it's a bigint so no precision loss if you get microseconds via epochNanoseconds / 1000n.

ms / 1e3 won't be seconds

The user would need to Math.trunc the result to get the equivalent of the current epochSeconds getter, but otherwise no I don't think there can be a difference. (I'd look to someone more experienced than me like @gibson042 to verify this.)

@ljharb
Copy link
Member

ljharb commented May 24, 2024

Replace 1e3 with 1000n where appropriate :-)

As long as this is the case, it's unfortunate to make users learn more unit math, but it seems "fine".

@anba
Copy link
Contributor

anba commented May 24, 2024

The current seconds, milliseconds, and microseconds getters perform floor'ed division of the nanoseconds value. If you rely on this exact semantics and some of your inputs can have negative nanoseconds, a bit more math is needed for microseconds.

IOW current seconds getters can be replaced with just Math.floor(milliseconds / 1000). Current microseconds getters can be replaced with nanoseconds / 1000n + BigInt(Math.floor(Number(nanoseconds % 1000n) / 1000)) (or similar expressions like nanoseconds / 1000n + BigInt(((nanoseconds % 1000n) < 0) * -1) or nanoseconds / 1000n + ((nanoseconds % 1000n) < 0n ? -1n : 0n), etc.)

Edited: Fixed typo in seconds example spotted by @ljharb

@ljharb
Copy link
Member

ljharb commented May 24, 2024

These replacement examples seem like pretty obviously massive ergonomic losses, but at least everyone can duplicate the same SO snippet or LLM output, so that instead of paying in engine size, we pay in "tons of websites' JS" size.

@justingrant
Copy link
Collaborator Author

Whoops, I forgot that we changed rounding for epoch* getters to be towards the beginning of time, not towards 1970-01-01.

we pay in "tons of websites' JS" size.

Fortunately, microseconds turns out to be rarely-used unit in real-world ECMAScript programs, relative to milliseconds and seconds. Nanoseconds is probably used least of all, but with Temporal's smallest unit being nanoseconds, it's sure to get more use. That leaves microseconds. Originally we weren't even going to add microseconds-related methods on Instant and ZDT because we didn't think they'd get enough use. But we decided to go for consistency and add it.

But in a world of size constraints, cutting microseconds seems like a pretty obvious choice.

All this is to say that IMO it's OK if microseconds requires some weird math. And if the world changes and suddenly everyone loves microseconds, then it'd be easy to add these methods back in a later proposal.

TL;DR - microseconds being difficult seems OK, IMO.

@ptomato
Copy link
Collaborator

ptomato commented May 27, 2024

There is an implementation of this, including documentation of the workarounds, here.

@ptomato ptomato self-assigned this Jun 13, 2024
ptomato added a commit that referenced this issue Jun 13, 2024
Closes: #2849

Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
ptomato added a commit that referenced this issue Jun 13, 2024
Closes: #2849

Co-Authored-By: Richard Gibson <richard.gibson@gmail.com>
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

5 participants