fix(core): formatDate reads options.style, with the positional argument winning - #7813
Merged
Merged
Conversation
…nt winning `DateDisplayOptions` is the one bag `formatDate` / `formatRelativeDate` / `formatDateTime` share. PR #7621 added `style?: string` to it for `formatDateTime`'s `'compact'` grid face and only `formatDateTime` read it, so on `formatDate` the key was inert — and inert beside a POSITIONAL parameter of the same name. One function, two spellings for one concept, one of them silently doing nothing, and the silent one is the spelling `formatDateTime` requires: formatDate(v, undefined, { style: 'short', locale: 'en-US' }) before "Jul 4, 2024" (the default face, no diagnostic) after "Jul 4, '24" This is the additive half of the maintainer's long-run ruling B on objectui#7443 (comment 5539935824): both functions accepting `options.style`. The precedence between the two spellings is PINNED rather than left to implementation order: the positional argument wins, and `options.style` is consulted only when the positional slot is `undefined` (`??`, not `||`, so an empty string still counts as given and still renders the default face). It is the only direction that is purely additive — it fires exactly on the input that is a silent no-op today — and the reverse would let a key aimed at a SIBLING function outrank an argument written for this call, which is objectui#7694's shape and the silent-override half of objectui#4272. `formatRelativeDate` still does not read the key; the ruling names `formatDate` only. Its out-of-window delegation to `formatDate` therefore strips `style` (`absoluteFallbackOptions`), which both preserves that function's behaviour exactly and keeps `formatRelativeDate(v, { style: 'relative' })` from recursing until the stack runs out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7745
The additive half of maintainer ruling B on objectui#7443 (comment 5539935824): both
functions accepting
options.style. Direction not re-litigated.Base for every reading below:
a617bb8d3(recorded at worktree creation — not atwo-dot
origin/main..HEADdiff, which over-reports under concurrent landings).All readings taken at head
0ef85f821, which is also the head of this branch.Before / after
v = '2024-07-04T07:00:00.000Z', container TZUTC,Intllocaleen-US.formatDate(v, undefined, { style: 'short', locale: 'en-US' })"Jul 4, 2024""Jul 4, '24"That is the only line that moved. The same 16-reading probe was run against the
unmodified tree and the fixed tree and diffed; the diff is one line. Everything else —
formatDate(v, 'short', …)="Jul 4, '24",formatDate(v, undefined, …)="Jul 4, 2024",formatDate(v, 'relative', …)="Jul 4, 2024", and everyformatDateTime/formatRelativeDatereading — is byte-identical across the change.The precedence, and why this direction
The positional argument wins.
options.styleis consulted only when the positionalslot is
undefined—style ?? options?.style.that is a silent no-op today (positional absent, key present). Every call that renders
a face today renders the same face after — measured above, not asserted.
a sibling. That is this module's own convention:
dueLikeandtare read byformatRelativeDatealone. A caller that built{ style: 'compact', locale }forformatDateTimeand reused the bag informatDate(v, 'short', bag)must keep itsshort face. A key aimed at a sibling must not outrank an argument written for THIS
call — objectui#7694's shape (an alias overwriting a canonical key), and the
silent-override half of objectui#4272.
??, not||.formatDate(v, '', bag)renders the default face today, so anempty string counts as GIVEN and does not fall through to the key.
||would havemoved it.
Pinned in three places so it cannot drift back to "whatever the implementation order
happens to be": the
formatDateJSDoc, theDateDisplayOptions.styleJSDoc, and thetest file.
The pin
packages/core/src/utils/__tests__/date-display.optionsStyle-7745.test.ts— 14 cases,including both spellings supplied together:
formatDate(v, 'short', { style: 'long', locale })is"Jul 4, '24"formatDate(v, 'long', { style: 'short', locale })is"Jul 4, 2024"formatDate(d, 'short', { style: 'relative', locale })equalsformatDate(d, 'short', { locale })— the pair where the loser is visibly differentformatDate(v, '', { style: 'short', locale })is"Jul 4, 2024"formatDate.lengthis 3, so dropping the positional slot turns redOne thing the card did not name: the relative path
formatRelativeDatedelegates toformatDatefor dates outside its ±7-day window. MakeformatDatereadoptions.styleand that delegation leaks the key intoformatRelativeDate— for out-of-window dates only — and, for{ style: 'relative' }, loops until the stack runs out. Both are measured below.So the out-of-window call strips the key (
absoluteFallbackOptions, a module-localnon-exported helper). It is a behaviour-PRESERVING guard, not a feature: it is what keeps
formatRelativeDatereading exactly as it read before this PR.Lit control (three legs, each hash-proved)
Each leg: hash the file, confirm it equals the HEAD blob, mutate, prove the mutation
landed by hash and by removed/injected occurrence counts (an empty hash reads as
FAILURE), run the pin file, restore with
git checkout HEAD -- ABSOLUTE_PATHundertrap … EXIT INT TERM, then prove restoration by state: blob hash equals the HEADblob AND
git diff HEADis empty. HEAD blob6cbabb21ccb0eb105722aaaf87b5185b3bfebd2cfor all three; each restored to that exact hash with an empty diff.
style ?? options?.styletostyle(revert the read)style ?? options?.styletooptions?.style ?? style(invert precedence)absoluteFallbackOptionsstripformatRelativeDatecases REDA and B turn disjoint sets red, so neither half of the deliverable can be green by
accident: no single mutation can hide behind the other.
Leg C's third red is honest and worth recording — my written prediction named two cases
and three went red.
formatDate(v, undefined, { style: 'relative' })on an out-of-windowdate recurses, so the equivalence loop in "the options spelling IS the positional
spelling, for every face" throws rather than mismatches. The two measured failures the
strip prevents:
Regression surface: the two existing inert keys
dueLikeandtbehave exactly as before — read byformatRelativeDatealone, silentlyinert everywhere else. Measured on both sides of the change, identical:
formatDateTime(v, { dueLike: true, t: () ='X', locale })"Jul 4, 2024, 07:00 AM"formatDate(v, undefined, { dueLike: true, t: () ='X', locale })"Jul 4, 2024"formatDateTime(v, { style: 'compact', locale })"7/4/2024 7:00 am"formatDateTime(v, { locale })"Jul 4, 2024, 07:00 AM"formatRelativeDate(v, { style: 'compact', locale })"Jul 4, 2024"All five are pinned in the test file, so they cannot drift silently later.
Judgement: should
formatRelativeDateread it too? — NOT in this PRThe ruling names
formatDateonly, so nothing here was added for it. Reported for thenext card rather than assumed:
It is not genuinely style-less, but it is not a one-liner either. Two candidate
meanings, and only one is reachable today:
face exists to select. Adding the read would accept a key with nothing to name.
Recommend no.
formatRelativeDate(v, { style: 'short' })on a two-year-old date could render"Jul 4, '24". Recommend yes — but as a deliberate card, because it makes the±7-day boundary a visible face boundary (
"In 2 days"inside,"Jul 4, '24"outside) and changes live grid/gantt output for any caller already threading a shared
bag.
The strip added here is precisely the line that card would flip, and leg C is the
measurement of what flipping it costs — so that card has its before/after already taken.
Changeset gate
node scripts/check-changeset-presence.mjs, verbatim, before and after:Acted on:
.changeset/7745-formatdate-options-style.md, scoredpatch(a fix thatmakes an already-declared key do what it says; the only output that moves is a call that
was silently rendering the wrong face).
node scripts/check-changeset-no-major.mjs:✅ No changeset declares a 'major' bump.Verification, all at
0ef85f821pnpm --filter @object-ui/core testpnpm turbo run type-check --filter @object-ui/core --concurrency=2pnpm turbo run lint --filter @object-ui/core --concurrency=2pnpm exec vitest run packages/core/src/utils/__tests__/date-display.optionsStyle-7745.test.tspnpm exec vitest run packages/fields/src/__tests__/date-display.reexport-identity.test.ts date-formatter-residue-4272 datetime-compact-style-7443Exit codes captured before any pipe (redirect first, then read
$?); the wrapper's ownVERDICT line was read rather than a bare
$?.Not measured by "type-check is green" unless stated:
tsc -p tsconfig.test.json --listFileswas checked to confirm the new test file is in that program (1occurrence) — so the green does cover it.
Lint scope, declared: the run is
turbo run lint --filter @object-ui/core, which isthe per-package
eslint .CI runs for this package, i.e. the whole population for theonly package this PR touches.
--format jsonreports 215 files in eslint's ownpopulation; both changed files are in it. eslint.config.js declares no
parserOptions.project/projectService, so linting is not type-aware and this diffcannot move a verdict on any untouched file. My two files: 0 errors;
date-display.tscarries 4
no-explicit-anywarnings, unchanged (as anycount 4 before, 4 after);the new test file 0/0.
Clause-② remains
noNo exported type or symbol is added or rewritten.
DateDisplayOptionskeeps exactly itsfour keys with exactly the same types —
style?: stringwas already public (PR #7621).formatDate's signature is unchanged (arity 3, pinned). The one new function,absoluteFallbackOptions, is module-local and not exported. JSDoc prose onDateDisplayOptions.styleand on the module header was rewritten because it stated theopposite of the new behaviour; that changes documentation, not the declared type. Flagged
for the reviewer rather than self-graded.
Instrument note for other seats
pnpm --filter PKG exec vitest run PATHis refused by this repo's guard(objectui#3378): running vitest from a package directory silently runs the wrong test set
(it reported
Test Files 22 passedfromapps/consolewhile running none of the targetpackage's). The guard caught it loudly rather than going falsely green. Correct form is
from the repo root:
pnpm exec vitest run packages/core/src/….🤖 Generated with Claude Code
https://claude.ai/code/session_01KbJQ1y1J12nZxYzFWhP8Q3
Generated by Claude Code