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

Own Properties as data #918

Closed
sffc opened this issue Sep 17, 2020 · 8 comments
Closed

Own Properties as data #918

sffc opened this issue Sep 17, 2020 · 8 comments
Labels
calendar Part of the effort for Temporal Calendar API spec-text Specification text involved

Comments

@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

If we decide to adopt own enumerable properties per #917, I wanted to offer another possibility. Should the own properties for calendar fields be actual data fields? This would require calendars to pre-compute the lesser used fields like daysInMonth, but maybe that's a small price to pay for the potentially significant reduced complexity. These methods would no longer have to proxy into the calendar all the time.

@ljharb
Copy link
Member

ljharb commented Sep 18, 2020

Own enumerable getters would be supported by spread and also lazily computed, so I don't think they have to be own data properties.

@sffc
Copy link
Collaborator Author

sffc commented Sep 18, 2020

Own enumerable getters would be supported by spread and also lazily computed, so I don't think they have to be own data properties.

They don't have to be pre-computed, but I was suggesting that if they were, they would simplify the calendar interface a bit.

@ljharb
Copy link
Member

ljharb commented Sep 18, 2020

In what way? Consumers of the interface shouldn't have any thoughts about whether it's an accessor or a data property, as long as it's enumerable.

@sffc
Copy link
Collaborator Author

sffc commented Sep 18, 2020

When the properties are getters, they proxy through the calendar object. This introduces quite a bit of complexity. If someone calls .year, they are probably going to call .month next; the calendar object may want to cache the computation results, which requires the use of a WeakMap keyed on the Temporal object. It works, but it seems like a complicated solution to a simple problem.

@ljharb
Copy link
Member

ljharb commented Sep 18, 2020

If the calendar object computing things is expensive, i'd assume it has its own cache; presumably something that isn't in the spec but every implementation effectively does.

class Calendar {
  #fieldData;
  get field() {
    this.#fieldData ??= computeExpensiveThing();
    return this.#fieldData;
  }
}

doesn't seem particularly complicated.

@sffc
Copy link
Collaborator Author

sffc commented Sep 18, 2020

The calendar object may be shared by many Temporal objects, so it can't simply store the data internally like that. It can do, for example,

class MyCalendar {
  #fieldCache;
  constructor() {
    this.#fieldCache = new WeakMap();
  }
  year(date) {
    if (!this.#fieldCache.has(date)) {
      this.#fieldCache.set(date, this.computeCalendarFields(date));
    }
    return this.#fieldCache.get(date).year;
  }
  // ...
}

Meanwhile, the Temporal.Date looks like this:

class Temporal.Date {
  get year() {
    return this.calendar.year(this);
  }
  // ...
}

That works. But, it seems more complicated than

class MyCalendar {
  dateFromFields(fields, constructor) {
    const { isoYear, isoMonth, isoDay } = this.computeISOFields(fields);
    const date = new constructor(isoYear, isoMonth, isoDay, this);
    fields = this.computeCalendarFields(date);  // normalize the fields
    date.year = fields.year;
    date.month = fields.month;
    date.day = fields.day;
    result.daysInMonth = fields.daysInMonth;
    // ...
    return date;
  }
  // ...
}

@ljharb
Copy link
Member

ljharb commented Sep 18, 2020

In practice tho, none of the caching mechanisms would need to be in the spec, and implementations would only add them when needed.

@ptomato ptomato added the spec-text Specification text involved label Sep 18, 2020
@ptomato
Copy link
Collaborator

ptomato commented Oct 13, 2020

Closing as per #917 (comment)

@ptomato ptomato closed this as completed Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
calendar Part of the effort for Temporal Calendar API spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

3 participants