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 parameterless conversion methods that return objects (e.g. DateTime.prototype.toDate) be property getters instead? #724

Closed
justingrant opened this issue Jul 2, 2020 · 13 comments

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jul 2, 2020

Temporal has many "downscaling" conversion methods that remove data so accept no properties, e.g.

Should these be property getters (.date, .time, .yearMonth, .monthDay) instead? Some reasons we might want to do this:

  • Less typing req'd for common use cases. A desire for brevity has come up multiple times in feedback from early adopters.
  • Clearly distinguishes conversion methods that need more data (e.g. Time.prototype.toDateTime) from conversion methods that don't need any more data (e.g. DateTime.prototype.date ).
  • Aligns read and write syntax if Easier way to combine multiple Temporal objects in one from or with call ? #720 is adopted (this issue is essentially the inverse of Easier way to combine multiple Temporal objects in one from or with call ? #720, although each could be done independently)
  • Matches naming of now methods. If you learn how to use now, you can apply that knowledge to conversions.
  • For LocalDateTime, provides more consistency between access to .calendar, .absolute, and .timeZone properties (which will be property getters) with access to .date, .time, .yearMonth, .monthDay which are currently methods.
  • For Date, DateTime, YearMonth, and MonthYear, provides more consistency between access to .calendar with access to .date, .time, .yearMonth, .monthDay.

Note: #750 discusses whether Absolute getter methods like getEpochNanoseconds (which return primitives, not objects) should be changed to properties like epochNanoseconds. This topic was originally part of this issue but I moved it over to #750 so that we could focus this issue on the challenges of object-valued property getters.

@ljharb
Copy link
Member

ljharb commented Jul 3, 2020

For any of the ones that produce a new object every time, absolutely not - that's just massively confusing.

Are there any that take no arguments and return the same === value every time?

@justingrant
Copy link
Collaborator Author

Are there any that take no arguments and return the same === value every time?

Yep. The getEpochXxx() methods on Absolute (getEpochNanoseconds, getEpochMicroseconds, getEpochMilliseconds, getEpochSeconds) all return primitive bigint or number values. Is there a reason why these were left as methods while calculated values in other types (e.g. DateTime.prototype.daysInMonth) are property getters?

For any of the ones that produce a new object every time, absolutely not - that's just massively confusing.

If reference equality is the main concern, could object-returning properties be backed by slots that are lazily initialized on first call of the getter? Date would get a yearMonth and monthDay slot, DateTime would get date and time slots, and LocalDateTime (if adopted) would get a dateTime slot.

@ljharb
Copy link
Member

ljharb commented Jul 4, 2020

That approach is one that members of the committee explicitly rejected for AggregateError's errors property - iow, when the value is an object, it needs to either be returned from a prototype method, or installed as an own data property.

the getEpochX methods could conceivably be epochX getters; why would that be an improvement?

@justingrant
Copy link
Collaborator Author

the getEpochX methods could conceivably be epochX getters; why would that be an improvement?

Less typing. I'm particularly thinking of legacy Date and how it's annoying to repeatedly type .getYear() instead of simpler/shorter .year. Is there some countervailing reason that getEpochX should be methods instead of get accessors?

when the value is an object, it needs to either be returned from a prototype method, or installed as an own data property.

I read tc39/proposal-promise-any#38 but I'm not sure I fully understand the scope of the issue. What's a good summary of the problem that's being avoided by not having shared object state? Would it be OK for a getter to return a frozen instance of a built-in type like Temporal.YearMonth? Or is that a problem too, e.g. because its calendar property could be a custom userland object?

@ljharb
Copy link
Member

ljharb commented Jul 5, 2020

Code is read much more often than it's written, and avoiding the () adds unnecessary confusion.

Yes, the only safe way to return an object from a prototype getter would be if it's deeply frozen - ie, if there's no way to attach information to it that someone else calling the same getter can receive.

@justingrant
Copy link
Collaborator Author

justingrant commented Jul 6, 2020

avoiding the () adds unnecessary confusion.

Is this an argument against property getter functions in general? What are cases where getters are OK? I admit that I don't see the confusion-- if property values are deterministic based on immutable internal slots and have no side effects, then I'm not sure why a shorter-parentheses-less syntax is less clear. But maybe I'm missing something important about why?

Also, there are many property getters in Temporal, including both calculated props like DateTime.prototype.dayOfWeek and non-calculated getters like DateTime.prototype.year. These 4 methods on Absolute are AFAIK the only primitives in Temporal that are not exposed via property getters. I'm not sure why these methods are different from the others.

Yes, the only safe way to return an object from a prototype getter would be if it's deeply frozen - ie, if there's no way to attach information to it that someone else calling the same getter can receive.

@ptomato - How is this issue handled with the calendar property on Date, DateTime, etc. ?

@ptomato
Copy link
Collaborator

ptomato commented Jul 6, 2020

How is this issue handled with the calendar property on Date, DateTime, etc. ?

It's not; the issue was raised in the June TC39 meeting but the person who raised it wasn't personally there, and I don't remember there being a consensus (yet) that it is a constraint that the JS language must adhere to everywhere. I'm attending a call about it soon, in order to learn more.

@justingrant
Copy link
Collaborator Author

I'm attending a call about it soon, in order to learn more.

@ptomato - could you leave a note here when you have more info about this and/or discuss in our weekly meetings? #742 (and perhaps other issues too) are likely blocked until the calendar question is resolved.

@ptomato
Copy link
Collaborator

ptomato commented Jul 8, 2020

Sure. The call is tentatively scheduled for next week: https://github.com/tc39/incubator-agendas/blob/master/2020/07-14.md

@justingrant
Copy link
Collaborator Author

I just opened #750 to track the question of the getters in Absolute, which don't return objects so the object identity, SES, and other concerns above don't apply. Let's focus this issue on object properties only. I edited the original post accordingly.

@justingrant justingrant changed the title Should parameterless toFoo & getFoo methods be property getters instead? Should parameterless toFoo & getFoo object-returning conversion methods be property getters instead? Jul 10, 2020
@justingrant justingrant changed the title Should parameterless toFoo & getFoo object-returning conversion methods be property getters instead? Should parameterless conversion methods that return objects (e.g. DateTime.prototype.toDate) be property getters instead? Jul 10, 2020
@ptomato
Copy link
Collaborator

ptomato commented Aug 3, 2020

FWIW, my feeling is that the type conversion methods should be methods, not property getters. More out of a conceptual understanding of what those methods do, rather than technical reasons.

@ryzokuken
Copy link
Member

Agreed with @ptomato and @ljharb here that converting these methods into getters would likely do more harm than good, but I'd be fine with converting the epoch methods.

@justingrant
Copy link
Collaborator Author

justingrant commented Aug 13, 2020

We've decided (in 2020-08-13 meeting) to leave the status quo in place, with the following heuristic:

  • Values that are immutable attributes of the object itself should be property getters, whether they're scalars (e.g. .year) or objects (e.g. .calendar, .timeZone).
  • Values that are conversions into another Temporal type should be toXXX() methods, not property getters.

Closing as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants