diff --git a/.changeset/7265-components-sort-direction-bound.md b/.changeset/7265-components-sort-direction-bound.md new file mode 100644 index 0000000000..74df1be44c --- /dev/null +++ b/.changeset/7265-components-sort-direction-bound.md @@ -0,0 +1,17 @@ +--- +--- + +Bind `@object-ui/components`' `SortDirection` to `@objectstack/spec` (objectui#7265, +the `@object-ui/components` slice of the `check:spec-symbols` DEBT burn-down). + +The DataTable renderer declared `SortDirection` module-locally, under the exact +name `@objectstack/spec/shared` exports — the planted-premise class that guard +exists to stop. It now imports the spec's own type. The `null` third state the +declaration carried is not a third direction but the absence of one, so it is +spelled `| null` at the single state slot that holds it, the way `sortColumn` +already spelled its own empty case, with the reason written there. + +Type-only. `import type`, a deleted type alias and an erased `useState` type +argument leave the emitted JavaScript byte-identical and the package's `.d.ts` +unmoved — the declaration was never exported, so nothing published moves and no +package is released by this change. diff --git a/packages/components/src/__tests__/share-filter-sort-spec-parity.test.ts b/packages/components/src/__tests__/share-filter-sort-spec-parity.test.ts index acb35c373b..43bb40836a 100644 --- a/packages/components/src/__tests__/share-filter-sort-spec-parity.test.ts +++ b/packages/components/src/__tests__/share-filter-sort-spec-parity.test.ts @@ -29,6 +29,19 @@ * a tripwire built on `Object.keys(await import(…))` would pass while proving * nothing. So this reads each subpath's `.d.ts` through the TypeScript checker, * exactly as `scripts/check-spec-symbol-derivation.mjs` does. + * + * ## Later arrivals are APPENDED under their own card + * + * Six / four / two is a measurement of the batch-5 burn-down at the time it was + * taken; re-counting it here whenever a new symbol arrives would make it + * unreproducible. `SortDirection` (objectui#7265) is the first such arrival — a + * module-local mirror in the DataTable renderer that rule 1 could not see until + * its export filter was dropped. It went the BIND route, and because the + * declaration is now an import rather than a derivation there is no local name + * left to reach for: what is pinned below is the SPEC-side property the binding + * rests on, the way the app-shell slice pinned its own module-local names. The + * absence of the old declaration is pinned where the scanner can execute it, in + * scripts/__tests__/spec-symbol-ledger-components-7265.test.ts. */ import { describe, it, expect } from 'vitest'; @@ -45,8 +58,9 @@ import type { ShareLinkAudience as SpecShareLinkAudience, ShareLinkPermission as SpecShareLinkPermission, } from '@objectstack/spec/contracts'; -import type { SortItem as SpecSortItem } from '@objectstack/spec/shared'; +import type { SortItem as SpecSortItem, SortDirection as SpecSortDirection } from '@objectstack/spec/shared'; import type { FilterCondition as SpecFilterCondition } from '@objectstack/spec/data'; +import type { TableSortItem } from '@object-ui/types'; /** Every name `@objectstack/spec` exports from any subpath — types AND values. */ function specExportNames(): Set { @@ -241,3 +255,68 @@ describe('FilterBuilderCondition is NOT the spec ObjectQL AST', () => { expect(true).toBe(true); }); }); + +/* -------------------------------------------------------------------------- */ +/* objectui#7265 — `SortDirection`, the DataTable renderer's module-local */ +/* mirror. BOUND: the declaration is gone and the renderer imports the spec's */ +/* own type, with the third state confined to the state slot that carries it. */ +/* -------------------------------------------------------------------------- */ + +describe('the spec still owns `SortDirection` — the collision this route answers is real', () => { + it('reads the name off the spec, not off a list written here', () => { + expect( + SPEC_NAMES.has('SortDirection'), + '@objectstack/spec no longer exports `SortDirection`. The DataTable ' + + 'renderer imports it, so this is a build break rather than a silent ' + + 'drift — but it also means the burn-down that removed the local ' + + 'declaration no longer has a spec symbol to bind to, and the route has ' + + 'to be taken again.', + ).toBe(true); + }); +}); + +describe('the DataTable sort state IS the spec direction, widened only by absence', () => { + it('is pinned at compile time', () => { + // Guard-header cases 1, 2 and 2b: an `any` or `unknown` on the SPEC side + // answers every assignability question affirmatively, so binding to it would + // be a type-safety regression wearing a burn-down's clothes. + type _SpecNotAny = Assert, false>>; + type _SpecNotUnknown = Assert, false>>; + + // The third state is genuinely OUTSIDE the spec's vocabulary. This is the + // assertion the confinement rests on: if `null` were already admissible + // there, the `| null` at the renderer's state slot would be a no-op and the + // comment explaining it would be false. + type _NullIsNotADirection = Assert, false>>; + + // …and the two real members are, so the widening is by absence and nothing + // else. Read as an assignability pair rather than by restating `'asc' | + // 'desc'` here: a member list typed out in a test is the same hand copy the + // renderer just stopped keeping. + type _DirectionsSurviveTheWidening = Assert>; + type _WideningIsStrict = Assert, false>>; + + // The seam the non-null half flows into. `activeSort` builds a + // `TableSortItem` out of `sortDirection` verbatim, so the day the spec grows + // a direction this reds on the hand-declared `order` in @object-ui/types + // rather than on the renderer — which is where the drift would actually be. + // (@object-ui/types takes no dependencies and declares the shape instead of + // importing it; that is a documented decision of that package, and this is + // the tripwire on it.) + type _OrderIsTheSpecDirection = Assert>; + + expect(true).toBe(true); + }); + + it('the three-state cycle the `| null` exists for is covered behaviourally', () => { + // Stated here rather than re-tested: the client-side header cycle that + // PRODUCES the unsorted state is pinned by the `leaves client-side sorting + // exactly as it was` case in data-table-manual-sorting.test.tsx, and the + // manual-sorting side is pinned there too by `never asks for "no sort" — the + // third click returns to ascending`. Duplicating them here would make two + // places to keep honest; naming them makes the type pins above and the + // behaviour one file, which is what a reader deciding to "simplify" the + // `| null` away needs to find. + expect(true).toBe(true); + }); +}); diff --git a/packages/components/src/renderers/complex/data-table.tsx b/packages/components/src/renderers/complex/data-table.tsx index 95f7fba6a2..d992df3234 100644 --- a/packages/components/src/renderers/complex/data-table.tsx +++ b/packages/components/src/renderers/complex/data-table.tsx @@ -14,6 +14,7 @@ import { useGridFieldAuthoring } from '../../context/gridFieldAuthoring'; import { describeIgnoredBind, describeNonArrayData } from './dataTableBindDiagnostic'; import { ComponentRegistry, compareSortValues, evalRowPredicate, formatDate, formatDateTime, getSortValue } from '@object-ui/core'; import type { DataTableSchema, TableSortItem, TableColumnType } from '@object-ui/types'; +import type { SortDirection } from '@objectstack/spec/shared'; import { SchemaRenderer, toRenderableSchema, useRowPredicate, usePredicateScope } from '@object-ui/react'; import { createSafeTranslation } from '@object-ui/i18n'; import { @@ -62,8 +63,6 @@ import { DropdownMenuTrigger, } from '../../ui/dropdown-menu'; -type SortDirection = 'asc' | 'desc' | null; - /** * Inline-edit helpers: convert a stored cell value to the string a native * `` / `` expects, and back. @@ -928,7 +927,22 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { // State management const [searchQuery, setSearchQuery] = useState(''); const [sortColumn, setSortColumn] = useState(null); - const [sortDirection, setSortDirection] = useState(null); + // The sort state's second half. `SortDirection` is `@objectstack/spec`'s own + // export, imported rather than re-declared: this module used to hand-write + // `'asc' | 'desc' | null` under that exact export name, which is the planted- + // premise class `check:spec-symbols` exists to stop (objectui#7265). + // + // `null` is the ONE divergence, and it lives HERE rather than in the name + // because it is not a third direction — it is the absence of one, the + // unsorted end of the client-side header cycle in `handleSort`. That is the + // same "this half is empty" that `sortColumn` above already spells at its own + // slot, which is why folding it into a type would have been the odd one out. + // No `null` can reach the protocol's vocabulary: the sort comparator is past + // the `!sortDirection` guard in `sortedData`, and `activeSort` emits a + // `TableSortItem` only when both halves are set. The cycle that produces the + // third state is pinned by the `leaves client-side sorting exactly as it was` + // case in data-table-manual-sorting.test.tsx. + const [sortDirection, setSortDirection] = useState(null); const [selectedRowIds, setSelectedRowIds] = useState>(new Set()); const [currentPage, setCurrentPage] = useState(1); const [pageSize, setPageSize] = useState(initialPageSize); @@ -1279,7 +1293,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => { * directly, which under `manualSorting` would have written to state nothing * reads: a menu item that highlights, closes, and changes nothing. */ - const applySort = (columnKey: string, order: 'asc' | 'desc') => { + const applySort = (columnKey: string, order: SortDirection) => { if (manualSorting) { onSortChange?.([{ field: columnKey, order }]); return; diff --git a/scripts/__tests__/spec-symbol-ledger-components-7265.test.ts b/scripts/__tests__/spec-symbol-ledger-components-7265.test.ts new file mode 100644 index 0000000000..536141eddb --- /dev/null +++ b/scripts/__tests__/spec-symbol-ledger-components-7265.test.ts @@ -0,0 +1,200 @@ +import { describe, expect, it } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { SortDirectionEnum } from '@objectstack/spec/shared'; + +// Plain-JS CI helper. Its types are INFERRED from the .mjs source by +// `tsconfig.scripts.json` (`allowJs`), so no `@ts-expect-error` here. +import { scanFile } from '../check-spec-symbol-derivation.mjs'; + +/** + * objectui#7265, the `@object-ui/components` slice -- the one name this package + * held in rule 1's DEBT block, in one file. + * + * Sibling of `spec-symbol-ledger-core-7265.test.ts`, + * `spec-symbol-ledger-app-shell-7265.test.ts` and + * `spec-symbol-ledger-types-7265.test.ts`, same two-part shape, because a ledger + * needs both halves: + * + * 1. THE SITE. The real scanner, run over the real file. This is the half that + * reds if a local copy comes back -- deleting a name from a ledger is not a + * burn-down unless the declaration went with it. + * 2. THE BLOCK. Shrink-only is the card's own invariant, so it is asserted, + * not just respected. Stated as a ceiling rather than an equality so the + * next slice can shrink it further without touching this file. + * + * ⭐ What is specific to THIS slice: the route was decided against a third + * state, not against a member count. The renderer declared `SortDirection` as + * the spec's two members plus `null`, and `null` is the one thing the spec's + * vocabulary refuses -- so the rename question was live, and the answer came + * from what that member MEANS at the site. It is the ABSENCE of a direction, + * written once at the end of the client-side header cycle and read only by + * guards that guard `sortColumn` in the same breath, and `sortColumn` -- the + * other half of the same state pair -- already spelled its own empty case at its + * slot. So the concept is the spec's, the declaration is gone rather than + * re-derived, and the third state is confined to the state slot. The refusal + * that makes the confinement load-bearing is probed below in BOTH directions. + * + * ⚠️ What this file does NOT assert, on purpose: that the surviving DEBT rows + * are still there. A pin on those would fail on the next slice's success -- it + * would red on healthy progress, which is the opposite of a ratchet. + * + * ⚠️ The scanner is only evidence if it can fail, so the site assertion is + * paired with a fixture of the same kind that it MUST flag. A green scan with an + * empty `specNames` map, or over a path that does not exist, looks exactly like + * a green scan over a burned-down site. + */ + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); +const gateSource = path.join(repoRoot, 'scripts/check-spec-symbol-derivation.mjs'); + +/** The name that was BURNED DOWN by binding it to the spec. */ +const BOUND = 'SortDirection'; +/** The subpath that owns it. */ +const SUBPATH = '@objectstack/spec/shared'; +/** The single site it lived at. */ +const SITE = 'packages/components/src/renderers/complex/data-table.tsx'; + +/** + * The names rule 1 matches on, built for the one name this slice dealt with + * rather than typed out as a list -- a hand-written name list would keep + * asserting a collision after the spec stopped exporting the symbol. + */ +const specNamesForBound = (): Map> => new Map([[BOUND, new Set([SUBPATH])]]); + +/** Reads one `const NAME = { … };` block out of the gate's source text. */ +function ledgerBlock(name: string): string { + const text = fs.readFileSync(gateSource, 'utf8'); + const start = text.indexOf(`const ${name} = {\n`); + expect(start, `${name} block not found in the gate source`).toBeGreaterThan(-1); + const end = text.indexOf('\n};\n', start); + expect(end, `${name} block is not terminated`).toBeGreaterThan(start); + return text.slice(start, end + 3); +} + +const ledgerNames = (block: string) => [...block.matchAll(/^ {4}"([^"]+)",$/gm)].map((m) => m[1]); + +describe('the spec still owns the name this slice bound to', () => { + it(`\`${BOUND}\` is still a live \`${SUBPATH}\` vocabulary`, () => { + // The `import` above already fails the build if the enum disappears. This + // asserts the stronger thing the route rests on -- that the name still names + // a PARSER, not a type-only shim that would make the probes below vacuous. + // (`SortDirection` itself is `z.input` of this enum and so is type-only; it + // is read through the compiler in the package's own parity file.) + expect(typeof SortDirectionEnum.safeParse).toBe('function'); + expect(SortDirectionEnum.safeParse('asc').success).toBe(true); + expect(SortDirectionEnum.safeParse('desc').success).toBe(true); + }); + + it('…and it refuses the third state, which is why `null` had to be confined', () => { + // Both directions of the one divergence. If the spec ever admits `null` as + // a direction, the confinement at the state slot is spent and the comment + // explaining it has gone false -- this is the assertion that says so out + // loud instead of staying quiet. + expect(SortDirectionEnum.safeParse(null).success).toBe(false); + // Lit control: a refusal is only news if the parser accepts anything at all, + // and the two accepted values above are read back off the enum rather than + // restated, so a widened vocabulary shows up here rather than in prose. + const members = Object.keys( + (SortDirectionEnum as unknown as { _zod?: { def?: { entries?: Record } } }) + ._zod?.def?.entries ?? {}, + ); + expect(members.length).toBeGreaterThan(1); + expect(members).not.toContain('null'); + }); +}); + +describe('the site: the module-local mirror is gone', () => { + const found = () => scanFile(path.join(repoRoot, SITE), specNamesForBound()).map((f: { name: string }) => f.name); + + it(`rule 1 no longer sees \`${BOUND}\` -- the renderer imports it now`, () => { + expect(found()).toEqual([]); + }); + + it('the scanner can still see the shape it used to flag -- the control', () => { + // Same kind as the subject: the exact declaration this slice deleted. + // Written to a throwaway path so the control cannot be satisfied by + // anything already in the tree. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'spec-symbol-components-7265-')); + try { + const fixture = path.join(dir, 'relapse.ts'); + fs.writeFileSync( + fixture, + [ + "type SortDirection = 'asc' | 'desc' | null;", + '', + 'export const used: SortDirection = null;', + '', + ].join('\n'), + ); + expect(scanFile(fixture, specNamesForBound()).map((f: { name: string }) => f.name)).toEqual([BOUND]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); + + it('…and a DERIVED re-declaration is NOT flagged -- the other half of the control', () => { + // This proves the scan judges DERIVATION rather than counting names: a + // scanner that flagged this would make the first assertion above unpassable, + // so its green would mean nothing. It also records what the gate does and + // does not decide -- the union-extension spelling below is a legitimate BIND + // form too, and the gate is indifferent between it and the outright import. + // The site chose the import; the gate did not choose for it. + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'spec-symbol-components-7265-ok-')); + try { + const fixture = path.join(dir, 'derived.ts'); + fs.writeFileSync( + fixture, + [ + "import type { SortDirection as SpecSortDirection } from '@objectstack/spec/shared';", + '', + 'type SortDirection = SpecSortDirection | null;', + '', + 'export const used: SortDirection = null;', + '', + ].join('\n'), + ); + expect(scanFile(fixture, specNamesForBound()).map((f: { name: string }) => f.name)).toEqual([]); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } + }); +}); + +describe('the block: shrink-only, and it shrank by exactly this package', () => { + it(`DEBT no longer lists \`${BOUND}\``, () => { + expect(ledgerBlock('DEBT')).not.toContain(`"${BOUND}"`); + }); + + it('the whole `@object-ui/components` group is gone', () => { + expect(ledgerBlock('DEBT')).not.toContain('"@object-ui/components"'); + }); + + it('DEBT has not grown -- 2 names is the ceiling this slice left', () => { + // The `@object-ui/types` slice left 3. This one took one of them. + // Any future measurement above this number is the ratchet failing, whatever + // reason is given for it. + expect(ledgerNames(ledgerBlock('DEBT')).length).toBeLessThanOrEqual(2); + }); + + it('CLAIM_DEBT did not grow either -- this slice removed no rule 2 claim', () => { + // The `@object-ui/core` slice was FORCED to regenerate that block too, + // because the const it deleted carried a `Mirrors …` comment. The + // declaration this slice deleted carried no spec-alignment claim, so + // `--claim-ledger` regenerates byte-identically. If a later edit here moves + // this number, that coupling is back. + expect(ledgerNames(ledgerBlock('CLAIM_DEBT')).length).toBeLessThanOrEqual(18); + }); + + it('ALLOW did NOT gain a waiver -- the route was BIND, and that is a fact worth pinning', () => { + // The sibling slice put one of its two names in ALLOW instead, so "a name + // left DEBT" does not say which route was taken. This one took none: there + // is nothing here that deliberately differs from the spec's direction + // vocabulary. A later hand that waives it instead has to delete this + // assertion, which is the visibility a silent re-fork would not have. + expect(fs.readFileSync(gateSource, 'utf8')).not.toContain(`"@object-ui/components:${BOUND}"`); + }); +}); diff --git a/scripts/check-spec-symbol-derivation.mjs b/scripts/check-spec-symbol-derivation.mjs index 18975c7827..6e76953280 100644 --- a/scripts/check-spec-symbol-derivation.mjs +++ b/scripts/check-spec-symbol-derivation.mjs @@ -924,10 +924,31 @@ const DEBT_ISSUE = 7265; // default }` preset dialect that the spec's strict ViewTabSchema rejects, and // binding it would 422 metadata that renders today. Both are pinned, in both // directions, by packages/types/src/__tests__/spec-symbol-parity.test.ts. +// +// Then the `@object-ui/components` slice at objectui#7265, where the route was +// decided by what the extra member MEANS rather than by how many members there +// were. The DataTable renderer declared `SortDirection` as `'asc' | 'desc' | +// null`, and that third state made the RENAME question genuinely live: the +// spec's `SortDirection` -- re-measured at the RESOLVED pin 17.4.0, on the +// `@objectstack/spec/shared` subpath, because byte-identical is a statement +// about a version and so is two-member -- is the direction and nothing else. +// Reading the site settled it the other way. `null` is not a third DIRECTION, +// it is the ABSENCE of one: it is written at exactly one place (the end of the +// client-side header cycle in `handleSort`) and read only by guards that guard +// `sortColumn` in the same breath -- and `sortColumn`, the other half of the +// same state pair, already spelled its own empty case as `useState` at the slot instead of folding it into a type name. So the concept IS +// the spec's, and the strongest BIND form was available: the type is now +// IMPORTED outright, the local declaration is gone rather than derived, and the +// third state is confined to the one state slot that carries it with the reason +// written there. `applySort`, one screen down, was a second hand copy of the +// same two members inline in its signature and now takes the bound type too. +// The spec-side properties the binding rests on are pinned in this package's +// existing spec-symbol file, packages/components/src/__tests__/ +// share-filter-sort-spec-parity.test.ts (appended to, not duplicated); the site +// and the block are pinned in +// scripts/__tests__/spec-symbol-ledger-components-7265.test.ts. const DEBT = { - "@object-ui/components": [ - "SortDirection", - ], "@object-ui/data-objectstack": [ "normalizeFilterOperator", ],