From ae1f386d87407586247cd2966c5d0f465eca6d0a Mon Sep 17 00:00:00 2001 From: Brian Love Date: Sun, 16 Aug 2026 20:26:16 -0700 Subject: [PATCH] fix(row-model): ColumnValueOf resolves to unknown, not never MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ColumnValueOf` fell back to `never` for any column that declares no `accessor`. `never` is assignable to everything, so every runtime guard written against such a value compiled green while checking nothing: if (columnId === "quantity" && typeof value === "number" && value < 0) That is the guard shipped in the editing page's worked example, and it could never have been wrong. No example in the docs corpus uses accessor columns, so `value` was `never` in every rows-mode `onRowChange` the docs teach. `unknown` is the honest fallback: it forces the guard instead of accepting it. `never` is still load-bearing INSIDE the distribution — it is the union identity, so non-matching column members must vanish rather than widen every answer for a mixed tuple. The fallback is applied outside the distribution via `[X] extends [infer TResolved]`; a naked `extends infer` would distribute, and distributing over `never` short-circuits the whole conditional to `never`, which is precisely the case the fallback exists to catch. Both behaviors are pinned in column-value-of.test.ts. Fallout: - apps/bench passed `columnId as never` to `distinctValues`, which forced the id to match no column. `BenchColumn` declares an accessor, so plain `columnId` resolves correctly; the cast was papering over this hole. - grid/editing.mdx's `value` row claimed "inferred from `columnId`", true only for accessored columns. Replaced with an accurate note. - The example passed ``, which bought nothing (TRow is inferred from `rows`) and read as broken JSX. - The docs-guard roster excused that table on the grounds that it documents `PretableSurfaceProps.onCellEdit` — a prop that exists nowhere in the repo. That stale excuse is why the false claim survived. Replaced with the real limitation: the member reader handles interfaces only and cannot bind a mapped-type-indexed union. Co-Authored-By: Claude Opus 5 --- apps/bench/src/row-model-diagnostics.ts | 4 +- apps/website/content/docs/grid/editing.mdx | 12 +++- .../async-cell-editing/AsyncEditingGrid.tsx | 2 +- .../docs/__tests__/docs-api-surface.test.ts | 2 +- packages/core/core.api.md | 8 ++- packages/react/react.api.md | 8 ++- .../src/__tests__/column-value-of.test.ts | 67 +++++++++++++++++++ packages/row-model/src/column-types.ts | 49 +++++++++++--- 8 files changed, 130 insertions(+), 22 deletions(-) create mode 100644 packages/row-model/src/__tests__/column-value-of.test.ts diff --git a/apps/bench/src/row-model-diagnostics.ts b/apps/bench/src/row-model-diagnostics.ts index 7bf6fb07..38ae9187 100644 --- a/apps/bench/src/row-model-diagnostics.ts +++ b/apps/bench/src/row-model-diagnostics.ts @@ -331,7 +331,7 @@ export function createRowModelDiagnosticsController( activeTransition?.cancel(); }, startDistinctDictionary(columnId) { - activeDistinct = rawModel.distinctValues(columnId as never, { + activeDistinct = rawModel.distinctValues(columnId, { limit: 32, }); void activeDistinct.finished.catch(() => undefined); @@ -382,7 +382,7 @@ export function createRowModelDiagnosticsController( async churnRetentionLimits() { controller.churnRevisions(journalCapacity + 2); for (const { id: columnId } of columns.slice(0, distinctCapacity + 1)) { - await rawModel.distinctValues(columnId as never, { limit: 1 }).finished; + await rawModel.distinctValues(columnId, { limit: 1 }).finished; } }, createRunSummary() { diff --git a/apps/website/content/docs/grid/editing.mdx b/apps/website/content/docs/grid/editing.mdx index fde87930..a457cd61 100644 --- a/apps/website/content/docs/grid/editing.mdx +++ b/apps/website/content/docs/grid/editing.mdx @@ -43,11 +43,21 @@ The proposal preserves the row, ID, column, and value correlations inferred from | ------------- | -------------------------------------------------------------------- | | `rowId` | stable ID of the edited row | | `columnId` | exact ID of the edited column | -| `value` | committed value, inferred from `columnId` | +| `value` | committed value — see the note below on how precisely it's typed | | `previousRow` | immutable row captured when editing began | | `row` | complete proposed row | | `changes` | partial row patch produced by the column's direct or computed setter | +How precisely `value` is typed depends on how you declare your columns. A column that declares an `accessor` carries its exact value type through, so `value` is correlated to `columnId` — narrow on `columnId` and `value` narrows with it. A column without one — the plain `PretableColumn` shape used throughout these docs — has no static value type to carry, so `value` is `unknown` and you check it yourself: + +```tsx +onRowChange={({ columnId, value }) => { + if (columnId === "quantity" && typeof value === "number" && value < 0) { + throw new Error("Quantity can't go negative"); + } +}} +``` + Explicit-model mode instead accepts `beforeRowChange={(changes) => ...}`. It may reject asynchronously; if it resolves, all proposals publish atomically through the supplied model. Rows mode does not accept `beforeRowChange`, and model mode does not accept `onRowChange`. ## Making a column editable diff --git a/apps/website/content/examples/async-cell-editing/AsyncEditingGrid.tsx b/apps/website/content/examples/async-cell-editing/AsyncEditingGrid.tsx index 5fc3623e..038c3b49 100644 --- a/apps/website/content/examples/async-cell-editing/AsyncEditingGrid.tsx +++ b/apps/website/content/examples/async-cell-editing/AsyncEditingGrid.tsx @@ -30,7 +30,7 @@ export function AsyncEditingGrid() { onRowChange rejects it — an inline error appears, the editor stays open, and Enter retries.

- + row.id} diff --git a/apps/website/lib/docs/__tests__/docs-api-surface.test.ts b/apps/website/lib/docs/__tests__/docs-api-surface.test.ts index 8f34c37d..91aee162 100644 --- a/apps/website/lib/docs/__tests__/docs-api-surface.test.ts +++ b/apps/website/lib/docs/__tests__/docs-api-surface.test.ts @@ -1420,7 +1420,7 @@ const TABLES: Record = { }, "grid/editing.mdx#The controlled model": { unbound: - "Documents the anonymous payload object of `PretableSurfaceProps.onCellEdit`, which is declared inline and has no exported name.", + 'Documents `PretableRowChange`, which IS exported — but as a mapped-type-indexed union (`{ [K in ColumnIdOf]: {…} }[ColumnIdOf]`), and the member reader here handles interfaces and inline object members only, so binding it fails with "is not an interface". This excuse is therefore about the READER, not the type: teach `resolveRefMembers` to read the inner object literal out of that shape and this table can and should bind. Until then it is unchecked, and it has already drifted once — the `value` row claimed the committed value was "inferred from `columnId`" while `ColumnValueOf` resolved to `never` for every accessor-less column, which is every column the docs corpus teaches.', }, "headless/state-model.mdx#Row-model state": { unbound: diff --git a/packages/core/core.api.md b/packages/core/core.api.md index c1666116..d47d4498 100644 --- a/packages/core/core.api.md +++ b/packages/core/core.api.md @@ -56,11 +56,13 @@ export type ColumnsOf = TModel extends { // @public (undocumented) export type ColumnType = "text" | "number" | "date" | "enum" | "boolean"; -// @public (undocumented) -export type ColumnValueOf> = TColumns extends readonly (infer TColumn)[] ? TColumn extends { +// @public +export type ColumnValueOf> = [ +TColumns extends readonly (infer TColumn)[] ? TColumn extends { readonly id: TColumnId; readonly accessor: (...args: never[]) => infer TValue; -} ? TValue : never : never; +} ? TValue : never : never +] extends [infer TResolved] ? [TResolved] extends [never] ? unknown : TResolved : never; // @public export function createColumnHelper(): PretableColumnHelper; diff --git a/packages/react/react.api.md b/packages/react/react.api.md index 3a40d630..e0efeb0c 100644 --- a/packages/react/react.api.md +++ b/packages/react/react.api.md @@ -68,11 +68,13 @@ export type ColumnsOf = TModel extends { // @public (undocumented) export type ColumnType = "text" | "number" | "date" | "enum" | "boolean"; -// @public (undocumented) -export type ColumnValueOf> = TColumns extends readonly (infer TColumn)[] ? TColumn extends { +// @public +export type ColumnValueOf> = [ +TColumns extends readonly (infer TColumn)[] ? TColumn extends { readonly id: TColumnId; readonly accessor: (...args: never[]) => infer TValue; -} ? TValue : never : never; +} ? TValue : never : never +] extends [infer TResolved] ? [TResolved] extends [never] ? unknown : TResolved : never; // @public export interface CopyPayload { diff --git a/packages/row-model/src/__tests__/column-value-of.test.ts b/packages/row-model/src/__tests__/column-value-of.test.ts new file mode 100644 index 00000000..f5906064 --- /dev/null +++ b/packages/row-model/src/__tests__/column-value-of.test.ts @@ -0,0 +1,67 @@ +/* eslint-disable @typescript-eslint/no-unused-vars -- the `_`-prefixed aliases + below ARE the assertions: each is a compile error if its `Expect<...>` + constraint fails, and nothing needs to read them at runtime. */ +import { expectTypeOf, test } from "vitest"; + +import type { ColumnValueOf } from "../index"; + +type Equal = + (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; +type Expect = T; + +interface StockItem { + id: string; + item: string; + quantity: number; +} + +/** + * The typed path: a column that declares an `accessor` keeps its exact value + * type. This is the guarantee everything else here must not cost. + */ +type Accessored = readonly [ + { readonly id: "item"; readonly accessor: (row: StockItem) => string }, + { readonly id: "quantity"; readonly accessor: (row: StockItem) => number }, +]; +type _AccessoredIsExact = Expect< + Equal, number> +>; + +/** + * A tuple where only SOME columns declare an accessor. The accessor-less + * members must not widen the accessored ones — `ColumnValueOf` distributes + * over the column union, so a per-member fallback would union `unknown` into + * every answer and destroy the precision above. + */ +type Mixed = readonly [ + { readonly id: "item"; readonly accessor: (row: StockItem) => string }, + { readonly id: "quantity" }, +]; +type _MixedKeepsAccessored = Expect< + Equal, string> +>; +type _MixedFallsBackForTheRest = Expect< + Equal, unknown> +>; + +/** + * The loose, id-keyed column shape the docs corpus actually teaches + * (`PretableColumn[]`): no accessor, `id: string`. This used to resolve + * to `never`, which is assignable to everything — so every runtime guard + * written against such a value (`typeof value === "number"`) compiled while + * being type-level nonsense. `unknown` is the honest answer: it forces the + * guard instead of silently accepting it. + */ +type Loose = readonly { readonly id: string }[]; +type _LooseIsUnknown = Expect, unknown>>; + +test("an accessor-less column narrows under a typeof guard", () => { + const value = 3 as ColumnValueOf; + // `never` would make this branch vacuous; `unknown` makes it real. + expectTypeOf(value).toBeUnknown(); + if (typeof value === "number") { + expectTypeOf(value).toBeNumber(); + } +}); diff --git a/packages/row-model/src/column-types.ts b/packages/row-model/src/column-types.ts index a050ee25..d604ca34 100644 --- a/packages/row-model/src/column-types.ts +++ b/packages/row-model/src/column-types.ts @@ -361,17 +361,44 @@ export type ColumnIdOf = TColumns extends readonly (infer TColumn)[] : never : never; -/** @public */ -export type ColumnValueOf< - TColumns, - TColumnId extends ColumnIdOf, -> = TColumns extends readonly (infer TColumn)[] - ? TColumn extends { - readonly id: TColumnId; - readonly accessor: (...args: never[]) => infer TValue; - } - ? TValue - : never +/** + * The value type of the column with the given id. + * + * An accessored column resolves to its exact declared type. A column that + * declares no accessor — the loose, id-keyed shape — resolves to `unknown`, + * not `never`: the value genuinely isn't known statically, and `never` is + * assignable to everything, so it silently accepted (and made vacuous) every + * runtime guard written against it. `unknown` forces the guard instead. + * + * Two pieces of the shape below are load-bearing: + * + * - The inner `never` stays `never`. The lookup distributes over the column + * union, so for a mixed tuple every non-matching member contributes to the + * result union; `never` is the union identity, so those members vanish and + * the matching member's type survives exactly. Falling back to `unknown` + * per member would union `unknown` into every answer and destroy that. + * - The fallback is applied through `[...] extends [infer TResolved]`, which + * binds the resolved type once (no repetition, no extra exported alias) + * while the tuple wrapper blocks distribution. A naked `extends infer` would + * distribute, and distributing over `never` short-circuits the whole + * conditional to `never` — which is precisely the case this fallback exists + * to catch. + * + * @public + */ +export type ColumnValueOf> = [ + TColumns extends readonly (infer TColumn)[] + ? TColumn extends { + readonly id: TColumnId; + readonly accessor: (...args: never[]) => infer TValue; + } + ? TValue + : never + : never, +] extends [infer TResolved] + ? [TResolved] extends [never] + ? unknown + : TResolved : never; /** @public */