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

Fallbacks to DefaultMergeCalendarFields should be removed (or made unconditional) #2463

Closed
gibson042 opened this issue Jan 6, 2023 · 5 comments

Comments

@gibson042
Copy link
Collaborator

CalendarMergeFields looks up a mergeFields method on its input calendar instance to invoke, but substitutes use of the DefaultMergeCalendarFields operation if no value is found. mergeFields itself exists on Temporal.Calendar.prototype in the primordial state, thinly wrapping DefaultMergeCalendarFields in the absence of ECMA-402 and extended in ECMA-402 to include built-in support for non-ISO-8601 calendars.

This has the net effect of delete Temporal.Calendar.prototype.mergeFields continuing to "work" but removing built-in calendar-sensitive behavior, which a) bad in its own right, and b) divergent from similar manipulation of other built-in methods (e.g., CalendarDateFromFields unconditionally invokes dateFromFields and will therefore fail if the built-in method is removed).

I see two general options for resolution:

  1. Remove the fallback and instead always look up and invoke methods.
  2. Remove the fallback and instead always always use built-in operations for built-in calendars, effectively ignoring the prototype (but presumably leaving the methods as thin wrappers around built-in operations for use by user-defined subclasses).

I think option 2 would be most consistent with the push in #1808 to minimize action at a distance.

@ptomato
Copy link
Collaborator

ptomato commented Jan 6, 2023

Didn't we just decide this yesterday already in #2310?

@gibson042
Copy link
Collaborator Author

The decision of #2310 requires the presence of (w.l.o.g.) mergeFields on every custom calendar instance, but does not cover what to do regarding built-in calendars when the built-in method has been removed or replaced.

@ptomato
Copy link
Collaborator

ptomato commented Jan 6, 2023

I would say that's covered by #1808, the builtin-calendars-are-strings behaviour would imply option (2).

@gibson042
Copy link
Collaborator Author

I don't think that implication holds because built-in calendar instances will presumably always inherit from Temporal.Calendar.prototype and could fail to override any methods there, but as noted above, option 2 is most consistent with a goal to which #1808 contributes.

@ptomato
Copy link
Collaborator

ptomato commented Feb 14, 2024

This has been addressed somewhere along the line, probably by #2519. The fallback does not exist anymore.

@ptomato ptomato closed this as completed Feb 14, 2024
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

2 participants