Repository navigation
[scheduler] Type the plain dates through the BaseTemporal namespace - #24034
flaviendelangle merged 11 commits into
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
c1210ae to
6eb4fbe
Compare
PR reviewOne merge-blocking regression, and it comes from the one semantic change hidden in this otherwise mechanical migration: Bugs (3)1. 🔴 Instants past 9999-12-31 in the event's timezone now throw while processing eventsLocation: const isoDate = dayNumberToIsoDate(dayNumber);
plainDate = new BaseTemporal.PlainDate(isoDate.year, isoDate.month, isoDate.day);Before the PR this was
On the merge base all three produce an unvalidated 10000-01-01 and the views work, because every consumer compares day numbers. Failure scenario: a backend that uses 9999-12-31 as its "no end date" sentinel (SQL Server Fix: keep 2. 🟡 The recurrence engine throws in the first and last 18 days of the supported range, where it didn't beforeLocation: skippedDays = getSkippedDays(
plainDateFromDayNumber(period * SKIPPED_DAYS_PERIOD),
plainDateFromDayNumber((period + 1) * SKIPPED_DAYS_PERIOD),The description lists these as "the edge of the supported range, like the dates after 9999", but they are regressions of this PR, not pre-existing limits: on the merge base all of them go through the unvalidated
Month and week views ending on 9999-12-31 already threw on master ( Failure scenario: Fix: compute the period bounds on day numbers and clamp them ( 3. ℹ️
|
| Operation | master | PR |
|---|---|---|
| day number of a cached date | 4.8 (.dayNumber) |
20.3 (WeakMap hit) |
day number of a fresh date (add()/with()/from() result) |
4.8 | 254.9 |
| date from a day number, hit | 37.9 (createPlainDate) |
18.6 |
| date from a day number, miss | 37.9 | 618 |
compare(a, b) |
7.3 | 32.5 |
The miss of getDayNumber() reads year, month and day through three getters that each recompute fromDayNumber() on the copy (fields are lazy since #23969), then runs isoDateToDayNumber. Every add()/with()/from() result is a fresh instance, so it always misses: RecurringSeries.ts:377,386,491 (.add({ days: ±1 })), getDayList.ts:24, adapter.ts:62 (createEndOfPlainDate), getRecurringEventOccurrencesForVisibleDays.ts:47, date-utils.ts:84-89 (getWeekNumber, two fresh dates per agenda week row). The miss of plainDateFromDayNumber() pays toInteger ×3, checkIsoDate, checkCalendarRange, Object.freeze, two map writes and the periodic clear(): a WEEKLY series with COUNT started in 1970 and viewed today touches ~2 900 distinct days on first expansion, ~1.8 ms versus ~0.1 ms on master. All of this is sub-frame at the Scheduler's call counts, so no user sees it, but "a cached read takes about as long as the internal field read" is 4x off and the cold cost is exactly what the warm iterate.bench.ts / grouped-by-day benches can't show.
Fix: where the PR itself builds a ±1-day neighbour, use plainDateFromDayNumber(getDayNumber(x) ± 1) so the result is pre-registered in both caches (the five sites above). In getWeekNumber, isoDateToDayNumber(year, 1, minimalDays) replaces getDayNumber(BaseTemporal.PlainDate.from({ year, month: 1, day: minimalDays })); date-utils.ts already imports from ./plain-date-utils/dayNumber and the ESLint zone only restricts the copy. Optionally add a cold-cache bench (the WEEKLY-since-1970 case) so the numbers in the description cover both paths.
3. ℹ️ draggedDay round-trips number → date → number
Location: packages/x-scheduler-internals/src/calendar-grid/day-event/CalendarGridDayEvent.tsx:74,90,207, calendar-grid/day-cell/useDayCellDropTarget.ts:52
The producer builds plainDateFromDayNumber(n) and the only reader does getDayNumber(data.draggedDay). CalendarGridDayEvent.DragData is internal (not SchedulerOccurrencePlaceholderExternalDragData), so a draggedDayNumber: number drops two lookups and both BaseTemporal imports. Pre-existing shape, but the PR touched both ends.
4. ℹ️ Design question: duplicate the ISO math now, or keep plain-date-utils as the one internals-aware layer until the package lands?
Location: packages/x-scheduler-internals/src/internals/utils/plain-date-utils/dayNumber.ts, eslint.config.mjs:293-325
dayNumber.ts is a transliteration of the copy's iso.ts / calendar.ts (toDayNumber, fromDayNumber, getIsoDayOfWeek, getStartOfWeek, getStartOfMonth, addIsoMonths, LAST_SUPPORTED_DAY): 91 lines plus 86 test lines plus the two caches, all so that the Scheduler is package-ready today. The alternative is two lines in the block's ignores for plain-date-utils/{plainDate,dayNumber}.ts, which is what that folder already was on the merge base (it imported createPlainDate and ISO_CALENDAR_ID), and writing the Hinnant code in the PR that swaps the copy for @base-ui/plus. Same end state, the duplication just moves. I'd want to know how close that swap is before picking; the stricter 0..9999 behaviour of plainDateFromDayNumber (bugs 1 and 2) is the one consequence that isn't a wash either way.
5. ℹ️ The esbuild cost of the namespace is +10.6 kB minified, not +3.1 kB
Location: packages/x-scheduler-internals/src/internals/utils/base-temporal/index.ts:1
Measured on a three-line entry that uses only PlainDate.compare and the constructor, minified: esbuild 0.28 emits 25 034 B through export * as BaseTemporal versus 14 436 B importing the class by name; Rolldown 1.2 and webpack 5 give 14 448 / 14 390 B either way. The +3.1 kB in the description is presumably the gzip figure, worth saying. esbuild also turns the namespace into a getter object, so every BaseTemporal.PlainDate access is a call under it. Only esbuild-based consumers pay (Vite dependency pre-bundling in dev, tsup, Bun); Next.js and Vite production builds don't. Fine to accept, as the description does, as long as the unit is explicit. Also: eslint.config.mjs:282-284 and :296-299 spell the two copy directories twice, and the zones a third time; a shared BASE_TEMPORAL_COPY_DIRS constant like INTERNALS_INDEX_RESTRICTED_ZONES would do.
Docs (4)
1. 🟡 The copy's revision, the "public API only" rule and the mirror process are recorded nowhere in-repo
Location: packages/x-scheduler-internals/src/internals/utils/base-temporal/ (no README or header), eslint.config.mjs:293-295, plain-date-utils/plainDate.ts:35-38
The only in-tree hints are the two ESLint comments. The copy at this PR's base isn't the 578caaf86 revision the new index files come from (it still has PlainDateCache and the "calendar grids" comments that #24040 removes), and nobody can tell that without rebuilding and diffing. A README.md in base-temporal/ stating the upstream path and SHA, "change it upstream first, then copy back", and the rule this PR introduces ("import only BaseTemporal from the index; day-number arithmetic lives in plain-date-utils/dayNumber.ts; never read plainDate.dayNumber, it's an internal field the package won't export") would serve both this PR and #24040. The last point belongs in getDayNumber()'s JSDoc too: the class still declares public readonly dayNumber (PlainDate.ts:355), autocomplete offers it, and ESLint can't catch a property read.
Failure scenario: the next contributor reads plainDate.dayNumber, or edits the copy directly and loses the change at the next sync.
2. 🟡 Description accuracy
- "A second round found that
countBefore()built the day before the start of a daily series with an interval of millennia, before the supported years, which the constructor rejects." The constructor only rejects it because this PR madeplainDateFromDayNumbervalidating; on master that line was harmless. Same for "the occurrences of a recurring series throw in the first 18 days of year 0 and the last 18 days of 9999", presented as a pre-existing edge: they don't throw on master (bug 2). - "New tests: ... the dates created by the methods of the plain dates" is in
plainDate.test.ts, buttimeZone.test.tsanddate-utils.test.tsgained no assertions. - "No public change": true for
@mui/x-schedulerand the premium package, but@mui/x-scheduler-internals/internalsis a published entrypoint (package.jsonexports) and losesPlainDate,ZonedDateTimeand thePlainDateclass. Fine if the internals packages are treated as unversioned, worth one clause. - Bundle table: say minified or gzipped.
3. ℹ️ The second ESLint zone message doesn't say what to import instead
Location: eslint.config.mjs:318
message: 'These are internals of the BaseTemporal copy.',The first zone's message is actionable ("Import BaseTemporal from the index of the copy, and the day number helpers from plain-date-utils"); this one should end with "Use BaseTemporal.ZonedDateTime from internals/utils/base-temporal instead." The block comment at :293-294 says "the BaseTemporal namespace of its index", while except: ['./index.ts'] also allows the index's type exports (PlainDateFields, ZonedDateTimeOptions...), which is the intent; "the exports of its index" matches.
4. ℹ️ Cache and loop comments
plainDate.ts:15: the WeakMap line should say why it has no bound (entries die with their date, so it can holdadd()/from()results) and thatplainDateLookup.clear()leaves it alone.plainDate.ts:18-20should state the contract callers now lean on: same instance for a day number until the cache is cleared at 10 000 entries, so compare withequals()/day numbers, never===.getDayList.ts:27"without building them, as they're cached" overstates a cold cache; "returns the cached instance of each day instead of building one withadd()" is what the rewrite buys.models/event.ts:426-427says consumers "compare its day number with the day numbers of instants" but no longer points at how:getDayNumber(plainDate)vstoLocalDayNumber().plainDate.test.ts:18"should return the same plain date for a day number" asserts reference identity; "the same instance" says what it tests.
Verdict
Request changes — the validating plainDateFromDayNumber() turns instants past 9999-12-31 in east-of-UTC timezones (a common "no end date" sentinel) into a render-time throw that master handled.
🤖 Review generated with Claude Code
a85420a to
fd85452
Compare
|
Thanks for the review. I checked each item against the code; fbbefbd and fd85452 apply the ones that held up. The branch is now stacked on #24040, which syncs the BaseTemporal copy. Bugs
Tests
Simplifications
Docs
🤖 Comment generated with Claude |
PR review (round 2)Nothing merge-blocking anymore. Round 1's blocker is fixed the right way: with #24040 underneath, What's new in this round: the shared 10,000-entry date cache turns long COUNT series into a steady-state cliff that master didn't have, the moved range end creates throws master didn't have either, and the Tokyo regression test doesn't pin what it's named after. Bugs (7)1. 🟡 Two long COUNT series thrash the plain-date cache on every recomputeLocation: if (plainDateLookup.size >= MAX_CACHED_PLAIN_DATES) {
plainDateLookup.clear();
Master had no shared state, so no cliff. This is not round 1's cold-fill cost (a one-off, which the reply rightly called sub-frame): here the cache never warms. DAILY without timezone skips takes the arithmetic branch ( Failure scenario: a calendar with two or more imported weekly COUNT series created decades ago; every navigation, edit or resource toggle re-expands them at ~3 ms each, 6 to 8x master, forever. Fix: walk day numbers in 2. 🟡 MONTHLY and YEARLY series landing after September 13th, 275760 throwLocation: candidate = plainDateFromDayNumber(getDayNumber(month) + this.monthlyTargetDay - 1); ? plainDateFromDayNumber(getDayNumber(monthStart) + day - 1)With the range ending on December 31st, these two could never leave the supported days: Failure scenario: a monthly series on the 20th that reaches the last month of the range crashes the view instead of ending, the same class round 1 reported for the 9999 edge. Fix: 3. ℹ️ Four spots throw at the start of the range, not twoLocation: plainDateFromDayNumber(period * SKIPPED_DAYS_PERIOD),The reply lists the WEEKLY week start and Fix: 4. ℹ️ "The last 31-day period stops at that day" holds only in UTCLocation: if (offsetAtDayStart !== readOffset(offsets.format, (day + 1) * MS_PER_DAY - 1)) {In any non-UTC series timezone, Fix: the same clamp in 5. ℹ️ The first instant of
|
rita-codes
left a comment
There was a problem hiding this comment.
See the review comment above.
Type the plain dates as `BaseTemporal.PlainDate`, call the statics through the namespace and read the day numbers through `getDayNumber()`, like the Base UI version of BaseTemporal. The namespace files are copied as is, and nothing is exported publicly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Leave the copied `zonedDateTime.ts` as it was. - Read the day numbers once in the loops instead of on each iteration. - Use `toLocalDayNumber()`, `getStartOfWeekDayNumber()`, `getStartOfMonthDayNumber()`, `addMonthsToDayNumber()` and `getDayCount()` instead of building a plain date to read its day number back. - Compare the plain dates of the agenda with `BaseTemporal.PlainDate.compare()` and `equals()`, and step a day with `add()` outside the occurrence loops. - Rename the imports where they were. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… occurrences `getDaysTheOccurrenceIsVisibleOn()` read the day number of each visible day for each occurrence. Through `getDayNumber()`, that's a call per day and per occurrence where the field was a property read: the month view case of the grouped-by-day bench was 6% slower. The caller now reads them once. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keeps one `adapter.getTime()` reference in the series instead of two. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Scheduler only imports the `BaseTemporal` namespace from the index of the copy, so it can move to the package of the library without the internals of the copy: - `plain-date-utils/dayNumber.ts` owns the ISO arithmetic of the day numbers: `isoDateToDayNumber()`, `dayNumberToIsoDate()`, the start of a week or a month, the addition of months, `MS_PER_DAY` and `LAST_SUPPORTED_DAY`. - `getDayNumber()` reads the ISO fields of a plain date, and `plainDateFromDayNumber()` creates it with the `BaseTemporal.PlainDate` constructor. The plain dates of the day numbers and the day numbers of the plain dates are cached: the dates are immutable, and the views read the same days on every render. - `getDayListFromPlainDates()` walks the day numbers, so its days come from the cache. - `toWallClockIsoString()` formats the time with `BaseTemporal.PlainTime`. - `PlainDateDuration` is the parameter of `BaseTemporal.PlainDate#add()`. - An ESLint zone forbids the other modules of the copy outside of it. - `toEqualPlainDate()` reads the public fields of a plain date. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`BaseTemporal.PlainTime` added the class to the bundles that don't use it, like the Event Calendar. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… counts one With an interval of millennia, the day before the first one fell before the supported years, which the constructor of the plain dates rejects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… Scheduler Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The BaseTemporal copy supports the days of `Date`, up to September 13th, 275760, so `LAST_SUPPORTED_DAY` moves there. An instant at the end of 9999 east of UTC, such as a "no end date" UNTIL, exDate or event end in Tokyo, now gives a date of the year 10000 instead of throwing. - The last 31-day period of skipped days stops at the last supported day, as the day after it has no plain date. - The ISO arithmetic is checked on every day of the years 0 to 9999, and on every 9973rd day of `Date`. - The index of the copy exports the `WithFields` types, like the Base UI version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- `getWeekNumber()` reads the day number of the first week from its ISO date, without building a plain date. - The ESLint config names the two directories of the copy once, and its messages say what to import instead. - The comments of the caches say how long their entries live, and that the plain dates of a day number are the same instance. - A test checks that the wall-clock strings pad every part of the time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- The monthly and yearly days after September 13th, 275760 return no occurrence instead of throwing, like the weekly ones. - The Tokyo UNTIL and exDate test queries the last days of 9999 and 10000. - `plainDateFromDayNumber()` says that it throws outside the supported days, and its tests cover the first supported day and the dates read before the cache is emptied. - The Timeline header reads the start of a year with `getStartOfYearDayNumber()`, and the day grid reads the day number of its day once for all the occurrences. - The ESLint zones of the BaseTemporal copy join the rule of all the files and cover `test/`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fd85452 to
c475d23
Compare
|
Thanks for the second round. c475d23 applies the items that held up. #24040 is merged, with your two clauses in its description, and this branch is rebased onto master. Bugs
Tests
Simplifications
Docs
🤖 Comment generated with Claude |
The Scheduler now reads its plain dates the way the Base UI version of BaseTemporal exposes them. Nothing is exported publicly yet.
Changes
BaseTemporal.PlainDate. It's the type the namespace exposes, instead of thePlainDateclass. The internal exports of@mui/x-scheduler-internals/internalsgiveBaseTemporalinstead of thePlainDateandZonedDateTimeclasses.BaseTemporal.PlainDate.from(),BaseTemporal.PlainDate.compare()andBaseTemporal.ZonedDateTime.from().getDayNumber(), because the interface has nodayNumber. The internal exports give it next toBaseTemporal.base-temporal/index.tsandbase-temporal/index.parts.tsare copied as is from the Base UI version. No file that was already in the copy changes.BaseTemporalfrom the package of the library:plain-date-utils/dayNumber.tsowns the ISO arithmetic of the day numbers:isoDateToDayNumber()anddayNumberToIsoDate(), the start of a week or a month, the addition of months,MS_PER_DAYandLAST_SUPPORTED_DAY. They replace the internals of the copy.getDayNumber()reads the ISO fields of a plain date, andplainDateFromDayNumber()creates one with theBaseTemporal.PlainDateconstructor. Both cache their results: the plain dates are immutable, and the views read the same days on every render. A cached read is aWeakMaporMaplookup instead of a field read. The first read of a date thatadd()orfrom()creates, and a date missing from the cache, cost more than the internal field and creation. The bundled benches run with warm caches.getDayListFromPlainDates()walks the day numbers, so its days come from the cache.PlainDateDurationis the parameter ofBaseTemporal.PlainDate#add().import/no-restricted-paths) forbid the other modules of the copy in the packages and intest/. They join the zones of all the files, and the copy keeps only the zones of the internals index.toEqualPlainDate()checks the public fields of a plain date instead of its internal day number.toLocalDayNumber(),getStartOfWeekDayNumber(),getStartOfMonthDayNumber(),getStartOfYearDayNumber(),addMonthsToDayNumber()andgetDayCount(). The privategetStartOfRRuleWeek()goes.BaseTemporal.PlainDate.compare()andequals(), and moved by a day withadd().getDaysTheOccurrenceIsVisibleOn()takes the day numbers of the visible days, read once by its caller for all the occurrences. Reading them throughgetDayNumber()for each occurrence and each day made the month view case of the grouped-by-day bench 6% slower than the field read.No public change:
BaseTemporalandgetDayNumber()are only exported from@mui/x-scheduler-internals/internals, which also stops exporting thePlainDateandZonedDateTimeclasses. It's a published entry point, but only for the Scheduler packages. The models that hold plain dates, such asSchedulerProcessedDay,EventCalendarFetchRangeand the preset definitions of the Timeline, aren't re-exported by@mui/x-scheduleror@mui/x-scheduler-premium.Bundle size
Measured from source with Rolldown, minified, with third-party packages external and without the error minifier, gzip level 9.
event-calendarevent-calendar-premiumevent-timeline-premiumesbuild keeps every class of a namespace when the namespace is re-exported by name. The entries that don't use them grow there: +3.14 kB gzipped for
event-calendarand +3.13 kB forevent-timeline-premium, which now includePlainTime,PlainDateTimeandZonedDateTime. Rolldown (the bundler of Vite) and webpack 5 drop them. A reduced case of the same re-exports confirmed this.Validation
pnpm test:unit --project "x-scheduler*" --runandpnpm test:browser --project "x-scheduler*" --runpass.The benches were run bundled with Rolldown, as the Vitest module runner reads every imported function through a getter: under Vitest, the
getDayNumber()calls of the loops cost up to 30% that a bundle doesn't pay. Bundled, they stay within their noise, apart from the header walk of the Timeline, which builds fewer plain dates:iterate()is about twice as fast fordayAndMonthandmonthAndYear. Reading the plain dates through their public API doesn't move the benches either, as the day numbers and the dates come from the caches.A
/simplifypass on the change made the simplifications above.A review round with Codex and the base-ui-review skill, on the first commits, approved with one finding rejected.
add({ days: -1 })throws on the first supported date, the edge of the supported range.A second round, on the move to the public API, found that
countBefore()built the day before the start of a daily series with an interval of millennia, before the supported days.createPlainDate()built it without checking it, but theBaseTemporal.PlainDateconstructor this PR uses rejects it. It's fixed and tested.A third round found that the Scheduler's ESLint zone replaced the one of all the files, which forbids importing the
internals/index.tsbarrel. The zones of the copy are now part of the rule of all the files.A review found that the validating constructor made the instants at the end of 9999 throw east of UTC, such as a "no end date"
UNTIL=99991231T235959Z, exDate or event end in Tokyo, which fall on January 1st, 10000. [scheduler] Sync the BaseTemporal copy with the Base UI version #24040 brings the days ofDateto the copy, so these instants give dates of the year 10000, andLAST_SUPPORTED_DAYmoves to September 13th, 275760. The last 31-day period of skipped days stops at that day, as the day after it has no plain date. Tests cover the UNTIL, the exDate and the event end in Tokyo.A second review found that the monthly and yearly days after September 13th, 275760 threw. They return no occurrence now, like the weekly ones. Unlike Temporal, the first instant of
Datethrows in the zones west of UTC, where it falls on the day before the first supported day.New tests: the ISO arithmetic of the day numbers on every day of the years 0 to 9999 and every 9973rd day of
Date, the caches of the plain dates and of their day numbers, the dates created by the methods of the plain dates and the dates in another calendar.TypeScript, ESLint and Prettier pass.
I have followed (at least) the PR section of the contributing guide.
🤖 Generated with Claude Code