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

size-suggestion: Consider merging three Calendar.p.*FromFields methods into one #2851

Closed
justingrant opened this issue May 20, 2024 · 5 comments
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns

Comments

@justingrant
Copy link
Collaborator

If we don't remove Temporal.Calendar in #2826, should we reduce its method count by two by merging dateFromFields, monthDayFromFields, yearMonthFromFields into a single fromFields method that accepts a kind parameter to determine the desired output type?

This issue is adapted from suggestions made by @FrankYFTang to help reduce the size of Temporal to address concerns from implementers (especially Android and Apple Watch) that Temporal's install size and/or in-memory size is too large.

@justingrant justingrant added the size-suggestion Suggestions to shrink size of Temporal to address implementer concerns label May 20, 2024
@gibson042
Copy link
Collaborator

I'd much prefer separate methods to combined parameterized methods, which are inherently less approachable and more complex.

@ptomato
Copy link
Collaborator

ptomato commented May 20, 2024

If we did end up keeping the Calendar protocol in roughly its current form, I'd be OK with this. It's less ergonomic, but that's something we've decided is OK for custom calendar use cases. It would make the builtin implementations of fromFields() more complicated as well, but it sounds like at least that's a preferable tradeoff for V8 according to Frank.

@FrankYFTang
Copy link
Contributor

FrankYFTang commented May 21, 2024

I'd much prefer separate methods to combined parameterized methods, which are inherently less approachable and more complex.

be aware when I suggest this, it is part of a bigger counterproposal to issues/2854
so the choices in front of us are

  1. keep all 3 as is
  2. merge 3 methods into 1 method in Calendar
  3. remove all 3 as stated in size-suggestion: Remove Temporal.Calendar class and protocol #2854

@justingrant
Copy link
Collaborator Author

Meeting 2024-05-23: In #2854, we're proposing removing the Temporal.Calendar class and protocol, so this issue doesn't apply to Temporal V1.

However, there is wide interest in creating a follow-up proposal to enable custom calendars, and the ideas proposed in this issue seem like simpler and more efficient solution than the current custom calendar design. Anyone working on a future custom calendar proposal should refer to this issue for inspiration.

@ptomato
Copy link
Collaborator

ptomato commented Jun 13, 2024

With #2854 approved, this issue no longer applies.

@ptomato ptomato closed this as completed Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size-suggestion Suggestions to shrink size of Temporal to address implementer concerns
Projects
None yet
Development

No branches or pull requests

4 participants