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

Should Absolute getEpoch* methods be property getters? #750

Closed
justingrant opened this issue Jul 10, 2020 · 12 comments · Fixed by #1012
Closed

Should Absolute getEpoch* methods be property getters? #750

justingrant opened this issue Jul 10, 2020 · 12 comments · Fixed by #1012
Assignees
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved

Comments

@justingrant
Copy link
Collaborator

Should Absolute's getter methods getEpochNanoseconds(), getEpochMicroseconds(), getEpochMilliseconds(), and getEpochSeconds() be property getters (e.g. .epochNanoseconds) instead of methods?

These methods all return primitive number or bigint values so the issues with object identity (see #724) don't apply here.

I assume there is some historical reason why Absolute's getters are methods while other types (e.g. Date.prototype.year) use property getters. But I'm new so don't know the history.

These methods will likely be the most performance-sensitive code in all of Temporal (because it'll be used for timing tasks), so any change should keep perf implications in mind.

This topic was split out of #724.

@thojanssens
Copy link

thojanssens commented Jul 11, 2020

I'm trying to catch up with the issues about Absolute and Time zones; there is much to read.

We apparently asked Google C++ developers for suggestions; and they mentioned as first point, being happy to never have exposed the epoch for (what we call now) an Absolute: #139

Could you explain why we ignore that suggestion now?

Secondly, could someone refer me to the choice for going with an Absolute, rather than a zoned-datetime (that may also be set to UTC and consequently replace the Absolute)? cc @ptomato

@ptomato
Copy link
Collaborator

ptomato commented Jul 13, 2020

We apparently asked Google C++ developers for suggestions; and they mentioned as first point, being happy to never have exposed the epoch for (what we call now) an Absolute: #139

Could you explain why we ignore that suggestion now?

We don't ignore any suggestions, but neither do they always have to be adopted 😄

My opinion is that in our case the choice of epoch is anchored by the existing, already exposed, choice of epoch for legacy Date.

There are quite a lot of closed issues discussing about Instant vs. Absolute vs. ZonedDateTime vs. ZonedAbsolute, but maybe these are a good starting point:

@gibson042
Copy link
Collaborator

I prefer the use of explicit methods. To me, use of fields implies an amount of independence and composibility that does not exist in the case of scaling—e.g., a Temporal.Time instance is an enhanced bag of {hour, minute, second, millisecond, microsecond, nanosecond} (which are exposed accordingly), but that is not true of a Temporal.Absolute instance.

@justingrant
Copy link
Collaborator Author

Given the feedback about brevity being the top concern, this seems like an easy way to make common tasks briefer.

I agree with @gibson042 that methods are a little clearer to distinguish compared to data properties vs. convenience properties.

But I'm not sure this clarity benefit outweighs the brevity benefit.

Also we have many other convenience properties throughout Temporal. Like hoursInDay, daysInMonth, etc. DateTime has 8 convenience properties. LDT has those 8 plus a few more like hoursInDay, timeZoneOffsetString, etc. Why are the Absolute convenience properties different?

@ptomato
Copy link
Collaborator

ptomato commented Sep 22, 2020

I had unfortunately missed adding this one to the list of design decisions to complete. It's added now.

I agree with @gibson042.

@justingrant
Copy link
Collaborator Author

Could we discuss at the next Champions meeting? My thinking is that brevity is the top user concern and dealing with epochXXX data is one of the top use cases where those users have been complaining about brevity.

Also, still need to answer this question:

Also we have many other convenience properties throughout Temporal. Like hoursInDay, daysInMonth, etc. DateTime has 8 convenience properties. LDT has those 8 plus a few more like hoursInDay, timeZoneOffsetString, etc. Why are the Absolute convenience properties different?

@ptomato
Copy link
Collaborator

ptomato commented Sep 25, 2020

Why are the Absolute convenience properties different?

Admittedly this is more of a vague feeling than a reasoned objection, but to me they feel like methods because they return a value that isn't exact. (That excludes getEpochNanoseconds, which I guess feels like it ought to be a method if the other three are methods.)

@gibson042
Copy link
Collaborator

My guiding principles here are that only fields should be accessed as properties, and fields should be limited to minimal non-redundant data that fully characterizes an instance (e.g., Temporal.Date is {calendar, era?, year, month, day} and Temporal.Time is {hour, minute, second, millisecond, microsecond, nanosecond}).

@justingrant
Copy link
Collaborator Author

justingrant commented Sep 26, 2020

There's tension between matching fields & properties and the consistent feedback we've gotten that the current API is too verbose. Furthermore, that verbosity feedback seems concentrated on msecs-since-epoch/legacy-Date-interop use cases which are the exact subject of this issue.

Given that we're using convenience properties exhaustively elsewhere in Temporal, and given the strong feedback we've gotten both for brevty in general and spefically for brevity of epoch-milliseconds, IMHO it seems reasonable to use properties in this case too.

My guiding principles here are that only fields should be accessed as properties

I like @gibson042's idea to have a specific set of guidelines or principles that determine whether something should be a property or method. I'd recommend slightly more flexible principles, though:

  • return the same (===) value when called again later
  • be pure (no side effects)
  • accept no parameters (obviously!)
  • be inexpensive to calculate
  • derived from internal fields/slots only (no external calls)

@ljharb
Copy link
Member

ljharb commented Sep 26, 2020

I like those criteria! but I don't think "inexpensive to calculate" has to matter, as long as it's memoized once computed.

@justingrant
Copy link
Collaborator Author

Meeting 2020-10-09: We'll make these methods into property getters.

@ptomato
Copy link
Collaborator

ptomato commented Oct 9, 2020

(Rationale being, that according to the above criteria there is no reason to be inconsistent with read-only properties like date.daysInMonth other than a feeling or preference that some of us have.)

@ptomato ptomato added documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved labels Oct 9, 2020
@ptomato ptomato self-assigned this Oct 17, 2020
ptomato added a commit that referenced this issue Oct 17, 2020
ptomato added a commit that referenced this issue Oct 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Additions to documentation non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! spec-text Specification text involved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants