tc39 / proposal-temporal

Provides standard objects and functions for working with dates and times.
https://tc39.es/proposal-temporal/docs/
Other
3.29k stars 149 forks source link

Most of methods of Temporal.PlainDate requires a fresh new Calendar instance for each Temporal.PlainDate #1808

Closed Constellation closed 1 year ago

Constellation commented 3 years ago

The current spec creates a fresh instance for each Temporal.PlainDate. Since most of fields of Temporal.PlainDate touches calendars, Temporal.PlainDate almost always requires two objects per instance.

This fresh object is meaningful only when the user adds adhoc method to these created calendar instances after creating Temporal.PlainDate. But the use of custom calendar can be cleanly achieved by passing an optional calendar parameter to the Temporal.PlainDate constructor.

So, how about removing necessity of creating a calendar object for known calendars for performance and memory saving?

justingrant commented 1 year ago

I think the first one should use this.#calendarObject.id instead of this.#calendarObject.toString().

This was intentional because it aligns with the current spec (and docs and polyfill) where the id getter is optional to override when creating a custom calendar or time zone, while overriding toString is required. Are you suggesting that we should change this behavior? If so, then we'd need to ask @ptomato why the current behavior makes toString required but id optional to override.

Here's info from the docs:

The class must override toString() to return its own identifier

The polyfill aligns with the docs:

  get id() {
    if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
    return ES.ToString(this);
  }

As does the spec.

12.5.3 get Temporal.Calendar.prototype.id Temporal.Calendar.prototype.id is an accessor property whose set accessor function is undefined. Its get accessor function performs the following steps:

  1. Let calendar be the this value.
  2. Perform ? RequireInternalSlot(calendar, [[InitializedTemporalCalendar]]).
  3. Return ? ToString(calendar).
ptomato commented 1 year ago

This was intentional because it aligns with the current spec (and docs and polyfill) where the id getter is optional to override when creating a custom calendar or time zone, while overriding toString is required. Are you suggesting that we should change this behavior? If so, then we'd need to ask @<!---->ptomato why the current behavior makes toString required but id optional to override.

Sorry, I missed this question originally. It was because we had to pick one to use consistently in observable calls within the Temporal spec, and toString seemed the more fundamental of the two.

justingrant commented 1 year ago

I've tried to capture the consensus from today's champions' meeting. Please correct me if I got anything wrong.

1. Internal slots for [[Calendar]] and [[TimeZone]] will either store a string ID (of a built-in calendar or time zone, respectively) or an object.

2. Temporal class constructors' timeZone and calendar parameters will continue to accept either a string or an object, like they do today.

3. The object returned from getISOFields prototype methods will continue to have calendar and timeZone properties that return the contents of the [[Calendar]] and [[TimeZone]] internal slots, regardless of whether they are strings or objects. Because this is an advanced method primarily intended for custom calendar authors, we don't need to optimize its ergonomics, nor do we need to worry about calendar authors forgetting that custom calendars exist!

4. Change the current behavior where the built-in id property getter is implemented by calling toString. Instead:

5. Replace .calendar and .timeZone prototype properties with string-typed calendarId and timeZoneId prototype properties and object-returning getCalendar() and getTimeZone() prototype methods.

6. Temporal.Now.timeZone() will be renamed to Temporal.Now.timeZoneId() and will return a string.

Pseudocode:

get calendarId(): string {
  if (typeof this.#calendarSlot === 'string') return this.#calendarSlot;
  const { id } = this.#calendarSlot;
  if (typeof id !== 'string') throw new TypeError (`Invalid type ${typeof id} of calendar ID`);
  return id;
}
get timeZoneId(): string {
  if (typeof this.#timeZoneSlot === 'string') return this.#timeZoneSlot;
  const { id } = this.#timeZoneSlot;
  if (typeof id !== 'string') throw new TypeError (`Invalid type ${typeof id} of time zone ID`);
  return id;
}
getCalendar(): Temporal.CalendarProtocol {
  return typeof this.#calendarSlot === 'string' ? Temporal.Calendar.from(this.#calendarSlot) : this.#calendarSlot; 
}
getTimeZone(): Temporal.TimeZoneProtocol {
  return typeof this.#timeZoneSlot === 'string' ? Temporal.TimeZone.from(this.#timeZoneSlot) : this.#timeZoneSlot;
}

Like any solution to this underlying issue, this will be a breaking change that would need to be communicated clearly to existing polyfill users.

Should we make the next few versions of the polyfills throw (with a helpful error message about migration path) in response to calls to the calendar and timeZone getters, so that we can help users migrate to the new names?

pipobscure commented 1 year ago
  • 2.3 QUESTION: if the caller passes in a Calendar or TimeZone object that is a built-in instance, should the constructor convert it into a string to get fast-path behavior? Or is "is a built-in instance" something that's not possible to detect, so this is a moot point?

A built-in instance could be modified/ patched after having been created to no longer act entirely the same way. Therefore such an object is no longer identical to passing in a string identifying a built in. So there should not be any attempt to detect. Casting from calendarId to calendarObject is a one way process.

pipobscure commented 1 year ago

Is there an actual use-case for the getCalendar() method? You could instead just use Temporal.Calendar.from(dateObject) with the same effect. If it’s a built in calendar then a new instance is created and if it’s a custom calendar then it’s just returned.

The reason I ask is because we’re already exposing too little info on the object itself. like the only way to know if it’s a built in or custom calendar is to use getISOFields. So if we already don’t expose everything on the object, is there a clear benefit of exposing the object in 2 ways.

I’m just putting that out there if it wasn’t part of the discussion (sorry I had to miss the meeting). If it was considered, then I’m ok with the consensus. If it wasn’t, then I’d propose to just drop the getXXX methods as superfluous.

littledan commented 1 year ago

Thanks @justingrant , I'm happy with the recorded conclusion. For the record, I'd be fine either way with the question in https://github.com/tc39/proposal-temporal/issues/1808#issuecomment-1310867994 (either including or excluding these methods).

sffc commented 1 year ago

Thanks for the writeup, @justingrant!

On the question of whether to include or not include getCalendar(): TimeZone has various useful methods intended to be called directly on the time zone, so it makes sense to include getTimeZone() on that basis. Omitting getCalendar() may reduce consistency. That's not a super strong argument.

On the question of fast-path for objects: I fear that people may write code like Temporal.Now.plainDate(something.getCalendar()), so if it's easy for us to detect built-in calendar objects, we may want to do that. Otherwise, I guess people should instead write Temporal.Now.plainDate(something.toISOFields().calendar) ?

gibson042 commented 1 year ago

5. Replace .calendar and .timeZone prototype properties with string-typed calendarId and timeZoneId prototype properties and object-returning getCalendar() and getTimeZone() prototype methods.

  • 5.1 The xxxId properties will return the slot value if the slot contains a string. Otherwise it will return the value of the id property if the slot contains an object.
  • 5.2 The getXxx methods will return a new Temporal.TimeZone or Temporal.Calendar object if the slot is a string. Otherwise they will return the object stored in the slot.

Pseudocode:

get calendarId(): string {
  return typeof this.#calendarSlot === 'string' ? this.#calendarSlot : this.#calendarSlot.id; 
}
get timeZoneId(): string {
  return typeof this.#timeZoneSlot === 'string' ? this.#timeZoneSlot : this.#timeZoneSlot.id; 
}
…

Will the calendarId and timeZoneId getters perform a lookup every time, or will they instead return data populated at initialization? I prefer the latter, because otherwise their value can change, which seems inappropriate for objects that generally strive for immutability. IOW:

 get calendarId(): string {
-  return typeof this.#calendarSlot === 'string' ? this.#calendarSlot : this.#calendarSlot.id; 
+  return typeof this.#calendarSlot === 'string' ? this.#calendarSlot : this.#calendarIdSlot; 
 }
 get timeZoneId(): string {
-  return typeof this.#timeZoneSlot === 'string' ? this.#timeZoneSlot : this.#timeZoneSlot.id; 
+  return typeof this.#timeZoneSlot === 'string' ? this.#timeZoneSlot : this.#timeZoneIdSlot; 
 }

Also, what should happen if the id of a purported calendar or time zone object is not a string? I would recommend throwing a TypeError.

pipobscure commented 1 year ago

I think the latter is very inconsistent, because it implies there is such a thing as a calendarId that is it’s own thing rather than something a calendar has. I’d be Ok with the second variant only if id is specified as {value:”calid”,enumerable:true,writable:false,configurable:false} ie. a property defined as a value and not a getter that is neither writable nor configurable, such that he lack of lookup is only observable via proxy. Otherwise I think we’re better of with a lookup.

pipobscure commented 1 year ago

The question of what happens when id is not a string makes me think we might have made a mistake in not using cal.toString() instead; but yes, in this case we should throw rather than coerce, because it’s a required part of what being a calendar means.

gibson042 commented 1 year ago

Otherwise I think we’re better of with a lookup.

Even though that means that void plainDate.calendarId can throw an exception or (d => d.calendarId === d.calendarId)(plainDate.withCalendar(cal)) can be false? To me, those seem much worse than some people mistakenly inferring that "calendarId" is a type in itself.

justingrant commented 1 year ago

in this case we should throw rather than coerce, because it’s a required part of what being a calendar means.

Good suggestion! I edited the proposal above accordingly:

  • 5.1.1 If the id is not a string, throw a TypeError instead of returning it.

. . .

get calendarId(): string {
  if (typeof this.#calendarSlot === 'string') return this.#calendarSlot;
  const { id } = this.#calendarSlot;
  if (typeof id !== 'string') throw new TypeError (`Invalid type ${typeof id} of calendar ID`);
  return id;
}
get timeZoneId(): string {
  if (typeof this.#timeZoneSlot === 'string') return this.#timeZoneSlot;
  const { id } = this.#timeZoneSlot;
  if (typeof id !== 'string') throw new TypeError (`Invalid type ${typeof id} of time zone ID`);
  return id;
}

On the question of fast-path for objects: I fear that people may write code like Temporal.Now.plainDate(something.getCalendar()), so if it's easy for us to detect built-in calendar objects, we may want to do that. Otherwise, I guess people should instead write Temporal.Now.plainDate(something.toISOFields().calendar) ?

Won't built-in calendar objects work exactly the same as using strings? My understanding was that the original proposal (and this latest one) here is an optimization, not a behavior change. The perf gains may matter ecosystem-wide, especially for code that doesn't use calendars/timezones intensively, but for any particular app I find it hard to imagine that using getCalendar() vs. using a string will generate significant perf differences.

That said, if the user already has a Temporal object and wants to re-use its calendar, then they can just use the Temporal object as-is. There's no need to extract the calendar. This would be both more ergonomic and better perf, so seems like we can just document this clearly and let perf-savvy users discover it.

something = Temporal.Now.zonedDateTime('chinese');
Temporal.Now.plainDate(something)

the only way to know if it’s a built in or custom calendar is to use getISOFields

AFAIK there's no (observable) way to know if a time zone or calendar is a built-in, either with the current API or with any proposed changes to the current API. Like in Shane's example above, the user can pass an object that is a built-in calendar or time zone instance. So the getISOFields trick would only work if we could differentiate "built-in calendar/TZ object" from "custom calendar/TZ object". Which per discussion above it sounds like we don't want to (or can't?) do.

If all we want to do is to detect whether an object calendar/TZ or a string ID was used to initialize the Temporal class instance, then getISOFields would work. As would this:

function hasObjectCalendar(temporalObject) {
  return temporalObject.getCalendar() === temporalObject.getCalendar();
}
pipobscure commented 1 year ago

AFAIK there's no (observable) way to know if a time zone or calendar is a built-in, either with the current API or with any proposed changes to the current API. Like in Shane's example above, the user can pass an object that is a built-in calendar or time zone instance. So the getISOFields trick would only work if we could differentiate "built-in calendar/TZ object" from "custom calendar/TZ object". Which per discussion above it sounds like we don't want to (or can't?) do.

Sorry for the misunderstanding. A calendar object created via Temporal.Calendar.from(‘gregory’) is a custom calendar that derives from the built-in, because I can modify its behaviour after creation. So when I say builtin calendar I mean something that doesn’t use the object, but rather the builtin primitive Functions.

The difference is that I f I specify the calendar as a string, then all calendar actions are actually done via the internal ICU functions; if on the other hand I first create a calendar object via Calendar.from then when it is used the methods on that object are called. I could for example override a method on that object to return a different result and if I do so then all things using that object will operate differently.

gibson042 commented 1 year ago

AFAIK there's no (observable) way to know if a time zone or calendar is a built-in, either with the current API or with any proposed changes to the current API.

There is, because calendar objects are returned as-is but calendar identifiers result in new object construction:

function usesBuiltinCalendar(obj) {
  return obj.getCalendar() !== obj.getCalendar();
}

(agreeing with @pipobscure that using an object calendar rather than a string is custom, even if the particular object was constructed by the implementation and has not [yet] been customized)

ptomato commented 1 year ago

This seems like a trap that I hadn't thought of before, unfortunately, and I agree with @pipobscure that it's worth considering to drop getCalendar.

If we don't special-case some sort of conversion path, Temporal.Now.plainDate(someDate.getCalendar()) or date.withCalendar(otherDate.getCalendar()) will produce objects that seem normal at first glance, but are unable to be optimized by the engine the way that using a built-in calendar ID would be. In other words, the object works fine but is invisibly shunted into a slow path.

(If you want to know the nitty-gritty — it's what @gibson042 said. Passing in an object calendar has to be treated as custom, even if that calendar object has not yet actively been customized. Because, you might later mutate the Temporal.Calendar prototype, or you could do something like this:

const calendar = someDate.getCalendar();
const date = Temporal.Now.plainDate(calendar);
calendar.month = () => 13;

...and that puts us right back in the difficult-to-optimize status quo.)

The right thing for the programmer to do here would be Temporal.Now.plainDate(someDate) or date.withCalendar(otherDate) so that no calendar object is created, but that's not obvious unless you dive deep into the implementation details of Temporal. The developer's thought process most likely goes "What do I need to put in this method? A calendar! Where do I get a calendar? From getCalendar!"

Another option might be to perform the fast-path anyway in the spec up-front, if we can. That might look something like, (in ToTemporalCalendar) "If calendar is an Object, and calendar has an [[InitializedTemporalCalendar]] internal slot, and OrdinaryGetPrototypeOf(calendar) is %Temporal.Calendar.prototype%, and calendar has no own properties, and %Temporal.Calendar.prototype% has not been mutated, return calendar.[[Identifier]]." (just a draft; the actual language would have to be more exact)

gibson042 commented 1 year ago

It might also be addressed if calendars are fully ingested up front, such that e.g. date.withCalendar(calendar) extracts the full collection of methods and is not affected by a later calendar.month = () => 13 override even though a subsequent date.withCalendar(calendar) would be. In that world, string calendars would be fast and object calendars that are built-in would only be slower by the effort spent for verification that they have not been modified. But the flip side is that identity comparison of calendar objects used by Temporal type instances would arguably become meaningless ("arguably" because that is already the case for impure calendar method implementations that are not deterministic with respect to input fields and options).

pipobscure commented 1 year ago

I have an instinctive dislike for that kind if special-casing. How about just removing getCalendar and leaving the calendarId property.

If there is no easy way to get to the calendar as such any use requires a bit of educating oneself and that should be enough. (or at least I think it’s where the right set of tradeoffs is)

justingrant commented 1 year ago

If we don't special-case some sort of conversion path, Temporal.Now.plainDate(someDate.getCalendar()) or date.withCalendar(otherDate.getCalendar()) will produce objects that seem normal at first glance, but are unable to be optimized by the engine the way that using a built-in calendar ID would be. In other words, the object works fine but is invisibly shunted into a slow path.

My understanding is that the most important case for optimization is also the most common case: when users are not doing anything with calendars nor timezones, but the implementation still needs to create a new calendar and/or timezone instance for every Temporal class instance.

If the "slow" path is limited to the obscure case where the caller calls the getXxx method and then re-uses that object, then I think that's OK, especially given that there's such an easy workaround that we can clearly document and that perf-savvy users can easily migrate to:

something = Temporal.Now.zonedDateTime('chinese');
Temporal.Now.plainDate(something)

Also, even in the worst case, I suspect it still won't be a huge perf impact in most JS apps which are generally limited by download speed, browser rendering, and waiting for back-end requests, not raw CPU or RAM while executing JS. I think this optimization matters for ecoysytem-wide perf, but for each individual app I find it hard to imagine that using an object vs. using a string will really make a massive app-wide difference except in unusual cases... and those unusual cases can be easily optimized in userland via the workaround above.

It'd also probably be possible for there to be a standard ESLint rule created that warns users about the potential perf gotchas of passing objects.

I don't have a strong feeling about removing getCalendar because Calendar objects are only needed in really obscure use cases. But TimeZone objects have several methods on them that are not obscure, so I'd be skeptical about removing the only easy-to-discover way to get to those methods.

ptomato commented 1 year ago

If the "slow" path is limited to the obscure case where the caller calls the getXxx method and then re-uses that object, then I think that's OK

I'm not sure I agree that that case is obscure or unusual; I think it's the most obvious way that programmers will reach for when they need to get the input for methods like withCalendar, withTimeZone, ZonedDateTime.from, etc., and they are not using a hard-coded string. It makes me uncomfortable that the obvious way is also the wrong way.

I don't have a strong feeling about removing getCalendar because Calendar objects are only needed in really obscure use cases. But TimeZone objects have several methods on them that are not obscure, so I'd be skeptical about removing the only easy-to-discover way to get to those methods.

I don't think this would be too bad ... if you want a TimeZone object from a string, you call TimeZone.from(string), so although I agree it'd be a bit surprising that we don't have a ZonedDateTime method to get the time zone, using TimeZone.from(zonedDateTime) for this case as well seems fairly self-explanatory and almost as obvious.

sffc commented 1 year ago

If there is no .getCalendar() function, people in this situation may reach for .calendarId, which is "even worse" since it starts throwing exceptions when you start with a custom calendar.

If people pass a .getCalendar() object around, they should get the correct behavior, yes? It's just a hair slower? That seems like a minor papercut that can be solved by refactoring a library's code base when necessary.

ptomato commented 1 year ago

If people pass a .getCalendar() object around, they should get the correct behavior, yes? It's just a hair slower? That seems like a minor papercut that can be solved by refactoring a library's code base when necessary.

I agreed with this at first, although I guess I put more value on not having an invisible performance cliff and that's why I prefer a different tradeoff. But it occurred to me later that they might not be equivalent in all cases, depending on the outcome of #2221. I assume for PlainTime we'd only want to allow the string "iso8601" as the calendar, and not an ISO calendar object. Other than #2221 I don't think there are currently cases where a Temporal object with an unmodified builtin calendar object in [[Calendar]] would behave differently from one with a string in [[Calendar]], but I'll keep an eye out for any more.

Anyway if I'm the only holdout who thinks we shouldn't have getCalendar() then I'm fine to drop the objection and move on with this.

pipobscure commented 1 year ago

I’m certainly of the opinion that we should not have getCalendar().

I also don’t accept the “user experience” argument; at least mot without actual data (ie more than just “my gut says”).

I also don’t think throwing is a bad thing. So I don’t know where exactly that leaves us.

(All that being said: this is not my hill to die on)

justingrant commented 1 year ago

invisible performance cliff

JS is filled with invisible perf cliffs that are even easier to stumble across than this one. Even basic things like how you do enumeration have perf gotchas that may not be obvious to novices but might matter in some cases.

For the perf case in this issue, I admit I'm not too concerned:

  1. it will only show up in the relatively rare case of extracting a time zone or calendar object and using it to create another Temporal object.
  2. The userland optimization is easy: use another Temporal object instead of a calendar or TZ object. Performing the optimization will be a one-line change in most cases with no effects elsewhere in the code.
  3. We can document the perf effect and the optimization, so the folks who care about this can discover it.
  4. AFAICT, perf-obsessive engineering teams can easily build a TS-aware ESLint rule to catch this case.

(All that being said: this is not my hill to die on)

I do have a fairly strong opinion that getTimeZone() is needed because TimeZone methods have a bunch of fairly-common use cases that can't be accomplished any other way, so having an ergonomic way to get a TimeZone object from a ZonedDateTime seems fairly important. I don't have a strong opinion about getCalendar().

depending on the outcome of https://github.com/tc39/proposal-temporal/issues/2221. I assume for PlainTime we'd only want to allow the string "iso8601" as the calendar, and not an ISO calendar object.

I think it'd be reasonable in this case to omit getCalendar() from PlainTime. This is a good reminder for me to finish the writeup I promised for #2221. I'll plan to do this after Dec 1 when my current job wraps up.

If there is no .getCalendar() function, people in this situation may reach for .calendarId, which is "even worse" since it starts throwing exceptions when you start with a custom calendar.

If people pass a .getCalendar() object around, they should get the correct behavior, yes? It's just a hair slower? That seems like a minor papercut that can be solved by refactoring a library's code base when necessary.

I think this interpretation is correct. A minor perf issue (that's easy to fix with a one-line change in a library) seems less bad than a functional breakage that will be harder to fix without breaking changes like changing types of library method params.

ptomato commented 1 year ago

In the Temporal meeting of 2022-12-01 we reached a few more conclusions:

There are still some open questions which we will continue discussing in following meetings.

ljharb commented 1 year ago

@ptomato oof, please "ID" instead of "Id"

justingrant commented 1 year ago

please "ID" instead of "Id"

A little more than 2 years ago, we renamed the name property of TimeZone to id (lower case), because we were concerned that using name might imply it's a localized name. id seemed closer to the spirit of what these properties represent, which is a machine-readable identifier.

Are you suggesting that we should rename them (big breaking change!) to ID or identifier or code or back to name or some other new name?

Or are you suggesting that we should keep the properties named id but methods should be xxxID?

ljharb commented 1 year ago

I'm suggesting that id or ID is appropriate, but never Id.

pipobscure commented 1 year ago

can I point to precedent in the webworld https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById for that camelCase version?

ljharb commented 1 year ago

There’s lots of precedent in many directions - there’s also the nightmare of XMLHttpRequest.

We shouldn’t hold ourselves to that low a bar. The only reason anyone chose “Id” is because of precedent in Java’s camelcasing algorithm - but code is for humans, and it’s not helpful to capitalize it that way.

anba commented 1 year ago

https://w3ctag.github.io/design-principles/#casing-rules seems to prefer "Id", too.

ljharb commented 1 year ago

I'm sure it does, but we needn't be constrained by W3C's casing rules.

pipobscure commented 1 year ago

Also ID (capitalised) is the acronym for Identity Document vs id (lowercase) is just the abbreviation for identity; enter camelCase and you have a capitalised I in calendarId

I’d just like to understand where that preference comes from, because my instinct, and all the evidence/precedence I’m seeing says that ID is the only version that’s wrong.

ljharb commented 1 year ago

I believe it's the abbreviation for "identifier", and colloquially I think of "ID" and "identifier" as the same thing.

"Id" also is a freudian concept paired with ego and superego, so I think anything's going to have conflicts somewhere :-)

sffc commented 1 year ago

getElementById is a very strong precedent IMO. Are there examples of Web Platform precedent in the other direction?

justingrant commented 1 year ago

Given that Id is individually called out in the W3C design principles spec, it seems like a strong precedent indeed, especially in absence of contrary guidance in 262.

image

I agree we don't have to be bound by W3C or any other standards body. But given that Web Platform and ECMAScript are very often used together, it seems like the right thing to do for the ecosystem to try to be consistent.

ljharb commented 1 year ago

This isn't "identity", though - this is "identifier".

justingrant commented 1 year ago

That subtle distinction will be lost on most web developers, for whom the high order bit is that the thing is composed of the letters "i" and "d". Beyond "identifier" vs "identity, a huge % of web developers don't even know that there's a distinction between Web Platform APIs and ECMAScript's built-in objects! To them, it's just "APIs built into my browser". To expect these developers to adopt different naming conventions depending on which built-in objects they're using seems like we'd be setting them up for predictable failure.

That said, this isn't a Temporal-only thing. Seems like it's a generalized question for the language: should ECMAScript's built-in objects' follow W3C's casing rules? Maybe it's worth making this a separate topic in the next plenary meeting, and then we can snap the proposed Temporal method & property name to whatever the committee decides for the language-wide rule? Then we can move forward with a PR for this issue, with the caveat that the names might change based on committee input on the wider question.

Would everyone be OK with this as a way to move fwd on this issue here?

littledan commented 1 year ago

When we discussed the question of whether to use camelCase or kebab-case for Intl string-valued options bag entries, we worried about compatibility and complexity, but everyone seemed fine with referencing that TAG-produced document. I think it'd be great if we could avoid jurisdictional fights and generally try to align with that sort of thing (especially since we haven't produced our own conventions document, and since that document is open to contributions). That said, the casing Id does feel weird to me too. I'm OK with either option, but let's try to resolve all bikeshedding here promptly (ideally outside of plenary, but OK to bring to plenary if we have to).

gibson042 commented 1 year ago

Possible precedents within TC39:

It looks like the only current use of "ID" is to preserve the case of letters in externally-defined Unicode property names "ID_Start" and "ID_Continue" despite dropping their internal separating underscores.

justingrant commented 1 year ago

Meeting 2023-01-05: We'll build a normative PR matching previous meeting discussions. AFAIK, this consensus is described here: https://github.com/tc39/proposal-temporal/issues/1808#issuecomment-1310747126. I just edited that comment to add the change from Temporal.Now.timeZone() to Temporal.Now.timeZoneId(), which I think was the only change from our previous discussions.

For naming, we're planning to stick with calendarId and timeZoneId because:

  1. There are no userland-facing precedents in 262 or 402 using *ID, *id, nor *Id. The only usage of those naming patterns are a handful of mentions that are internal to the specs and are not otherwise exposed to developers.
  2. There is very well-known userland-facing precedent in the wider web platform, e.g. document.getElementById
  3. There's an explicit design guideline from W3C to use *Id not *ID, and in absence of 262/402 precedent to the contrary, maintaining consistency across the wider web platform is important.
ljharb commented 1 year ago

That’s not a conclusion i can accept.

sffc commented 1 year ago

@ljharb Is the disagreement over *Id vs *ID your remaining holdout, or are there other issues in what @justingrant or @ptomato proposed?

ljharb commented 1 year ago

@sffc theres only two items in https://github.com/tc39/proposal-temporal/issues/1808#issuecomment-1334236302, and one of them is the naming, and the naming is the only thing i take issue with.

pipobscure commented 1 year ago

Ok, so let’s just compare and contrast the arguments so far:

Pro calendarId / timeZoneId:

  1. W3C normative document
  2. precedent (getElementById)
  3. missing precedent for all capital ID in userland 262
  4. capital letters in English generally abbreviate distinct words:
    • ID => Identity Document
    • Id => Identifier/Identity

Pro calendarID / timeZoneID

  1. It feels nicer
  2. ??? (please fill in more here that I might have missed so we end up with a comprehensive list)

I have a feeling we’ll need to have a more detailed discussion in plenary on this

ljharb commented 1 year ago

The snark notwithstanding, I don't have to convince you of my objection, it stands nonetheless.

pipobscure commented 1 year ago

No snark intended.

ljharb commented 1 year ago

To audit your first list, W3C recommendations aren't something we're bound to, and there's no precedent for Id in 262 at all, and "ID" is frequently used for "identifier", not just for "identity document" (i'd argue that in a programming context it's almost always "identifier").

You can invert the latter for the second list as well.

sffc commented 1 year ago

We could circumvent this discussion by choosing a different suffix, like:

pipobscure commented 1 year ago

We could circumvent this discussion by choosing a different suffix, like:

  • timeZoneCode
  • timeZoneIdentifier
  • timeZoneName (note: this is the name of an ECMA-402 setting that controls the time zone style)

I like it. Let’s go with timeZoneName and calendarName to keep things short (after all there is a reason Identifier is frequently abbreviated 😄 )

ljharb commented 1 year ago

Totally fine with any of those alternatives.