From e552f88262d7936f7bf6ef6a1a3a92775b0edd38 Mon Sep 17 00:00:00 2001 From: yinlianghui Date: Tue, 11 Aug 2026 11:56:43 +0000 Subject: [PATCH] feat(plugin-grid): declare the host-driven external-pagination contract and type-check its tests (#4277, #4040) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ObjectGridProps declared 12 members while ObjectGrid read 12 more out of `...rest`, each through an `as any` cast — the framework#2212 host-driven external-pagination path, deliberate (the `:418` comment says so) and declared nowhere. Per the #4277 裁决 (option B + derivation pin): - `ObjectGridExternalPaginationProps` names the mode; `ObjectGridProps` extends it. The 11 members with a `DataTableSchema` counterpart — the type ObjectGrid forwards them to — are TYPE-DERIVED (`Partial< Pick< … > >`), not a second hand-written enumeration. Only `onColumnStateChange` is explicit, with its reason: the table vocabulary has per-event `onColumnResize` / `onColumnReorder` and no merged-layout callback. - The 12 `(rest as any)` reads become typed destructuring; `...rest` is gone. - `tsconfig.test.json` per the #3032 template, chained from `type-check`. - The two mechanical errors #4277 recorded: `importMissingRequiredHint` was omitting the required `dataSource`, and the `runAggregate` stubs declared no parameters while the cases read the second one through casts. - plugin-grid leaves TEST_DEBT (declared 2, measured 4, now 0). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- ...jectgrid-external-pagination-props-4277.md | 13 ++ packages/plugin-grid/package.json | 2 +- packages/plugin-grid/src/ObjectGrid.tsx | 136 ++++++++++++++---- .../importMissingRequiredHint.test.tsx | 17 ++- .../src/__tests__/useBulkExecutor.test.ts | 33 +++-- packages/plugin-grid/src/index.tsx | 2 +- packages/plugin-grid/tsconfig.test.json | 48 +++++++ scripts/check-type-check-coverage.mjs | 1 - 8 files changed, 214 insertions(+), 38 deletions(-) create mode 100644 .changeset/objectgrid-external-pagination-props-4277.md create mode 100644 packages/plugin-grid/tsconfig.test.json diff --git a/.changeset/objectgrid-external-pagination-props-4277.md b/.changeset/objectgrid-external-pagination-props-4277.md new file mode 100644 index 0000000000..6b8a29d3cb --- /dev/null +++ b/.changeset/objectgrid-external-pagination-props-4277.md @@ -0,0 +1,13 @@ +--- +'@object-ui/plugin-grid': minor +--- + +ObjectGrid's host-driven pagination mode is a declared interface instead of twelve `(rest as any)` reads + +`ObjectGridProps` declared twelve members while the component read twelve more out of `...rest`, each through an `as any` cast: `data`, `manualPagination`, `rowCount`, `page`, `pageSize`, `onPageChange`, `onPageSizeChange`, `sort`, `onSortChange`, `search`, `onSearchChange` and `onColumnStateChange`. They are not accidental — together they are the host-driven external-pagination path from framework#2212, where a host has already fetched one window of a larger collection and drives the page/sort/search controls itself, and the component's own comment said so. They were simply declared nowhere, so no call site could be checked against them and no editor could offer them. + +Nothing had caught it because the only untyped caller is `ObjectGridRenderer`, whose `{ schema: any; [key: string]: any }` index signature accepts anything; every typed caller happens to pass only declared props; and the test that exercises the path was compiled by nothing. + +They now live on a named `ObjectGridExternalPaginationProps`, which `ObjectGridProps` extends — a separate interface rather than twelve more members flattened into the authoring surface, so the "advanced host-driven mode" boundary stays visible. The eleven members that already have a counterpart on `DataTableSchema` — the type ObjectGrid forwards them to — are **type-derived** from that declaration (`Partial< Pick< DataTableSchema, … > >`) rather than hand-copied, so the two cannot drift apart; only `onColumnStateChange` is declared explicitly, because the table vocabulary reports per-event `onColumnResize` / `onColumnReorder` rather than the merged `{ order, widths }` layout this reports. `ObjectGridColumnState` is exported for that payload. + +Purely additive for callers: every member is optional, so existing code compiles unchanged, and hosts that were already passing these props now get them checked instead of silently accepted. Runtime behavior is unchanged. diff --git a/packages/plugin-grid/package.json b/packages/plugin-grid/package.json index 2aa5bdee67..ac7e318823 100644 --- a/packages/plugin-grid/package.json +++ b/packages/plugin-grid/package.json @@ -17,7 +17,7 @@ "scripts": { "build": "vite build", "test": "vitest run", - "type-check": "tsc --noEmit && tsc -p tsconfig.typetests.json", + "type-check": "tsc --noEmit && tsc -p tsconfig.typetests.json && tsc -p tsconfig.test.json", "lint": "eslint ." }, "dependencies": { diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 7ee41c9d48..4c0b1776cd 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -22,7 +22,7 @@ */ import React, { useEffect, useState, useCallback, useMemo } from 'react'; -import type { ObjectGridSchema, DataSource, ListColumn, ViewData, TableSortItem } from '@object-ui/types'; +import type { ObjectGridSchema, DataSource, ListColumn, ViewData, TableSortItem, DataTableSchema } from '@object-ui/types'; import { isSystemManagedField } from '@object-ui/types'; import type { I18nLabel } from '@objectstack/spec/ui'; import { SchemaRenderer, useDataScope, useNavigationOverlay, useAction, useSafeFieldLabel, usePredicateScope } from '@object-ui/react'; @@ -157,7 +157,78 @@ function resolveColumnLabel(label: string | I18nLabel | undefined): string | und return typeof label === 'string' ? label : undefined; } -export interface ObjectGridProps { +/** + * The column layout ObjectGrid persists and reports back — the merged result of + * a resize and a reorder, not either event on its own. Named so the state hook + * below and the `onColumnStateChange` prop cannot drift apart. + */ +export interface ObjectGridColumnState { + order?: string[]; + widths?: Record; +} + +/** + * The HOST-DRIVEN ("external") mode of ObjectGrid — framework#2212. + * + * The ordinary authoring surface hands ObjectGrid a `schema` and a `dataSource` + * and lets it fetch, page, sort and search for itself. In this mode a host + * (ListView, a designer preview, an app screen with its own toolbar) has + * already fetched one window of a larger collection and drives the controls + * itself: it passes the window as `data` plus the real match total and the + * page/sort/search state, and ObjectGrid forwards them straight to its + * DataTable instead of client-slicing the window it was handed. + * + * Kept as its own named interface rather than flattened into `ObjectGridProps` + * (#4277 裁决 B, 2026-08-11): the two are different classes of contract, and a + * dozen more members merged into the authoring surface would erase that + * boundary. Until this existed, every member below was read out of `...rest` + * through an `as any` cast and was declared nowhere at all. + * + * DERIVATION (#4277 裁决 §3, the anti-drift pin): this vocabulary is already + * declared once, on `DataTableSchema` — which is exactly where ObjectGrid + * forwards it — so the members that have a counterpart there are TYPE-DERIVED + * from that declaration rather than hand-copied into a second enumeration. Two + * hand-written copies of one vocabulary is how the next drift happens. The + * `Partial<...>` wrapper is deliberate and is the only shape change: the whole + * mode is opt-in, and `DataTableSchema['data']` is required because a table + * always has rows, while a grid that was given no `data` fetches its own. + */ +export interface ObjectGridExternalPaginationProps + extends Partial< + Pick< + DataTableSchema, + // The host's already-fetched window. Highest-priority data source: it + // wins over `schema.data` / `schema.bind` when present. + | 'data' + // Turns off client slicing. With `rowCount` + `onPageChange` it is what + // makes the mode active at all (see `externalManualPagination` below). + | 'manualPagination' + | 'rowCount' + | 'page' + | 'pageSize' + | 'onPageChange' + | 'onPageSizeChange' + | 'sort' + | 'onSortChange' + | 'search' + | 'onSearchChange' + > + > { + /** + * Grid-only: `DataTableSchema` has no counterpart to derive from. + * + * The table vocabulary reports column changes as separate per-event + * callbacks — `onColumnResize(columnKey, width)` and + * `onColumnReorder(newOrder)` — whereas this reports the MERGED, persisted + * `{ order, widths }` layout after ObjectGrid has folded either event into + * the state it also writes to `localStorage`, so a host can save one blob + * through `dataSource.updateViewConfig`. Deriving it from either table + * callback would misstate both the payload and when it fires. + */ + onColumnStateChange?: (state: ObjectGridColumnState) => void; +} + +export interface ObjectGridProps extends ObjectGridExternalPaginationProps { schema: ObjectGridSchema; dataSource?: DataSource; className?: string; @@ -266,7 +337,23 @@ export const ObjectGrid: React.FC = ({ onRowSave, onBatchSave, onAddRecord, - ...rest + // The host-driven mode (`ObjectGridExternalPaginationProps`). Every one of + // these was read out of `...rest` through an `as any` cast until #4277 gave + // them a declaration; they are ordinary typed props now, and `rest` is gone + // with them. Renamed on the way in only where the component already owns the + // plain name (`data` is the fetched rows, `pageSize` the schema's). + data: passedData, + manualPagination: hostManualPagination, + rowCount: hostRowCount, + page: hostPage, + pageSize: hostPageSize, + onPageChange: hostOnPageChange, + onPageSizeChange: hostOnPageSizeChange, + sort: hostSort, + onSortChange: hostOnSortChange, + search: hostSearch, + onSearchChange: hostOnSearchChange, + onColumnStateChange, }) => { const [data, setData] = useState([]); const [loading, setLoading] = useState(true); @@ -317,10 +404,7 @@ export const ObjectGrid: React.FC = ({ : `grid-columns-${schema.objectName}`; }, [schema.objectName, schema.id]); - const [columnState, setColumnState] = useState<{ - order?: string[]; - widths?: Record; - }>(() => { + const [columnState, setColumnState] = useState(() => { // Priority: 1) externally provided (e.g. persisted view override), // 2) localStorage (per-browser fallback), 3) empty. const fromProps = (schema as any).columnState; @@ -356,11 +440,10 @@ export const ObjectGrid: React.FC = ({ console.warn('Failed to persist column state:', e); } // Notify parent so it can persist via dataSource.updateViewConfig. - const onChange = (rest as any).onColumnStateChange; - if (typeof onChange === 'function') { - try { onChange(state); } catch (e) { console.warn('onColumnStateChange threw:', e); } + if (typeof onColumnStateChange === 'function') { + try { onColumnStateChange(state); } catch (e) { console.warn('onColumnStateChange threw:', e); } } - }, [columnStorageKey, rest]); + }, [columnStorageKey, onColumnStateChange]); const handlePullRefresh = useCallback(async () => { setRefreshKey(k => k + 1); @@ -380,8 +463,8 @@ export const ObjectGrid: React.FC = ({ return () => window.removeEventListener('resize', checkWidth); }, []); - // Check if data is passed directly (from ListView) - const passedData = (rest as any).data; + // `passedData` — data handed down directly (from ListView) — is destructured + // from props above. // Resolve bound data if 'bind' property exists const boundData = useDataScope(schema.bind); @@ -415,11 +498,12 @@ export const ObjectGrid: React.FC = ({ // real match total + page controls. We must forward those straight to DataTable // instead of client-slicing the window — otherwise the footer would report // "pages = window / pageSize" and records beyond the window stay unreachable - // (framework #2212). `data` arrives via `rest` (a prop), so do these too. + // (framework #2212). `data` is a prop, and so are these — all declared on + // `ObjectGridExternalPaginationProps` since #4277. const externalManualPagination = - (rest as any).manualPagination === true && - typeof (rest as any).rowCount === 'number' && - typeof (rest as any).onPageChange === 'function'; + hostManualPagination === true && + typeof hostRowCount === 'number' && + typeof hostOnPageChange === 'function'; // Extract stable primitive/reference-stable values from schema for dependency arrays. // This prevents infinite re-render loops when schema is a new object on each render @@ -2159,16 +2243,16 @@ export const ObjectGrid: React.FC = ({ ? schema.searchableFields.length > 0 : (schema.showSearch !== undefined ? schema.showSearch : true); - const manualRowCount = externalManualPagination ? (rest as any).rowCount : totalMatching; - const manualPage = externalManualPagination ? (rest as any).page : serverPage; + const manualRowCount = externalManualPagination ? hostRowCount : totalMatching; + const manualPage = externalManualPagination ? hostPage : serverPage; const manualPageSize = externalManualPagination - ? ((rest as any).pageSize ?? serverPageSize) + ? (hostPageSize ?? serverPageSize) : serverPageSize; const manualOnPageChange = externalManualPagination - ? (rest as any).onPageChange + ? hostOnPageChange : setServerPage; const manualOnPageSizeChange = externalManualPagination - ? (rest as any).onPageSizeChange + ? hostOnPageSizeChange : (size: number) => { setServerPageSize(size); setServerPage(1); }; // Before anyone clicks, the headers show the sort the view was authored with @@ -2185,10 +2269,10 @@ export const ObjectGrid: React.FC = ({ schemaSort ?? (schema.defaultSort ? [schema.defaultSort] : undefined), ); const manualSort: TableSortItem[] = externalManualPagination - ? ((rest as any).sort ?? []) + ? (hostSort ?? []) : (headerSort ?? declaredSort); const manualOnSortChange = externalManualPagination - ? (rest as any).onSortChange + ? hostOnSortChange : setHeaderSort; // The search term, in whichever server mode applies. When a parent owns the @@ -2198,10 +2282,10 @@ export const ObjectGrid: React.FC = ({ // turns it into `$search`. A parent that drives the rows but offers no // `onSearchChange` gets NO box rather than one scoped to its window. const manualSearch = externalManualPagination - ? ((rest as any).search ?? '') + ? (hostSearch ?? '') : searchTerm; const manualOnSearchChange = externalManualPagination - ? (rest as any).onSearchChange + ? hostOnSearchChange : setSearchTerm; const dataTableSchema: any = { diff --git a/packages/plugin-grid/src/__tests__/importMissingRequiredHint.test.tsx b/packages/plugin-grid/src/__tests__/importMissingRequiredHint.test.tsx index e33c442c15..2ccf36f75a 100644 --- a/packages/plugin-grid/src/__tests__/importMissingRequiredHint.test.tsx +++ b/packages/plugin-grid/src/__tests__/importMissingRequiredHint.test.tsx @@ -30,9 +30,24 @@ function pasteRows(text: string) { act(() => { window.dispatchEvent(evt); }); } +// A data source offering none of the optional import capabilities. These cases +// never leave the mapping step, so nothing on it is ever called — but the wizard +// probes it for `listImportMappings` / `importRecords` / `listImportJobs` on +// render, and an empty object is the honest answer to all three: "a source that +// supports none of them". `ImportWizardProps.dataSource` is required, so it has +// to be passed rather than omitted (the omission was invisible until #4040 let +// `tsc` read this file). +const NO_CAPABILITY_DATA_SOURCE = {}; + function renderWizard() { render( - {}} />, + {}} + />, ); } diff --git a/packages/plugin-grid/src/__tests__/useBulkExecutor.test.ts b/packages/plugin-grid/src/__tests__/useBulkExecutor.test.ts index f69be2a809..1cfe476041 100644 --- a/packages/plugin-grid/src/__tests__/useBulkExecutor.test.ts +++ b/packages/plugin-grid/src/__tests__/useBulkExecutor.test.ts @@ -354,8 +354,27 @@ describe('useBulkExecutor', () => { } as BulkActionDef); const ds = () => ({ update: vi.fn(), delete: vi.fn() }); + // A stub that DECLARES the parameters the hook really passes. + // `BulkExecutorOptions.runAggregate` is `(def, rows, params)` and the hook + // dispatches it with all three (`hooks/useBulkExecutor.ts`), but a bare + // `vi.fn(async () => undefined)` declares none of them: vitest records the + // real arguments at runtime, so `mock.calls[0][1]` works while the compiler + // is told the call tuple has length 0. The cases below read that second + // argument and papered over the contradiction with casts — a types-only lie + // about the exact signature they exist to pin (#4277). Typing it here once + // makes the reads compile on their own and keeps every aggregate case + // agreeing about the dispatcher's shape. + const aggregateStub = () => + vi.fn( + async ( + _def: BulkActionDef, + _rows: Array>, + _params: Record, + ): Promise => undefined, + ); + it('dispatches runAggregate exactly once with every row and the params', async () => { - const runAggregate = vi.fn(async () => undefined); + const runAggregate = aggregateStub(); const runAction = vi.fn(async () => undefined); const rows = [{ id: 'r1' }, { id: 'r2' }, { id: 'r3' }]; const { result } = renderHook(() => @@ -366,9 +385,7 @@ describe('useBulkExecutor', () => { }); expect(runAggregate).toHaveBeenCalledTimes(1); - const [defArg, rowsArg, paramsArg] = runAggregate.mock.calls[0] as unknown as [ - BulkActionDef, Array>, Record, - ]; + const [defArg, rowsArg, paramsArg] = runAggregate.mock.calls[0]; expect(defArg.name).toBe('generate_qr_zip'); expect(rowsArg.map(r => r.id)).toEqual(['r1', 'r2', 'r3']); expect(paramsArg).toEqual({ format: 'png' }); @@ -378,7 +395,7 @@ describe('useBulkExecutor', () => { }); it('a single-row selection still goes through the ONE aggregate call, never per-record', async () => { - const runAggregate = vi.fn(async () => undefined); + const runAggregate = aggregateStub(); const runAction = vi.fn(async () => undefined); const { result } = renderHook(() => useBulkExecutor({ resource: 'device', dataSource: ds(), runAction, runAggregate })); @@ -393,7 +410,7 @@ describe('useBulkExecutor', () => { }); it('ignores batchSize — 5 rows with batchSize 2 is still one call', async () => { - const runAggregate = vi.fn(async () => undefined); + const runAggregate = aggregateStub(); const rows = [{ id: '1' }, { id: '2' }, { id: '3' }, { id: '4' }, { id: '5' }]; const { result } = renderHook(() => useBulkExecutor({ resource: 'device', dataSource: ds(), runAggregate })); @@ -403,7 +420,7 @@ describe('useBulkExecutor', () => { }); expect(runAggregate).toHaveBeenCalledTimes(1); - expect((runAggregate.mock.calls[0][1] as unknown[]).length).toBe(5); + expect(runAggregate.mock.calls[0][1].length).toBe(5); expect(result.current.result?.succeeded).toBe(5); }); @@ -462,7 +479,7 @@ describe('useBulkExecutor', () => { }); it('a def WITHOUT execution: aggregate keeps per-record dispatch even when runAggregate is wired', async () => { - const runAggregate = vi.fn(async () => undefined); + const runAggregate = aggregateStub(); const runAction = vi.fn(async () => undefined); const rows = [{ id: '1' }, { id: '2' }]; const { result } = renderHook(() => diff --git a/packages/plugin-grid/src/index.tsx b/packages/plugin-grid/src/index.tsx index 2c53900327..5904c0b34d 100644 --- a/packages/plugin-grid/src/index.tsx +++ b/packages/plugin-grid/src/index.tsx @@ -35,7 +35,7 @@ export { useGroupReorder } from './useGroupReorder'; export { useColumnSummary } from './useColumnSummary'; export { FormulaBar } from './FormulaBar'; export { SplitPaneGrid } from './SplitPaneGrid'; -export type { ObjectGridProps } from './ObjectGrid'; +export type { ObjectGridProps, ObjectGridExternalPaginationProps, ObjectGridColumnState } from './ObjectGrid'; export type { VirtualGridProps, VirtualGridColumn } from './VirtualGrid'; export type { InlineEditingProps } from './InlineEditing'; export type { ImportWizardProps, ImportResult } from './ImportWizard'; diff --git a/packages/plugin-grid/tsconfig.test.json b/packages/plugin-grid/tsconfig.test.json new file mode 100644 index 0000000000..148f4c667c --- /dev/null +++ b/packages/plugin-grid/tsconfig.test.json @@ -0,0 +1,48 @@ +{ + // Type-checks this package's TESTS, which `tsconfig.json` excludes. + // See `packages/types/tsconfig.test.json` for why that exclusion was a hole: + // the build correctly keeps tests out of `dist`, but nothing else compiled + // them, so a test could assert a contract the compiler never checked. Here it + // hid a whole undeclared prop contract: `serverPagination.test.tsx` passed + // `data` / `manualPagination` / `rowCount` / `page` / ... to `ObjectGrid`, + // none of which `ObjectGridProps` declared (#4277). + // + // `tsconfig.typetests.json` next door is the narrow rescue this supersedes for + // this package: it compiles `spec-symbol-batch7.test.ts` alone, because that + // one file's whole value is compile-time assertions and it could not wait for + // the rest of the tree to compile (objectui#3181). It stays chained — the file + // is now read by both projects, which costs one extra compile and keeps the + // narrow project's reasoning where it was written. + "extends": "../../tsconfig.json", + "compilerOptions": { + "noEmit": true, + // The package build emits `dist`; this project emits nothing, so it must + // not inherit `composite` / `declaration` from the build config. + "composite": false, + "jsx": "react-jsx", + // One notch above the root `lib` (ES2020), and only here: the tests use + // `Array.prototype.at` (ES2022) — `column-features.test.tsx`, + // `columnIdentity.test.tsx` — which shipped SOURCE must not, so raising it + // in `tsconfig.json` would let the build compile calls the browser targets + // do not have. + "lib": ["ES2022", "DOM"], + // Two global augmentations, neither of them an import: + // - `node` for the import/export suites, which reach for `Buffer` and + // Node globals while exercising the ExcelJS/CSV seams. + // - `@testing-library/jest-dom` for the `toBeInTheDocument` / + // `toHaveTextContent` matchers used across the `.tsx` suites; it does + // not live under `@types/`, so it is never picked up automatically. + // Naming `types` at all switches off automatic `@types/*` inclusion, so + // both have to be listed here. + "types": ["node", "@testing-library/jest-dom"], + // Drop the root tsconfig's source-tree `paths` so `@object-ui/*` and + // `@objectstack/spec` resolve through the workspace dependency's built + // `.d.ts` instead of pulling sibling sources in as program inputs (TS6059). + "paths": {} + }, + // `src/**/*.d.ts` is pulled in explicitly: the build program gets ambient + // declarations for free from `"include": ["src"]`, but an ambient declaration + // file is only a program input when a pattern NAMES it — being imported is + // not enough, because nothing imports it (plugin-map's lesson, #4270). + "include": ["src/**/*.test.ts", "src/**/*.test.tsx", "src/**/*.d.ts"] +} diff --git a/scripts/check-type-check-coverage.mjs b/scripts/check-type-check-coverage.mjs index c09fd1908a..2ec3d791b9 100644 --- a/scripts/check-type-check-coverage.mjs +++ b/scripts/check-type-check-coverage.mjs @@ -127,7 +127,6 @@ export const TEST_DEBT = { "@object-ui/plugin-detail": { errors: 5, issue: 4118, note: "TS2353x3 — dialect keys" }, "@object-ui/plugin-gantt": { errors: 3, issue: 4118 }, "@object-ui/plugin-chatbot": { errors: 2, issue: 4118 }, - "@object-ui/plugin-grid": { errors: 2, issue: 4118 }, }; // ── Collect workspace packages ───────────────────────────────────────────────