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

Make fields enumerable own (data?) properties on instances #917

Closed
gibson042 opened this issue Sep 17, 2020 · 11 comments
Closed

Make fields enumerable own (data?) properties on instances #917

gibson042 opened this issue Sep 17, 2020 · 11 comments
Labels
behavior Relating to behavior defined in the proposal ergonomics feedback needs plenary input Needs to be presented to the committee and feedback incorporated

Comments

@gibson042
Copy link
Collaborator

gibson042 commented Sep 17, 2020

This is a rehash of #403 in response to feedback about Temporal being overly verbose. It seems like supporting Temporal.DateTime.from({...date, ...time}) rather than requiring Temporal.DateTime.from({...date.getFields(), ...time.getFields()}) (cf. #720) would be an easy win, and would have the additional benefit of making Temporal instances behave more like Record values. Such properties, whether implemented as accessor or data properties, would reflect the immutability of Temporal instances by theirselves being immutable.

Further, if this is rejected then the rationale should be well-documented (unlike #403, which claimed a "consensus" that was not adequately captured in meeting notes).

@gibson042 gibson042 added behavior Relating to behavior defined in the proposal ergonomics feedback labels Sep 17, 2020
@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

Feedback from April 1 TC39 (https://github.com/tc39/notes/blob/master/meetings/2020-03/april-1.md#temporal-update):

MM: For properties that are per-instance data I think that optimizing for object spread and rest is a nice thing to do. For anything method-like, I would not do that.

JHD: I have a queue item about this. Optimizing for rest spread is not what most instances in the language do. Array.length is an own per-instance data value, it’s not enumerable. Like Map and Set size are accessors, but conceptually, that is part of an instance. An error message, a name. There are a bunch of instances where that is not the case. We just don’t have own enumerable data properties. The convention is that those are things that users make, not the language.

@gibson042
Copy link
Collaborator Author

gibson042 commented Sep 17, 2020

Existing examples of own properties:

  • Object.hasOwnProperty.call(new Error(""), "message")
  • Object.hasOwnProperty.call(new Function(), "name")
  • Object.hasOwnProperty.call(new Function(), "prototype")
  • Object.hasOwnProperty.call(new RegExp(), "lastIndex")
  • Object.hasOwnProperty.call(new String(), "length")
  • Object.hasOwnProperty.call(Object.getOwnPropertyDescriptor({a:0}, "a"), "enumerable")
  • (function(){ return Object.hasOwnProperty.call(arguments, "length") })()
  • (strings => Object.hasOwnProperty.call(strings, "raw"))``

@ljharb
Copy link
Member

ljharb commented Sep 17, 2020

It's notable that the vast majority of those are legacy.

I'm totally on board with being able to ergonomically make a DateTime from date and time - i made https://npmjs.com/daytime long ago for this purpose :-) - but I don't think object spread is the right mechanism for it.

@gibson042
Copy link
Collaborator Author

gibson042 commented Sep 17, 2020

OK, more recent examples:

  • import("data:text/javascript,export default Object.hasOwnProperty.call(import.meta, 'url')")
  • (async () => Object.hasOwnProperty.call((await Promise.allSettled([0]))[0], "status"))()
  • Object.hasOwnProperty.call(Proxy.revocable({}, {}), "revoke")
  • Object.hasOwnProperty.call(/(?<a>.*)/.exec(""), "groups")
  • Object.hasOwnProperty.call([][Symbol.iterator]().next(), "done")
  • Object.hasOwnProperty.call((new Intl.RelativeTimeFormat()).resolvedOptions(), "numeric")
  • Object.hasOwnProperty.call((new Intl.RelativeTimeFormat()).formatToParts(-1, "day")[0], "type")

There is significant construction of values that are similar to property bags throughout ECMAScript, and Temporal already makes heavy use of that pattern for input (e.g., Temporal.Time.from({ hour: 10, minute: 23 })). It should be valid for output as well, primarily to decrease verbosity but also for symmetry.

Are you opposed to this change, and if so can you explain why?

@ljharb
Copy link
Member

ljharb commented Sep 17, 2020

I'm opposed to adding own properties for all the fields, because the prevailing pattern ES6+ has pushed hard to use is to prefer slot-revealing prototype accessor properties (which wouldn't show up in spread). I am opposed to making them all own properties solely for use in object spread, because I don't think the inconsistency is worth the ergonomics, and because I'm not convinced there's no other alternative.

For example, DateTime.from(date, time) could pivot on whether the first argument is a Date or Time, or a plain object - or DateTime.from({ date, time }) could work when their values are a Date and Time respectively - etc.

@sffc
Copy link
Collaborator

sffc commented Sep 18, 2020

Making the properties be own enumerable properties would make certain calendar-related problems substantially less complex. See #901 and #918.

@ptomato ptomato added the needs plenary input Needs to be presented to the committee and feedback incorporated label Sep 18, 2020
@gilmoreorless
Copy link
Contributor

FWIW, while experimenting with Temporal-based forks of some D3 modules, I hit a testing problem due to the lack of own properties.

D3 uses tape for tests, which in turn uses deep-equal for object equality checking (thanks @ljharb for maintaining it). I had some tests that were false positives, which turned out to be caused by deep equality checks. For the Temporal objects, deep-equal ended up defaulting to the checking method of "do all the own properties on these objects match?" With each Temporal object having no own properties, the objects were considered equal. An example can be found here: https://runkit.com/gilmoreorless/5f65e85777089d001ade948b

To fix my experiment, I just did a hacky patch of deep-equal and moved on. Obviously the most correct test would be checking that both objects have the same Temporal class, then using the .equals() method. But that will also require an update to every library that does deep equality checking. In the meantime, having the key fields be exposed as own properties would allow those libraries to work with Temporal as-is.

Personally, I'm not specifically for or against exposing own properties, just giving a data point. But I was certainly initially surprised when I found the source of the bug.

@ljharb
Copy link
Member

ljharb commented Sep 19, 2020

@gilmoreorless for deep-equal, since Temporal objects are builtins, it would brand-check them (like it does for Dates, RegExps, etc) and check their internal slots - so a lack of own properties wouldn't be an issue there. You're correct that in the meantime, own properties would "just work" for these libraries.

@gibson042 gibson042 changed the title Make fields enumerable own data properties on instances Make fields enumerable own (data?) properties on instances Sep 23, 2020
@ptomato
Copy link
Collaborator

ptomato commented Oct 13, 2020

Summary of discussion in Oct. 9 meeting: Both prototype properties and own properties have advantages and disadvantages, so either way the solution is a tradeoff. Since prototype properties are the norm, the burden of convincing is higher if we were to propose a solution involving own properties for Temporal. None of the current champions feel strongly enough about this issue to spend the required effort advocating for own properties, so we are closing this issue for lack of a champion. We would reopen it if a champion presented themselves, given that there was not universal opposition in the September TC39 plenary.

@ptomato ptomato closed this as completed Oct 13, 2020
@ljharb
Copy link
Member

ljharb commented Oct 13, 2020

@ptomato was there a decision on how to ergonomically support creating a DateTime from a Date and a Time?

@ptomato
Copy link
Collaborator

ptomato commented Oct 13, 2020

I believe that's addressed by #889.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
behavior Relating to behavior defined in the proposal ergonomics feedback needs plenary input Needs to be presented to the committee and feedback incorporated
Projects
None yet
Development

No branches or pull requests

5 participants