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

Fewer CalendarFields/DateFromFields calls during many PlainYearMonth ops #2794

Closed
arshaw opened this issue Feb 29, 2024 · 3 comments
Closed

Comments

@arshaw
Copy link
Contributor

arshaw commented Feb 29, 2024

For these PlainYearMonth operations...

  • add
  • subtract
  • since
  • until

...it essentially converts to a PlainDate by going to the start-of-month, then carries out its operation, then converts back to PlainYearMonth.

To convert to the start-of-month, it uses a very heavyweight set of operations:

  • CalendarFields
  • CalendarDateFromFields
  • (and re-parse all fields previously parsed)

It's much more efficient to leverage the CalendarProtocol's day() method to move to the start-of-month. Here's what fullcalendar's polyfill does:

export function moveToMonthStart(
  calendarOps: { day: DayOp },
  isoFields: IsoDateFields,
): IsoDateFields {
  return moveByIsoDays(isoFields, 1 - calendarOps.day(isoFields))
}

I'll experiment with making the reference polyfill do this. I'll open a PR...

@arshaw arshaw changed the title Fewer CalendarFields/DateFromFields calls during almost all PlainYearMonth ops Fewer CalendarFields/DateFromFields calls during many PlainYearMonth ops Feb 29, 2024
@ptomato
Copy link
Collaborator

ptomato commented Mar 1, 2024

Long story, but I'm not sure we can assume that moveByIsoDays(isoFields, 1 - calendarOps.day(isoFields)) is equivalent to .with({day: 1}). See #1315.

tl;dr ICU may in the future get a historically accurate Julian-Gregorian calendar, which skips 10 days in October 1582. (Or some later date, depending on how quickly the jurisdiction adopted the calendar reform.) The day() method of the Calendar protocol gives the calendar number of the day, not the 1-based index of the day. So October 15, 1582 is only 4 days from the start of the month, not 14 days.

This isn't a concern with any current builtin calendar, but we decided that we need to be robust against it.

@arshaw
Copy link
Contributor Author

arshaw commented Mar 5, 2024

Wow, I'll add that to the list of strange things I've learned about calendar systems!

Makes sense that .with() would need to be used for bulletproofing.

Closing this issue.

@arshaw arshaw closed this as completed Mar 5, 2024
@justingrant
Copy link
Collaborator

I'll add that to the list of strange things I've learned about calendar systems!

@arshaw You may want to take a look at these sections in the docs:

Feel free to PR additions/edits to those docs if you learn something strange about calendars that isn't represented there!

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

3 participants