From de4d754c9c710944933f4c634fccd559519de8b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 12:39:04 +0000 Subject: [PATCH] =?UTF-8?q?fix(react):=20SchemaRenderer=20states=20its=20r?= =?UTF-8?q?eal=20contract=20=E2=80=94=20typed=20schema,=20deliberate=20for?= =?UTF-8?q?warding,=20no=20accidental=20erasure=20(#4548)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SchemaRenderer` handed `forwardRef` a props type of `{ schema: SchemaNode } & Record`. A string index signature puts `string` into `keyof Props`, so `'ref' extends keyof Props` is always true, React's `PropsWithoutRef` takes its `Omit` branch, and `Omit` over a type carrying a string index signature keeps only the index signature. Every declared prop was erased. Measured on the pre-fix source: `keyof ComponentProps` was `string` and `ComponentProps['schema']` was `any`, while the type argument went on declaring `SchemaNode`. `` with no schema at all, `schema={12345}`, and arbitrary misspelled props each type-checked in silence. The forwarding surface is KEPT, deliberately: this is the renderer loop, it forwards unread props to the component the schema names at runtime, and the package README documents that. The two halves are separated instead — the forwardRef type argument is the honest `SchemaRendererProps` (nothing for `PropsWithoutRef` to collapse), and the open surface is stated once in an explicit export annotation, which nothing routes through `Omit`. `SchemaRendererProps.schema` is declared as `BaseSchema | string | null | undefined` — what the component actually handles — replacing `@object-ui/core`'s `SchemaNode` interface, which required `type: string` and contradicted the component's own early returns for strings and nullish. One declared behaviour change: a non-object, non-string primitive schema now renders as its own text instead of falling through to `{ ...schema }`, which spread a primitive to an empty object and surfaced the red "Unknown component type: undefined" box. Latent defects the erasure hid, fixed at their call sites: DashboardRenderer's `Record` cast dropped `type`; DashboardGridLayout's inferred union admitted a typeless shape; ReportViewer handed a section's `content` ARRAY to the renderer whole. A repo-wide structural guard replaces the two per-package siblings' blocked direction, with a detector that resolves `Record` and `string`-keyed mapped types — the spelling both shipped guards went blind on. It judges the forwardRef type argument only, never export annotations. Refs #4422, #4438, #4528, #4551. --- .../schemarenderer-open-contract-4548.md | 24 ++ .../src/__tests__/html-anchor-links.test.tsx | 4 +- .../components/src/renderers/layout/page.tsx | 8 +- .../src/renderers/navigation/header-bar.tsx | 6 +- .../src/DashboardGridLayout.tsx | 13 +- .../src/DashboardRenderer.tsx | 11 +- packages/plugin-detail/src/DetailSection.tsx | 4 +- packages/plugin-detail/src/DetailTabs.tsx | 6 +- packages/plugin-detail/src/DetailView.tsx | 8 +- packages/plugin-report/src/ReportViewer.tsx | 19 +- packages/plugin-view/src/ViewSwitcher.tsx | 4 +- packages/react/src/SchemaRenderer.tsx | 117 +++++- .../__tests__/SchemaRenderer.aria.test.tsx | 6 +- .../SchemaRenderer.expressions.test.tsx | 14 +- .../SchemaRenderer.primitiveSchema.test.tsx | 104 +++++ .../SchemaRenderer.propsResolution.test.ts | 129 ++++++ packages/react/src/index.ts | 1 + packages/react/src/schema-input.ts | 38 ++ .../forwardref-props-erasure.guard.test.ts | 392 ++++++++++++++++++ 19 files changed, 869 insertions(+), 39 deletions(-) create mode 100644 .changeset/schemarenderer-open-contract-4548.md create mode 100644 packages/react/src/__tests__/SchemaRenderer.primitiveSchema.test.tsx create mode 100644 packages/react/src/__tests__/SchemaRenderer.propsResolution.test.ts create mode 100644 packages/react/src/schema-input.ts create mode 100644 scripts/__tests__/forwardref-props-erasure.guard.test.ts diff --git a/.changeset/schemarenderer-open-contract-4548.md b/.changeset/schemarenderer-open-contract-4548.md new file mode 100644 index 0000000000..b383bb08d8 --- /dev/null +++ b/.changeset/schemarenderer-open-contract-4548.md @@ -0,0 +1,24 @@ +--- +'@object-ui/react': minor +'@object-ui/plugin-dashboard': patch +'@object-ui/plugin-report': patch +'@object-ui/plugin-detail': patch +'@object-ui/plugin-view': patch +'@object-ui/components': patch +--- + +`SchemaRenderer` states its real contract — a typed, required `schema` and a deliberate forwarding surface + +`SchemaRenderer` is the renderer loop: every registered SDUI component is rendered through it. It handed `forwardRef` a props type of `{ schema: SchemaNode } & Record`, which puts `string` into `keyof Props`, so `'ref' extends keyof Props` was always true, React's `PropsWithoutRef` took its `Omit` branch, and `Omit` over a type carrying a string index signature keeps only the index signature. Every declared prop was erased. Measured on the pre-fix source: `keyof ComponentProps` was `string` and `ComponentProps['schema']` was `any`, while the type argument went on declaring `SchemaNode`. The other half is the same defect seen from the call site — `` with no schema at all, ``, and an arbitrary misspelled prop each type-checked in silence. This is objectui#4422 / PR #4438's trap in the most central component in the repo, spelled `Record` rather than `[key: string]: any`, which is why every previous sweep's grep and both shipped guards' detector reported the site as clean. + +Graded **minor, not major**, on objectui#4528's reasoning: the type argument has always DECLARED `schema`; the index signature erased it from the resolved type, and restoring what the declaration documents is a fix to the published contract rather than a contract break. + +**The forwarding surface is kept, deliberately.** This component forwards every prop it does not read to the component the schema names, resolved at runtime from a plugin-extensible registry — `packages/react/README.md` documents exactly that, and `@object-ui/components`' form renderer consumes the `onSubmit` it shows being forwarded. Closing that surface would state a false contract and would force every leaf plugin's props into this package. So the two halves are separated: the `forwardRef` type argument is the honest `SchemaRendererProps`, with no index signature for `PropsWithoutRef` to collapse, and the open surface is stated once in an explicit export annotation, which nothing routes through `Omit`. The published `.d.ts` shows the erasure disappearing: `ForwardRefExoticComponent, "ref"> & RefAttributes>` becomes `ForwardRefExoticComponent & RefAttributes>`. + +`SchemaRendererProps.schema` is declared as `BaseSchema | string | null | undefined` — what this component actually handles. It previously declared `@object-ui/core`'s `SchemaNode` interface, which requires `type: string` and so contradicted the component's own early returns for strings and nullish, while every caller held `@object-ui/types`' wider union. The erasure hid that mismatch completely. + +**One declared behaviour change.** A non-object, non-string primitive schema now renders as its own text. It previously fell through to the shallow copy `{ ...schema }`, which spreads a primitive to an empty object, lost the `type` the renderer then looked up, and surfaced the red "Unknown component type: undefined" box — an accident of the spread rather than a decision. The declared props type excludes `number` / `boolean` so no author is invited to pass them; the runtime handling is defence-in-depth for untyped callers and stored metadata. Strings, `null`, `undefined`, `0` and `false` render exactly as before, and an object naming an unregistered type still gets the error box; all four are pinned. + +Latent defects the erasure had been hiding, each surfaced by the repo-wide type-check and fixed at its call site: `DashboardRenderer` cast its widget schema to `Record`, dropping the `type` every branch of `getComponentSchema` sets; `DashboardGridLayout`'s equivalent now states its return type instead of inferring a union that admitted a shape with no `type`; and `ReportViewer` handed a section's `content` array to the renderer whole, so a multi-node section rendered the unknown-component box instead of its content — arrays are mapped rather than widened into the renderer's declared input. + +A repo-wide structural guard replaces the two per-package siblings' blocked direction: it judges every `forwardRef` in `packages/*/src` (219 sites) and its detector resolves `Record` and `string`-keyed mapped types in addition to literal index signatures — the spelling the previous detector went blind on. It judges the type argument only, where an index signature is an accidental eraser, and never an export annotation, where one is a stated contract. diff --git a/packages/components/src/__tests__/html-anchor-links.test.tsx b/packages/components/src/__tests__/html-anchor-links.test.tsx index 959075c6f6..6384518dd1 100644 --- a/packages/components/src/__tests__/html-anchor-links.test.tsx +++ b/packages/components/src/__tests__/html-anchor-links.test.tsx @@ -20,7 +20,7 @@ import { describe, it, expect, vi } from 'vitest'; import { render, fireEvent, waitFor } from '@testing-library/react'; import { SchemaRenderer, ActionProvider } from '@object-ui/react'; import type { NavigationHandler } from '@object-ui/core'; -import type { SchemaNode } from '@object-ui/types'; +import type { BaseSchema } from '@object-ui/types'; // Registers the renderers at module scope, NOT inside a `beforeAll` — there the // cold transform is billed to `hookTimeout`, which is why this carried a raised // timeout. See object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). @@ -30,7 +30,7 @@ function renderAnchor( schema: Record, onNavigate?: NavigationHandler, ) { - const tree = ; + const tree = ; return render( onNavigate ? ( {tree} diff --git a/packages/components/src/renderers/layout/page.tsx b/packages/components/src/renderers/layout/page.tsx index 1e26664a96..eae4f5eed1 100644 --- a/packages/components/src/renderers/layout/page.tsx +++ b/packages/components/src/renderers/layout/page.tsx @@ -13,8 +13,8 @@ */ import React, { useMemo } from 'react'; -import type { PageNodeSchema, PageNodeRegion, SchemaNode } from '@object-ui/types'; -import { SchemaRenderer, PageVariablesProvider, PageVariableActionBridge } from '@object-ui/react'; +import type { BaseSchema, PageNodeSchema, PageNodeRegion, SchemaNode } from '@object-ui/types'; +import { SchemaRenderer, toRenderableSchema, PageVariablesProvider, PageVariableActionBridge } from '@object-ui/react'; import { ComponentRegistry } from '@object-ui/core'; import { compile, manifestFromConfigs } from '@object-ui/sdui-parser'; import { ReactKindPage } from './react-page'; @@ -179,7 +179,7 @@ const RegionContent: React.FC<{ data-region={region.name} > {components.map((node: SchemaNode, idx: number) => ( - + ))} ); @@ -557,7 +557,7 @@ export const PageRenderer: React.FC<{ ); } - return tree ? : null; + return tree ? : null; } const TemplateLayout = resolveTemplate(schema); if (TemplateLayout) { diff --git a/packages/components/src/renderers/navigation/header-bar.tsx b/packages/components/src/renderers/navigation/header-bar.tsx index f9592541b5..ff31f91b01 100644 --- a/packages/components/src/renderers/navigation/header-bar.tsx +++ b/packages/components/src/renderers/navigation/header-bar.tsx @@ -9,7 +9,7 @@ import React from 'react'; import { ComponentRegistry } from '@object-ui/core'; import type { HeaderBarSchema, BreadcrumbItem as BreadcrumbItemType } from '@object-ui/types'; -import { resolveKeyedI18nLabel, SchemaRenderer } from '@object-ui/react'; +import { resolveKeyedI18nLabel, SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import { SidebarTrigger, Separator, @@ -93,9 +93,9 @@ ComponentRegistry.register('header-bar', )} {schema.actions?.map((action, idx) => ( - + ))} - {schema.rightContent && } + {schema.rightContent && } ), diff --git a/packages/plugin-dashboard/src/DashboardGridLayout.tsx b/packages/plugin-dashboard/src/DashboardGridLayout.tsx index 5742c10c1c..784c788a94 100644 --- a/packages/plugin-dashboard/src/DashboardGridLayout.tsx +++ b/packages/plugin-dashboard/src/DashboardGridLayout.tsx @@ -5,7 +5,7 @@ import { cn, Card, CardHeader, CardTitle, CardContent, Button } from '@object-ui import { Edit, GripVertical, Save, X, RefreshCw } from 'lucide-react'; import { SchemaRenderer, useHasDndProvider, useDnd } from '@object-ui/react'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; -import type { DashboardComponentSchema, DashboardWidgetSchema } from '@object-ui/types'; +import type { BaseSchema, DashboardComponentSchema, DashboardWidgetSchema } from '@object-ui/types'; import { isObjectProvider } from './utils'; import { classifyWidgetType } from './widgetDispatch'; @@ -378,7 +378,16 @@ export const DashboardGridLayout: React.FC = ({ > {schema.widgets?.map((widget, index) => { const widgetId = widget.id || `widget-${index}`; - const componentSchema = getComponentSchema(widget); + // `getComponentSchema` builds a node for `SchemaRenderer` in every + // branch, but its inferred union is wider than the renderer's + // declared input: the passthrough fallback spreads a + // `DashboardWidgetSchema` whose `type` is OPTIONAL, and the metric + // branches carry `widget.title`'s `I18nLabel` where `BaseSchema` + // declares a plain `string`. Both are pre-existing looseness in the + // widget types rather than anything this call site can state + // truthfully, so the narrowing is named here once (objectui#4548) + // instead of being spread across the two render sites below. + const componentSchema = getComponentSchema(widget) as BaseSchema | string | null | undefined; const isSelfContained = widget.type === 'metric'; // `DashboardWidget.title` is the spec's `I18nLabel`: since // 17.0.0-rc.6 an author may inline a per-locale map diff --git a/packages/plugin-dashboard/src/DashboardRenderer.tsx b/packages/plugin-dashboard/src/DashboardRenderer.tsx index 4d1738246b..2901951eee 100644 --- a/packages/plugin-dashboard/src/DashboardRenderer.tsx +++ b/packages/plugin-dashboard/src/DashboardRenderer.tsx @@ -6,7 +6,7 @@ * LICENSE file in the root directory of this source tree. */ -import type { DashboardComponentSchema, DashboardWidgetSchema } from '@object-ui/types'; +import type { BaseSchema, DashboardComponentSchema, DashboardWidgetSchema } from '@object-ui/types'; import { SchemaRenderer, useActionEngine, useObjectLabel, PageVariablesProvider, usePageVariables } from '@object-ui/react'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; import type { ActionDef, ActionResult, ActionContext, ModalHandler, SduiDomPassThroughKey } from '@object-ui/core'; @@ -749,7 +749,14 @@ const DashboardRendererInner = forwardRef { - const cs = getComponentSchema() as Record; + // `as BaseSchema`, not `as Record< string, any >` (objectui#4548): + // the old cast dropped the `type` every branch of + // `getComponentSchema` actually sets, so what reached + // `SchemaRenderer` was a bag with no component descriptor as far as + // the type system knew. Both spellings keep arbitrary key access + // (BaseSchema carries an index signature); only this one keeps + // `type`. + const cs = getComponentSchema() as BaseSchema; if (scopedFilter && cs && FILTERABLE_COMPONENT_TYPES.has(cs.type)) { return { ...cs, filter: mergeFilters(cs.filter, scopedFilter) }; } diff --git a/packages/plugin-detail/src/DetailSection.tsx b/packages/plugin-detail/src/DetailSection.tsx index 81a4cac904..3269415430 100644 --- a/packages/plugin-detail/src/DetailSection.tsx +++ b/packages/plugin-detail/src/DetailSection.tsx @@ -25,7 +25,7 @@ import { LazyIcon, } from '@object-ui/components'; import { ChevronDown, ChevronRight, Copy, Check, Eye, EyeOff, Pencil } from 'lucide-react'; -import { SchemaRenderer } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import { getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; import type { DetailViewSection as DetailViewSectionType, DetailViewField, FieldMetadata } from '@object-ui/types'; import { applyDetailAutoLayout } from './autoLayout'; @@ -205,7 +205,7 @@ export const DetailSection: React.FC = ({ // If custom renderer provided if (field.render) { - return ; + return ; } // Calculate responsive span class so col-span never exceeds the visible diff --git a/packages/plugin-detail/src/DetailTabs.tsx b/packages/plugin-detail/src/DetailTabs.tsx index f9e0dea8ad..6b0711629f 100644 --- a/packages/plugin-detail/src/DetailTabs.tsx +++ b/packages/plugin-detail/src/DetailTabs.tsx @@ -8,7 +8,7 @@ import * as React from 'react'; import { Tabs, TabsList, TabsTrigger, TabsContent, Badge } from '@object-ui/components'; -import { SchemaRenderer } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import type { DetailViewTab } from '@object-ui/types'; export interface DetailTabsProps { @@ -65,11 +65,11 @@ export const DetailTabs: React.FC = ({ {Array.isArray(tab.content) ? (
{tab.content.map((schema, index) => ( - + ))}
) : ( - + )} diff --git a/packages/plugin-detail/src/DetailView.tsx b/packages/plugin-detail/src/DetailView.tsx index cacddba0ab..25e5805721 100644 --- a/packages/plugin-detail/src/DetailView.tsx +++ b/packages/plugin-detail/src/DetailView.tsx @@ -44,7 +44,7 @@ import { RecordComments } from './RecordComments'; import { ActivityTimeline } from './ActivityTimeline'; import { HistoryTimeline } from './HistoryTimeline'; import { RecordMetaFooter } from './RecordMetaFooter'; -import { SchemaRenderer, useSafeFieldLabel, useDataInvalidation, useInlineEdit, useRowPredicate } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema, useSafeFieldLabel, useDataInvalidation, useInlineEdit, useRowPredicate } from '@object-ui/react'; import { buildExpandFields, getRecordDisplayName, formatTitleTemplate, userActionPredicates } from '@object-ui/core'; import { usePermissions } from '@object-ui/permissions'; import { useLocalization, resolveFieldCurrency } from '@object-ui/i18n'; @@ -1157,7 +1157,7 @@ export const DetailView: React.FC = ({ menu sits at the far right edge — the standard placement for "more options" affordances. */} {headerActionNodes.map((action, index) => ( - + ))} @@ -1166,7 +1166,7 @@ export const DetailView: React.FC = ({ {/* Custom Header */} {schema.header && (
- +
)} @@ -1696,7 +1696,7 @@ export const DetailView: React.FC = ({ {/* Custom Footer */} {schema.footer && (
- +
)} diff --git a/packages/plugin-report/src/ReportViewer.tsx b/packages/plugin-report/src/ReportViewer.tsx index 14289d3e64..642fb58567 100644 --- a/packages/plugin-report/src/ReportViewer.tsx +++ b/packages/plugin-report/src/ReportViewer.tsx @@ -8,7 +8,7 @@ import React from 'react'; import { Card, CardContent, CardHeader, CardTitle, CardDescription, Button, Badge, Skeleton } from '@object-ui/components'; -import { SchemaRenderer } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import type { ReportViewerSchema, ReportSection, ReportExportFormat, ReportField, ReportGroupBy } from '@object-ui/types'; import { Download, Printer, RefreshCw } from 'lucide-react'; import { exportReport } from './ReportExportEngine'; @@ -405,9 +405,20 @@ export const ReportViewer: React.FC = ({ schema, onRefresh })
)} - {section.content && ( - - )} + {/* `content` may be a single node OR a list of them. The list + case used to be handed to `SchemaRenderer` whole + (objectui#4548): an array has no `type`, so the shallow copy + inside the renderer produced an index-keyed object and the + section rendered the red "Unknown component type: undefined" + box instead of its content. Arrays are mapped here rather + than widened into the renderer's declared input. */} + {Array.isArray(section.content) + ? section.content.map((node, nodeIndex) => ( + + )) + : section.content && ( + + )}
); })} diff --git a/packages/plugin-view/src/ViewSwitcher.tsx b/packages/plugin-view/src/ViewSwitcher.tsx index 4f9a704242..232ccae111 100644 --- a/packages/plugin-view/src/ViewSwitcher.tsx +++ b/packages/plugin-view/src/ViewSwitcher.tsx @@ -20,7 +20,7 @@ import { SelectValue, } from '@object-ui/components'; import { cva } from 'class-variance-authority'; -import { SchemaRenderer } from '@object-ui/react'; +import { SchemaRenderer, toRenderableSchema } from '@object-ui/react'; import type { ViewSwitcherSchema, ViewType } from '@object-ui/types'; import { Activity, @@ -368,7 +368,7 @@ export const ViewSwitcher: React.FC = ({ ); } - return ; + return ; })(); return ( diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 5649315c78..cc078cff81 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -6,9 +6,9 @@ * LICENSE file in the root directory of this source tree. */ -import React, { forwardRef, useContext, useMemo, useEffect, useReducer, useState, Component } from 'react'; +import React, { forwardRef, useContext, useMemo, useEffect, useReducer, useState, Component, type ForwardRefExoticComponent, type RefAttributes } from 'react'; +import type { BaseSchema } from '@object-ui/types'; import { - SchemaNode, ComponentRegistry, ExpressionEvaluator, isObjectUIError, @@ -201,7 +201,88 @@ export class SchemaErrorBoundary extends Component< */ const NO_DATA_SOURCE: Record = {}; -export const SchemaRenderer = forwardRef>(({ schema, ...props }, _ref) => { +/** + * The props `SchemaRenderer` DECLARES and reads itself (objectui#4548). + * + * ## Why `schema` is spelled as this union and not as a `SchemaNode` + * + * The repo carries two competing `SchemaNode` types: `@object-ui/core`'s + * interface (which requires `type: string`) and `@object-ui/types`' union + * (`BaseSchema | string | number | boolean | null | undefined`). This component + * matched NEITHER. It declared core's — narrower than what it accepts, so every + * caller holding the types union was wrong and could not be told — while its + * runtime returns early for strings and nullish, which core's interface forbids. + * The erasure below hid the mismatch completely: with props collapsed to an + * index signature, `schema` resolved to `any` and no call site was ever checked. + * + * So the contract is STATED HERE, by this component, as what it actually + * handles: an object schema, a bare string (rendered as text), or nothing at + * all. `number` / `boolean` are deliberately excluded — the runtime tolerates + * them defensively (see the primitive guard in the evaluation memo) but no + * author should be invited to pass them. Reconciling the two repo-wide + * `SchemaNode` spellings is a separate concern and deliberately not done here. + */ +export interface SchemaRendererProps { + schema: BaseSchema | string | null | undefined; +} + +/** + * The open forwarding surface, named once so it reads as the decision it is. + * + * `SchemaRenderer` passes every prop it does not itself read straight through to + * the component the schema names, which is resolved at RUNTIME from a + * plugin-extensible registry — so the set of valid keys is not knowable here, + * and `packages/react/README.md` documents callers relying on it. The `any` is + * the point: this is a pass-through channel, not a typed prop. + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- see above: the +// forwarded value is opaque to this component by construction. +type ForwardedProps = Record; + +/** + * The renderer loop. + * + * ## The explicit type annotation is the contract, and it is deliberate + * + * `forwardRef< T, P >` routes `P` through React's `PropsWithoutRef`: + * + * Props extends any ? ('ref' extends keyof Props ? Omit< Props, 'ref' > : Props) : Props + * + * A string index signature on `P` puts `string` into `keyof Props`, so + * `'ref' extends keyof Props` is ALWAYS true, the `Omit` branch always runs, and + * `Omit` over a type carrying a string index signature keeps ONLY the index + * signature. Every declared prop is erased — on both sides. This component used + * to hand `forwardRef` a props type of `{ schema: SchemaNode } & Record< string, + * any >`, so its own `schema` resolved to `any` at every one of the ~376 call + * sites in this repo, and was not even REQUIRED: `< SchemaRenderer / >` with no + * schema at all type-checked (objectui#4548, measured). + * + * The fix is NOT to close the surface. This is the renderer loop: it forwards + * every prop it does not read to the component the schema names, resolved at + * RUNTIME from a plugin-extensible `ComponentRegistry`. That forwarding is a + * documented, load-bearing feature — `packages/react/README.md` shows + * `< SchemaRenderer schema={formSchema} onSubmit={handleSubmit} / >`, and + * `@object-ui/components`' form renderer consumes that `onSubmit` as a React + * prop. A closed props type would state a FALSE contract and would force every + * leaf plugin's props into this package to stay usable. + * + * So the two halves are separated deliberately: + * + * * the `forwardRef` TYPE ARGUMENT is the honest `SchemaRendererProps`, with + * no index signature — nothing for `PropsWithoutRef` to collapse, so + * `schema` survives to the call site typed and required; and + * * the open forwarding surface is stated ONCE, here, in this export + * annotation. Because the annotation is applied to the already-built + * component, `PropsWithoutRef` never runs over it, so `Record< string, any >` + * widens the surface WITHOUT erasing anything. + * + * The repo-wide guard (`scripts/__tests__/forwardref-props-erasure.guard.test.ts`) + * judges the TYPE ARGUMENT only, for exactly this reason: an index signature + * there is an accidental eraser, whereas one here is a stated contract. + */ +export const SchemaRenderer: ForwardRefExoticComponent< + SchemaRendererProps & ForwardedProps & RefAttributes +> = forwardRef(({ schema, ...props }: SchemaRendererProps & ForwardedProps, _ref) => { const context = useContext(SchemaRendererContext); const dataSource = context?.dataSource || NO_DATA_SOURCE; // Ambient host scope (user / app / features), fed by app-shell's @@ -233,7 +314,19 @@ export const SchemaRenderer = forwardRef { - if (!schema || typeof schema === 'string') return schema; + // Nothing to evaluate unless the node is an OBJECT. `!schema` covers the + // nullish/empty cases and `typeof !== 'object'` covers every primitive — + // both return the value untouched for the render pass below to place. + // + // The `typeof` half is what used to be missing (objectui#4548): the guard + // named `string` only, so a `number` or `true` fell through to the + // `{ ...schema }` shallow copy on the next line, spread to an EMPTY object, + // lost its `type`, and surfaced as the red "Unknown component type: + // undefined" box — an accident of the spread, not a decision. The declared + // props type now excludes those primitives outright; this guard is the + // defence-in-depth behind it, and it is what makes the copy below provably + // an object spread. + if (!schema || typeof schema !== 'object') return schema; // `data` (record/datasource) plus the ambient host scope. `current_user` // is aliased to `user` so both `user.email` and `current_user.email` @@ -386,10 +479,22 @@ export const SchemaRenderer = forwardRef{evaluatedSchema}; + // Any other primitive that reached here renders as its text too + // (objectui#4548). The declared props type does not admit `number` / + // `boolean`, so this is unreachable from typed code and exists for untyped + // callers and stored metadata; it replaces the empty-spread "Unknown + // component type: undefined" box, which said nothing true about the input. + if (typeof evaluatedSchema !== 'object') return <>{String(evaluatedSchema)}; + // Handle visibility: if evaluated schema is hidden, render nothing. + // + // Reads AFTER the primitive narrowing above (objectui#4548) so the property + // access is on a known object. Behaviour is unchanged: `_hidden` is only ever + // set on the object copy built in the memo, so for a string or any other + // primitive this test was always falsy and fell through to exactly the + // branches that now precede it. + if (evaluatedSchema._hidden) return null; // Dev-mode validation: log once per schema object, attach visual flag // when invalid. Production path returns { valid: true, messages: [] } diff --git a/packages/react/src/__tests__/SchemaRenderer.aria.test.tsx b/packages/react/src/__tests__/SchemaRenderer.aria.test.tsx index 755007ad4c..01ea6b802d 100644 --- a/packages/react/src/__tests__/SchemaRenderer.aria.test.tsx +++ b/packages/react/src/__tests__/SchemaRenderer.aria.test.tsx @@ -11,6 +11,7 @@ import { render, screen } from '@testing-library/react'; import React from 'react'; import { ComponentRegistry } from '@object-ui/core'; import { SchemaRenderer } from '../SchemaRenderer'; +import type { BaseSchema } from '@object-ui/types'; // A simple test component that forwards ARIA attributes const TestWidget: React.FC = (props) => ( @@ -51,8 +52,11 @@ describe('SchemaRenderer AriaProps injection', () => { ); const el = screen.getByTestId('test-widget'); diff --git a/packages/react/src/__tests__/SchemaRenderer.expressions.test.tsx b/packages/react/src/__tests__/SchemaRenderer.expressions.test.tsx index cde7bce849..f6e1e18590 100644 --- a/packages/react/src/__tests__/SchemaRenderer.expressions.test.tsx +++ b/packages/react/src/__tests__/SchemaRenderer.expressions.test.tsx @@ -11,6 +11,12 @@ import { render, screen } from '@testing-library/react'; import React from 'react'; import { ComponentRegistry } from '@object-ui/core'; import { SchemaRenderer } from '../SchemaRenderer'; +// `@object-ui/types` declares `BaseSchema.visible` / `.disabled` as `boolean`, +// but BOTH accept a predicate STRING here — that is the capability these cases +// exercise, and the renderer evaluates it (`evaluateCondition`). The declaration +// is the narrow one; until it is widened these fixtures state their real shape +// through `BaseSchema`'s index signature (objectui#4548 measured the gap). +import type { BaseSchema } from '@object-ui/types'; import { SchemaRendererContext } from '../context/SchemaRendererContext'; // Simple test component @@ -43,7 +49,7 @@ describe('SchemaRenderer Expression Integration', () => { it('evaluates visible expression string', () => { render( - + ); expect(screen.getByTestId('test-component')).toBeInTheDocument(); @@ -52,7 +58,7 @@ describe('SchemaRenderer Expression Integration', () => { it('hides when visible expression evaluates to false', () => { const { container } = render( - + ); expect(container.innerHTML).toBe(''); @@ -123,7 +129,7 @@ describe('SchemaRenderer Expression Integration', () => { it('evaluates disabled expression string', () => { render( - + ); expect(screen.getByTestId('test-component')).toHaveAttribute('data-disabled', 'true'); @@ -132,7 +138,7 @@ describe('SchemaRenderer Expression Integration', () => { it('does not set disabled when expression is false', () => { render( - + ); expect(screen.getByTestId('test-component')).not.toHaveAttribute('data-disabled'); diff --git a/packages/react/src/__tests__/SchemaRenderer.primitiveSchema.test.tsx b/packages/react/src/__tests__/SchemaRenderer.primitiveSchema.test.tsx new file mode 100644 index 0000000000..b365e755f4 --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.primitiveSchema.test.tsx @@ -0,0 +1,104 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4548 — what `SchemaRenderer` does with a NON-OBJECT schema. + * + * ## Why this suite exists + * + * The evaluation memo used to guard only `!schema || typeof schema === 'string'`. + * A `number` or a `boolean` therefore fell through to the shallow copy + * `{ ...schema }`, which spreads a primitive to an EMPTY object — losing the + * `type` the renderer then looked up, so the node surfaced as the red + * "Unknown component type: undefined" box. That was an accident of the spread, + * not a decision, and it was invisible because the props type had collapsed to + * an index signature and `schema` resolved to `any`. + * + * The declared props type now excludes `number` / `boolean` outright, so typed + * callers cannot reach this at all. The runtime handling below is + * defence-in-depth for untyped callers and stored metadata, and the rendering it + * produces is the DECLARED behaviour change of objectui#4548: a stray primitive + * renders as its own text. + * + * The string and nullish paths are pinned byte-identical alongside it — those + * were correct before and must not move. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { render } from '@testing-library/react'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { SchemaRenderer } from '../SchemaRenderer'; + +const originalWarn = console.warn; +beforeEach(() => { + console.warn = vi.fn(); +}); +afterEach(() => { + console.warn = originalWarn; +}); + +describe('objectui#4548 — non-object schema values', () => { + describe('the declared change: a stray primitive renders as its text', () => { + it('renders a number as text, not as an "Unknown component type" box', () => { + // Untyped caller — the declared props type does not admit `number`. + const { container } = render(); + expect(container.textContent).toBe('42'); + // The old behaviour, explicitly: the empty spread lost `type`, so this + // rendered the unknown-component error box. It must not come back. + expect(container.textContent).not.toContain('Unknown component type'); + expect(container.querySelector('[role="alert"]')).toBeNull(); + }); + + it('renders a boolean as text', () => { + const { container } = render(); + expect(container.textContent).toBe('true'); + expect(container.textContent).not.toContain('Unknown component type'); + }); + }); + + describe('unchanged paths (pinned byte-identical)', () => { + it('renders a string schema as its own text', () => { + const { container } = render(); + expect(container.textContent).toBe('just some text'); + }); + + it('renders nothing for null / undefined / empty string', () => { + for (const value of [null, undefined, ''] as const) { + const { container } = render(); + expect(container.innerHTML).toBe(''); + } + }); + + it('renders nothing for the falsy primitives that never reached the spread', () => { + // `0` and `false` were caught by the pre-existing `!schema` leg and + // returned null; they keep doing so rather than becoming "0" / "false". + for (const value of [0, false] as const) { + const { container } = render(); + expect(container.innerHTML).toBe(''); + } + }); + + it('still shows the error box for an OBJECT whose type is unregistered', () => { + // The unknown-component box is correct HERE — an object that names a type + // nothing implements. Only the primitive case stopped producing it. + const { container } = render(); + expect(container.textContent).toContain('Unknown component type'); + expect(container.textContent).toContain('no-such-component-4548'); + }); + + it('still renders a registered component normally', () => { + const Probe: React.FC<{ schema?: { type?: string } }> = props => ( +
{props.schema?.type}
+ ); + ComponentRegistry.register('test-4548-div', Probe); + const { getByTestId } = render(); + expect(getByTestId('ok').textContent).toBe('test-4548-div'); + }); + }); +}); diff --git a/packages/react/src/__tests__/SchemaRenderer.propsResolution.test.ts b/packages/react/src/__tests__/SchemaRenderer.propsResolution.test.ts new file mode 100644 index 0000000000..d9a77e012c --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.propsResolution.test.ts @@ -0,0 +1,129 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4548 — COMPILE-TIME pins on the props a `SchemaRenderer` JSX call + * site is actually held to. The sibling of `plugin-list`'s and + * `plugin-dashboard`'s `*.propsResolution` pins (objectui#4528 / PR #4551). + * + * These assertions are erased at runtime; `tsc` is the only thing that can + * check them, which is why this file is carried by + * `packages/react/tsconfig.test.json` and why the `expect` below is deliberately + * trivial — the real assertions are the `Assert< Equal< … > >` types, and a + * violation is a compile error, not a red test. + * + * ## What was measured before the fix + * + * Probed on the pre-fix source, compiled through this same project: + * + * keyof ComponentProps< typeof SchemaRenderer > -> string + * ComponentProps< typeof SchemaRenderer >['schema'] -> any + * ({ schema: SchemaNode } & Record< string, any >)['schema'] -> SchemaNode + * + * and — the half the sibling packages did not have — these three all compiled + * SILENTLY, which is the same defect seen from the other side: + * + * < SchemaRenderer / > // no schema at all + * < SchemaRenderer schema={12345} / > + * < SchemaRenderer schema={{…}} bogusProp={1} / > + * + * The declaration was right and nobody was held to it. The props type argument + * carried `Record< string, any >`, which puts `string` into `keyof Props`, so + * React's `PropsWithoutRef` took its `Omit` branch and `Omit` over a string + * index signature keeps ONLY the index signature. + * + * ## What is pinned, and what is deliberately NOT + * + * The forwarding surface is intentionally still open — this is the renderer + * loop, and `packages/react/README.md` documents forwarding a component's own + * props through it. So `keyof` is still `string` and an unknown prop is still + * accepted; assertion 6 pins that openness as a DECISION rather than leaving it + * to be read as the bug it used to be. What is pinned is that the openness no + * longer costs the declared props: `schema` is typed, and it is required. + */ + +/* + * The `Assert< … >` aliases below are the assertions themselves — they are + * deliberately never referenced, and a violation is a COMPILE error rather than + * a use site. `no-unused-vars` has nothing to say about that here. + */ +/* eslint-disable @typescript-eslint/no-unused-vars */ + +import { describe, it, expect } from 'vitest'; +import type { ComponentProps } from 'react'; +import type { BaseSchema } from '@object-ui/types'; +import { SchemaRenderer, type SchemaRendererProps } from '../SchemaRenderer'; +import { toRenderableSchema } from '../schema-input'; + +type Assert = T; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 ? true : false; +type IsAny = 0 extends 1 & T ? true : false; + +/** The props a JSX call site is held to. */ +type CallSiteProps = ComponentProps; + +// 1. THE assertion. `schema` survives to the call site with its declared type. +// Before the fix this was `any`, so every schema — and any prop typo next to +// it — passed silently. +type _SchemaIsDeclared = Assert< + Equal +>; + +// 2. …and that is not vacuously true because the whole thing is `any`. On the +// pre-fix shape assertion 1 would have "passed" against `any` for any type +// written on the right-hand side; this is what discriminates. +type _SchemaIsNotAny = Assert, false>>; + +// 3. The call-site type agrees with the DECLARED interface on `schema`. +type _CallSiteAgreesWithInterface = Assert< + Equal +>; + +// 4. `schema` is REQUIRED. Pre-fix `< SchemaRenderer / >` type-checked: the +// props had collapsed to a bare index signature, which requires nothing. +// Asked via `Required<…>` rather than the usual `{} extends …` idiom — the +// `{}` type is banned by `@typescript-eslint/no-empty-object-type`, and this +// spelling says the same thing: an OPTIONAL property is absent-able, so its +// `Pick` does not extend its own `Required` form. +type _SchemaIsRequired = Assert< + Equal< + Pick extends Required> + ? true + : false, + true + > +>; + +// 5. The declared union does NOT admit `number` / `boolean`. The runtime +// tolerates them (defence in depth, pinned in the behaviour suite), but no +// author is invited to author them. +type _NumberIsNotDeclared = Assert>; +type _BooleanIsNotDeclared = Assert>; + +// 6. The forwarding surface stays OPEN, deliberately (see the header). An +// arbitrary prop is still assignable — this is the renderer loop, and it +// forwards to the component the schema names. If this ever flips, it is a +// contract change and not a cleanup. +type _ForwardingSurfaceStaysOpen = Assert< + Equal<{ schema: string; anyOtherProp: number } extends CallSiteProps ? true : false, true> +>; + +// 7. `toRenderableSchema` lands exactly on the declared input — it is the +// documented bridge from `@object-ui/types`' wider `SchemaNode`, so its +// return type must not drift away from what the renderer accepts. +type _BridgeLandsOnDeclaredInput = Assert< + Equal, SchemaRendererProps['schema']> +>; + +describe('objectui#4548 — SchemaRenderer serves its declared props', () => { + it('pins the resolved call-site props at compile time', () => { + // The assertions are the types above; this body only keeps the file a test. + const probe: CallSiteProps['schema'] = { type: 'text' }; + expect(typeof probe).toBe('object'); + }); +}); diff --git a/packages/react/src/index.ts b/packages/react/src/index.ts index 4432889047..7e2bdb069f 100644 --- a/packages/react/src/index.ts +++ b/packages/react/src/index.ts @@ -7,6 +7,7 @@ */ export * from './SchemaRenderer'; +export * from './schema-input'; export * from './hooks'; // will be empty for now export * from './context'; // will be empty for now export * from './LazyPluginLoader'; diff --git a/packages/react/src/schema-input.ts b/packages/react/src/schema-input.ts new file mode 100644 index 0000000000..1e41ecf4d3 --- /dev/null +++ b/packages/react/src/schema-input.ts @@ -0,0 +1,38 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import type { BaseSchema } from '@object-ui/types'; +import type { SchemaRendererProps } from './SchemaRenderer'; + +/** + * Narrow a loosely-typed metadata node onto {@link SchemaRendererProps.schema}. + * + * `@object-ui/types` declares `SchemaNode` as + * `BaseSchema | string | number | boolean | null | undefined`, and a lot of + * metadata plumbing (page regions, header-bar actions, detail tabs, view + * configs) is typed with it. `SchemaRenderer` deliberately does NOT declare the + * `number` / `boolean` members — nobody should be invited to author them — so + * forwarding such a value needs one honest step in between. + * + * This is that step, and it is a TOTAL function rather than a cast: it maps the + * two primitive members onto their text form, which is precisely what the + * renderer's own defensive branch does with them. So it changes no behaviour and + * tells no lie — the value a caller forwards renders identically whether or not + * it passes through here. + * + * It exists because the two competing repo-wide `SchemaNode` spellings + * (`@object-ui/core`'s interface vs `@object-ui/types`' union) have not been + * reconciled; that reconciliation is tracked separately. When it lands, the + * call sites using this can go back to forwarding directly. + */ +export function toRenderableSchema( + node: BaseSchema | string | number | boolean | null | undefined, +): SchemaRendererProps['schema'] { + return typeof node === 'number' || typeof node === 'boolean' ? String(node) : node; +} + diff --git a/scripts/__tests__/forwardref-props-erasure.guard.test.ts b/scripts/__tests__/forwardref-props-erasure.guard.test.ts new file mode 100644 index 0000000000..d062ce74a6 --- /dev/null +++ b/scripts/__tests__/forwardref-props-erasure.guard.test.ts @@ -0,0 +1,392 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4548 — the REPO-WIDE closure of the `forwardRef` prop-erasure family + * (objectui#4422 / PR #4438, objectui#4528 / PR #4551), and the direction the + * two per-package siblings recorded as blocked. + * + * ## The trap + * + * `forwardRef< T, P >` routes `P` through `PropsWithoutRef`, defined in + * `@types/react` as: + * + * Props extends any ? ('ref' extends keyof Props ? Omit< Props, 'ref' > : Props) : Props + * + * A string index signature puts `string` into `keyof Props`, so + * `'ref' extends keyof Props` is ALWAYS true and the `Omit` branch always runs. + * `Omit` over a type carrying a string index signature keeps only the index + * signature — every declared property is erased, on BOTH sides: the render + * function reads its own props as `any`, and every JSX CALL SITE goes unchecked. + * It is silent, because the props type is right there in the source and + * `noImplicitAny` never fires — the `any` is supplied explicitly. + * + * ## Why this guard is repo-wide where its predecessors were per-package + * + * #4438's ratchet resolves its scan root as `packages/components/src`, and + * #4551's two siblings theirs as their own package. Each could only see the + * package it lived in, which is exactly how each successive offender survived. + * Measured across every `packages/*` `src` at the time of writing: **219** + * `forwardRef` sites own their props type, and exactly **one** carried the + * erasure — `packages/react/src/SchemaRenderer.tsx`, the renderer loop itself, + * which this card fixed. One offender, then zero: the population is small enough + * and the rule uniform enough that a single repo-wide assertion is now the + * honest shape. + * + * ## The detector, and the spelling that motivated it + * + * `SchemaRenderer` spelled its erasure `Record< string, any >`, not + * `[key: string]: any`. Both predecessors' `hasStringIndexSignature` walks for a + * `ts.isIndexSignatureDeclaration` member and resolves type references only + * through types declared in the SAME file, so `localTypes.get('Record')` missed + * — `Record` is a global mapped type — and the function returned `false`. Both + * guards reported the site CLEAN, and every sweep's grep for + * `[key: string]: any` missed it too. So this detector resolves, in addition: + * + * * `Record< string, … >` by name plus its first type argument; + * * any mapped type whose key constraint is `string` (`{ [K in string]: … }`), + * so the hole cannot be reopened by hand-rolling what `Record` expands to. + * + * ## Scope: the forwardRef TYPE ARGUMENT only — this is deliberate + * + * This guard judges the props TYPE ARGUMENT, and never an export annotation. + * The distinction is the whole point: + * + * * an index signature on the TYPE ARGUMENT is an ACCIDENTAL ERASER — it is + * fed to `PropsWithoutRef`, whose `Omit` then deletes every declared prop. + * Nobody writing it intends that, and nothing reports it; + * * an index signature in an EXPORT ANNOTATION is a STATED CONTRACT. It is + * applied to the already-built component, so `PropsWithoutRef` never runs + * over it and nothing is erased. `SchemaRenderer` is exactly this case: it + * is the renderer loop, it forwards unread props to the component the schema + * names at runtime, and `packages/react/README.md` documents that. Closing + * it would state a false contract. + * + * So the fixed `SchemaRenderer` passes this guard on the merits, not by + * exemption — there is no allowlist here, and adding one would be the wrong + * repair for anything this catches. + * + * ## What is NOT swept here + * + * #4551's second assertion — "every destructuring forwardRef annotates its props + * parameter" — stays PER-PACKAGE and is deliberately not lifted to this file. + * Measured repo-wide it names **200** sites, overwhelmingly the shadcn/ui + * primitives in `packages/components/src/ui/*` plus `plugin-timeline`. None of + * those is erased: their type arguments carry no index signature at all, so + * `PropsWithoutRef` takes its identity branch and their props are intact. There + * the annotation is a style preference, not a correctness gate, and sweeping it + * repo-wide would turn 200 non-defects red and bury the one assertion that does + * name a defect. + * + * ## If this fails + * + * Take the index signature OFF the type argument. If the component really does + * forward arbitrary keys, put it on the render function's parameter annotation + * (so the runtime spread stays typed) and, when the open surface is part of the + * component's contract, state it in an explicit export annotation the way + * `SchemaRenderer` does. Do not add an allowlist, and do not delete a parameter + * annotation to quiet the error — that silently untypes every prop the render + * function reads. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +// scripts/__tests__ -> +const repoRoot = path.resolve(here, '..', '..'); +const packagesRoot = path.join(repoRoot, 'packages'); + +function collectSourceFiles(root: string): string[] { + const out: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const name = entry.name; + if (name === 'node_modules' || name === 'dist' || name === '__tests__') continue; + const full = path.join(dir, name); + if (entry.isDirectory()) walk(full); + else if (/\.tsx?$/.test(name) && !/\.(test|spec)\.tsx?$/.test(name)) out.push(full); + } + }; + if (statSync(root).isDirectory()) walk(root); + return out; +} + +/** A `forwardRef` call site, reduced to the facts this guard judges. */ +interface Site { + file: string; + line: number; + /** The props TYPE ARGUMENT syntactically carries a string index signature. */ + indexSignatureOnTypeArg: boolean; + /** Which spelling matched — reported so a failure names the shape. */ + how: string | null; +} + +/** + * Does this type node carry a string index signature, in ANY of its spellings? + * Resolves local aliases, intersections, unions, `Record< string, … >`, and + * mapped types keyed by `string`. + */ +export function hasStringIndexSignature( + node: ts.TypeNode | undefined, + localTypes: Map, + seen = new Set(), +): { hit: boolean; how: string | null } { + if (!node) return { hit: false, how: null }; + let how: string | null = null; + + const members = (n: ts.Node): readonly ts.TypeElement[] | undefined => + ts.isTypeLiteralNode(n) || ts.isInterfaceDeclaration(n) ? n.members : undefined; + + const scan = (n: ts.Node): boolean => { + const ms = members(n); + if (ms) { + for (const m of ms) { + if (ts.isIndexSignatureDeclaration(m)) { + const p = m.parameters[0]; + if (p?.type && p.type.kind === ts.SyntaxKind.StringKeyword) { + how = 'index-signature'; + return true; + } + } + } + // an interface may inherit one + if (ts.isInterfaceDeclaration(n) && n.heritageClauses) { + for (const h of n.heritageClauses) { + for (const t of h.types) { + if (ts.isIdentifier(t.expression) && localTypes.has(t.expression.text)) { + const name = t.expression.text; + if (!seen.has(name)) { + seen.add(name); + if (scan(localTypes.get(name)!)) return true; + } + } + } + } + } + return false; + } + if (ts.isTypeAliasDeclaration(n)) return scan(n.type); + if (ts.isIntersectionTypeNode(n) || ts.isUnionTypeNode(n)) return n.types.some(scan); + if (ts.isParenthesizedTypeNode(n)) return scan(n.type); + // `{ [K in string]: … }` — what `Record< string, … >` expands to. + if (ts.isMappedTypeNode(n)) { + const constraint = n.typeParameter?.constraint; + if (constraint && constraint.kind === ts.SyntaxKind.StringKeyword) { + how = 'mapped-type'; + return true; + } + return false; + } + if (ts.isTypeReferenceNode(n) && ts.isIdentifier(n.typeName)) { + const name = n.typeName.text; + // The global mapped type, which a local-declaration lookup cannot see. + // This is the objectui#4548 spelling. + if (name === 'Record') { + const key = n.typeArguments?.[0]; + if (key && key.kind === ts.SyntaxKind.StringKeyword) { + how = 'Record< string, … >'; + return true; + } + return false; + } + if (seen.has(name)) return false; + seen.add(name); + const decl = localTypes.get(name); + return decl ? scan(decl) : false; + } + return false; + }; + + const hit = scan(node); + return { hit, how }; +} + +/** Every `forwardRef(...)` whose props type argument this file can resolve. */ +export function collectSitesFrom(file: string, text: string): Site[] { + const sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + + const localTypes = new Map(); + const indexDecls = (n: ts.Node): void => { + if (ts.isInterfaceDeclaration(n) || ts.isTypeAliasDeclaration(n)) localTypes.set(n.name.text, n); + ts.forEachChild(n, indexDecls); + }; + indexDecls(sf); + + const sites: Site[] = []; + const visit = (node: ts.Node): void => { + if (ts.isCallExpression(node)) { + const callee = node.expression; + const isForwardRef = + (ts.isIdentifier(callee) && callee.text === 'forwardRef') || + (ts.isPropertyAccessExpression(callee) && callee.name.text === 'forwardRef'); + if (isForwardRef) { + const typeArg = node.typeArguments?.[1]; + // In scope only when THIS file owns the props contract: an inline type + // literal, a named type declared here, or a bare global mapped type + // (`Record< string, any >`) whose shape is fully readable from source. + // A props type IMPORTED from elsewhere is out of a source scan's reach + // and is covered where it is declared instead. + let ownsProps = false; + if (typeArg) { + ownsProps = + !ts.isTypeReferenceNode(typeArg) || + !ts.isIdentifier(typeArg.typeName) || + localTypes.has(typeArg.typeName.text) || + typeArg.typeName.text === 'Record'; + } + const render = node.arguments[0]; + if (ownsProps && render && (ts.isArrowFunction(render) || ts.isFunctionExpression(render))) { + const r = hasStringIndexSignature(typeArg, localTypes); + sites.push({ + file, + line: sf.getLineAndCharacterOfPosition(node.getStart()).line + 1, + indexSignatureOnTypeArg: r.hit, + how: r.how, + }); + } + } + } + ts.forEachChild(node, visit); + }; + visit(sf); + return sites; +} + +const collectSites = (file: string): Site[] => collectSitesFrom(file, readFileSync(file, 'utf8')); + +const ALL_SITES = readdirSync(packagesRoot, { withFileTypes: true }) + .filter(d => d.isDirectory()) + .flatMap(d => { + const src = path.join(packagesRoot, d.name, 'src'); + try { + if (!statSync(src).isDirectory()) return []; + } catch { + return []; + } + return collectSourceFiles(src).flatMap(collectSites); + }); + +const rel = (s: Site) => `${path.relative(repoRoot, s.file)}:${s.line}`; + +describe('objectui#4548 — no forwardRef props type is erased, repo-wide', () => { + it('finds the population (guards against a broken scan)', () => { + // 219 sites at the time of writing. If this collapses, the walk or the AST + // matcher has gone stale and the guard would silently pass on nothing. + expect(ALL_SITES.length).toBeGreaterThanOrEqual(150); + }); + + it('detects every spelling it is meant to ban (guards against a dead matcher)', () => { + const scan = (src: string): Site[] => + collectSitesFrom(path.join(packagesRoot, '__inmemory__.tsx'), src); + + // 1. objectui#4548's OWN pre-fix shape — the spelling both shipped guards + // report as CLEAN, and the reason this detector exists. + const record = scan( + 'const C = React.forwardRef>' + + '(({ schema, ...props }, _ref) => null);', + ); + expect(record).toHaveLength(1); + expect(record[0].indexSignatureOnTypeArg).toBe(true); + expect(record[0].how).toBe('Record< string, … >'); + + // 2. The classic literal spelling (objectui#4422 / #4528). + const named = scan( + 'interface P { schema: XSchema; [key: string]: any }\n' + + 'const C = React.forwardRef(({ schema, ...props }, ref) => null);', + ); + expect(named).toHaveLength(1); + expect(named[0].indexSignatureOnTypeArg).toBe(true); + expect(named[0].how).toBe('index-signature'); + + // 3. Hidden behind a local alias and an intersection. + const aliased = scan( + 'type Pass = { [key: string]: any };\n' + + 'type P = { schema: XSchema } & Pass;\n' + + 'const C = React.forwardRef(({ schema }, ref) => null);', + ); + expect(aliased[0].indexSignatureOnTypeArg).toBe(true); + + // 4. `Record` behind a local alias — both extensions cooperating. + const aliasedRecord = scan( + 'type P = { schema: XSchema } & Record;\n' + + 'const C = React.forwardRef((props, ref) => null);', + ); + expect(aliasedRecord[0].indexSignatureOnTypeArg).toBe(true); + expect(aliasedRecord[0].how).toBe('Record< string, … >'); + + // 5. A hand-rolled mapped type — what `Record` expands to. The hole must + // not be reopenable by spelling it out. + const mapped = scan( + 'type P = { [K in string]: any };\n' + + 'const C = React.forwardRef((props, ref) => null);', + ); + expect(mapped[0].indexSignatureOnTypeArg).toBe(true); + expect(mapped[0].how).toBe('mapped-type'); + + // 6. The COMPLIANT shape reads as compliant: signature off the type + // argument, on the parameter annotation, so the spread still collects + // arbitrary keys while the declared props survive. + const fixed = scan( + 'interface P { schema: XSchema }\n' + + 'const C = React.forwardRef(' + + '({ schema, ...props }: P & { [key: string]: any }, ref) => null);', + ); + expect(fixed).toHaveLength(1); + expect(fixed[0].indexSignatureOnTypeArg).toBe(false); + + // 7. And so does SchemaRenderer's shipped shape: a clean type argument with + // the open surface stated in the EXPORT ANNOTATION, which this guard + // does not read (see the header — stated contract, not accidental + // eraser). This is the case that would be an allowlist entry if the + // claim were drawn any wider. + const stated = scan( + 'interface P { schema: XSchema }\n' + + 'export const C: ForwardRefExoticComponent

& RefAttributes> =\n' + + ' forwardRef(({ schema, ...props }: P & Record, _ref) => null);', + ); + expect(stated).toHaveLength(1); + expect(stated[0].indexSignatureOnTypeArg).toBe(false); + + // 8. `Record< number, … >` does NOT collapse (`keyof` still excludes the + // string `'ref'`), so it must not be reported. + const numericRecord = scan( + 'const C = React.forwardRef>(({ schema }, ref) => null);', + ); + expect(numericRecord[0].indexSignatureOnTypeArg).toBe(false); + + // 9. Nor does a NUMBER index signature. + const numeric = scan( + 'const C = React.forwardRef(({ schema }, ref) => null);', + ); + expect(numeric[0].indexSignatureOnTypeArg).toBe(false); + + // 10. A props type IMPORTED from another file is out of a source scan's + // reach, and this guard does not pretend otherwise: it reports nothing + // rather than a false verdict. + const imported = scan( + "import type { P } from './Other';\n" + + 'const C = React.forwardRef((props, ref) => null);', + ); + expect(imported).toEqual([]); + }); + + it('no forwardRef carries a string index signature on its props type argument', () => { + // If this fails: read the header. Take the signature OFF the type argument — + // on it, `PropsWithoutRef` collapses the props to the bare index signature, + // erasing every declared property from the render function AND from every + // JSX call site. Do not allowlist. + const offenders = ALL_SITES.filter(s => s.indexSignatureOnTypeArg).map( + s => `${rel(s)} [${s.how}]`, + ); + expect(offenders).toEqual([]); + }); +});