diff --git a/.changeset/6752-degenerate-props-bag-guard.md b/.changeset/6752-degenerate-props-bag-guard.md new file mode 100644 index 000000000..05594d405 --- /dev/null +++ b/.changeset/6752-degenerate-props-bag-guard.md @@ -0,0 +1,33 @@ +--- +'@object-ui/react': patch +--- + +`SchemaRenderer`: a non-object `props` bag is no longer object-spread into +indexed React props (objectui#6752). + +A node written `{ type: 'card', props: 'not-a-bag' }` reached `createElement` +carrying nine React props named `0` through `8`, one per character, because +`{ ...'not-a-bag' }` enumerates a string's character indices. Nothing threw and +nothing was logged, so the symptom — a component handed nine props it never +declared — sat a long way from the `props` value that caused it. Measured +through the real `SchemaRenderer`; the in-repo corpus has no such node today, so +this is a latent shape rather than a live failure. + +The canonical `properties` branch already carried the wider guard. Its comment +claimed the guard was hoist-specific, and that turned out not to survive +measurement: ablating it leaves the indexed keys the hoist puts on the node +completely unchanged, and moves only whether `schema.properties` still holds the +value the author wrote. The reason is channel-independent — a degenerate value +must not have its shape reinterpreted by an object spread — so `props` now +carries the same guard, both bags share one `isConfigBag` predicate, and the +`properties` comment states the measured reason instead of the old one. + +Both sites that spread the bag are covered: the evaluation memo (so +`schema.props` keeps the authored value) and `propsWithoutCanonicalKeys` (so the +`createElement` spread does not re-enumerate it). A degenerate bag now +contributes no keys, and the authored value is passed through unmangled on the +React prop named `props`. + +Unchanged: a normal object `props` (still evaluated, still spread per key), +objectui#5123's two-bag precedence, the `properties` hoist, and objectui#6708's +dropped-`props` diagnostic. diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 1e48de087..b3bdabd0c 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -139,6 +139,27 @@ function resolveAriaProps(schema: Record): Record { + return !!value && typeof value === 'object' && !Array.isArray(value); +} + /** * The legacy `props` bag, minus every key the canonical `properties` bag also * declares (objectui#5123). @@ -191,19 +212,24 @@ const HOIST_PROTECTED_KEYS = new Set(['type', 'id']); * position on whether either should exist. */ function propsWithoutCanonicalKeys( - propsBag: Record | undefined, + propsBag: unknown, propertiesBag: unknown ): Record { - if (!propsBag) return {}; - // Only a real object bag can win a key. `typeof null === 'object'` is covered - // by the truthiness check; arrays are excluded for the same reason the - // evaluation guard excludes them — a degenerate `properties` must not have - // its shape reinterpreted here. - if ( - !propertiesBag || - typeof propertiesBag !== 'object' || - Array.isArray(propertiesBag) - ) { + // A degenerate `props` contributes NO keys (objectui#6752). This used to be a + // bare `if (!propsBag)`, so a non-object bag was returned as-is and the + // `createElement` spread below re-enumerated it: measured on `b76ca6764`, + // `props: 'not-a-bag'` reached the element as nine React props named `0` … `8`. + // Returning `{}` also makes the declared return type true — the old signature + // said `Record` while this line could hand back a string. + // + // This does NOT move objectui#5123's precedence. That rule only decides which + // of two co-present values a key carries, and a degenerate bag declares no + // key for either bag to win: the indices were never authored, they are the + // object spread's reading of a string. + if (!isConfigBag(propsBag)) return {}; + // Only a real object bag can win a key — the same predicate, for the reason + // stated there: a degenerate bag must not have its shape reinterpreted here. + if (!isConfigBag(propertiesBag)) { return propsBag; } let narrowed: Record | null = null; @@ -892,19 +918,29 @@ export const SchemaRenderer: ForwardRefExoticComponent< // source today). Deepening that is a separate decision and would have to be // taken for both spellings at once — not smuggled in on one side here. // - // The guard is wider than the `props` branch's bare truthiness on purpose: - // this value FEEDS the hoist, so re-shaping a degenerate `properties` - // (a string, an array) via the object spread would propagate. Non-objects - // skip evaluation and reach the hoist exactly as they do today. + // Guarded by {@link isConfigBag}: a degenerate value must not have its shape + // reinterpreted by an object spread. Non-objects skip evaluation and reach + // the hoist exactly as they do today. + // + // ⚠️ This guard used to say it was wider than the `props` branch BECAUSE + // this value feeds the hoist. That reason does not survive measurement + // (objectui#6752). Ablating this guard to bare truthiness and re-rendering + // `{ type, properties: 'not-a-bag' }` on `b76ca6764` leaves the indexed + // keys the hoist puts on the node completely UNCHANGED — `0` … `8` either + // way, because the hoist's own `Object.entries` walk enumerates a string's + // character indices whatever this line did — and moves exactly one thing: + // whether `schema.properties` still holds the value the author wrote + // (`'not-a-bag'` guarded, `{ '0': 'n', … }` ablated). So what this buys is + // the AUTHORED value's shape, which is channel-independent, and the `props` + // branch below now carries the same guard for the same reason. // Snapshotted BEFORE evaluation — objectui#5756's diagnostic below reads // the RAW (pre-collapse) text from this reference, since `newSchema.properties` // is about to be replaced with the evaluated copy. - const rawPropertiesBag: Record | undefined = - newSchema.properties && - typeof newSchema.properties === 'object' && - !Array.isArray(newSchema.properties) - ? (newSchema.properties as Record) - : undefined; + const rawPropertiesBag: Record | undefined = isConfigBag( + newSchema.properties + ) + ? newSchema.properties + : undefined; if (rawPropertiesBag) { const newProperties: Record = { ...rawPropertiesBag }; @@ -989,8 +1025,28 @@ export const SchemaRenderer: ForwardRefExoticComponent< newSchema.content = evaluator.evaluate(newSchema.content); } - // Evaluate 'props' - if (newSchema.props) { + // Evaluate 'props' — the legacy alias of the config bag. + // + // The guard MIRRORS the `properties` branch above rather than testing bare + // truthiness (objectui#6752). Same predicate, both channels: a degenerate + // bag must not have its shape reinterpreted by an object spread. + // + // Measured on `b76ca6764` through the real `SchemaRenderer`, before this + // guard existed: a node written `{ type: 'card', props: 'not-a-bag' }` + // reached `createElement` carrying NINE React props named `0` … `8`, one + // per character, because `{ ...'not-a-bag' }` enumerates a string's + // character indices. Nothing threw and nothing was logged, so the symptom + // (a component handed nine props it never declared) sits a long way from + // the `props` value that caused it. + // + // This guard is HALF the fix and does not on its own remove those props — + // also measured, on the same base with only this line widened: the bag + // reached `propsWithoutCanonicalKeys` as the authored string, was returned + // unchanged, and `{ ...outgoingPropsBag }` at the `createElement` call + // re-enumerated it. The other half is that function's own guard. What this + // line buys is that `schema.props` keeps the value the author wrote instead + // of the spread's reading of it. + if (isConfigBag(newSchema.props)) { const newProps = { ...newSchema.props }; for (const [key, val] of Object.entries(newProps)) { newProps[key] = evaluator.evaluate(val as any); diff --git a/packages/react/src/__tests__/SchemaRenderer.degeneratePropsBag.test.tsx b/packages/react/src/__tests__/SchemaRenderer.degeneratePropsBag.test.tsx new file mode 100644 index 000000000..59f736f62 --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.degeneratePropsBag.test.tsx @@ -0,0 +1,258 @@ +/** + * 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#6752 — a non-object `props` was object-spread into indexed React + * props: `props: 'not-a-bag'` reached the element as `0`, `1`, `2`, … + * + * ## Why this card landed as a guard rather than as a comment + * + * Triage dispatched it measurement-first: read WHY the sibling `properties` + * branch carries a wider guard, then let the reason pick the arm. If the reason + * is channel-independent the missing `props` guard is an oversight (widen it); + * if it is hoist-specific the asymmetry is intended (document it instead). + * + * The reason was measured, not read off the comment — and the comment turned + * out to be wrong about its own guard. On `b76ca6764`, ablating the + * `properties` guard to bare truthiness and re-rendering + * `{ type, properties: 'not-a-bag' }` left the indexed keys the hoist puts on + * the node COMPLETELY UNCHANGED (`0` … `8` either way): the hoist's own + * `Object.entries` walk enumerates a string's character indices whatever that + * guard did. Exactly one thing moved — whether `schema.properties` still holds + * the value the author wrote (`'not-a-bag'` guarded, `{ '0': 'n', … }` + * ablated). So the guard buys the AUTHORED value's shape, which is + * channel-independent, and the repo already states it that way at a second, + * non-hoist site: `propsWithoutCanonicalKeys` excludes a degenerate bag + * "for the same reason the evaluation guard excludes them — a degenerate + * `properties` must not have its shape reinterpreted here." + * + * ⇒ arm 1. The `props` branch was missing the same guard for the same reason. + * + * ## Why the fix has two halves + * + * Measured, not assumed. With ONLY the evaluation memo's guard widened, the + * nine indexed props came BACK: the bag reached `propsWithoutCanonicalKeys` as + * the authored string, was returned unchanged, and `{ ...outgoingPropsBag }` at + * the `createElement` call re-enumerated it. Both sites object-spread the bag, + * so both needed the predicate. + * + * ## `BASE_READING` + * + * Captured on `b76ca6764` — this branch's base, with `SchemaRenderer.tsx` at its + * committed blob `1e48de087` and no part of this card in the tree — by + * rendering exactly the nodes below through the real `SchemaRenderer`, and + * pasted verbatim. It is the pre-fix tree, so the "unchanged" legs are a real + * before/after comparison rather than a snapshot this file wrote for itself, + * and the degenerate legs pin a shape that was MEASURED to be there before the + * guard removed it. A pin that never went red is not evidence. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render } from '@testing-library/react'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; +import { SchemaRenderer } from '../SchemaRenderer'; +import { SchemaRendererContext } from '../context/SchemaRendererContext'; +import { + DROPPED_PROPS_BAG_PREFIX, + __resetDroppedPropsBagWarnings, +} from '../utils/propsBagDiagnostic'; + +/** The provider really does hold the path the object-bag case spells. */ +const DATA = { customers: ['ada', 'grace'] }; + +const renderWithData = (schema: unknown) => + render( + + + , + ); + +const snap = (v: unknown) => JSON.parse(JSON.stringify(v ?? null)); + +interface Reading { + propKeys: string[]; + indexedPropKeys: string[]; + propsReactProp: unknown; + propertiesReactProp: unknown; + schemaProps: unknown; + schemaProperties: unknown; +} + +const captured: Record = {}; + +/** + * Records BOTH channels: the React props the element actually receives, and + * what a `({ schema })` renderer reads. `indexedPropKeys` is the card's whole + * subject, isolated so a failure names it instead of burying it in a diff of + * every prop. + */ +const makeProbe = (label: string) => { + const Probe: React.FC & { schema?: Record }> = ({ + schema, + ...rest + }) => { + captured[label] = { + propKeys: Object.keys(rest).sort(), + indexedPropKeys: Object.keys(rest) + .filter(k => /^\d+$/.test(k)) + .sort(), + propsReactProp: snap(rest.props ?? null), + propertiesReactProp: snap(rest.properties ?? null), + schemaProps: snap(schema?.props ?? null), + schemaProperties: snap(schema?.properties ?? null), + }; + return
; + }; + return Probe; +}; + +/** One node per envelope shape the card names. `not-a-bag` is nine characters. */ +const CASES: ReadonlyArray = [ + ['propsString', { type: 'test:deg', id: 'a', props: 'not-a-bag' }], + ['propertiesString', { type: 'test:deg', id: 'b', properties: 'not-a-bag' }], + ['propsArray', { type: 'test:deg', id: 'c', props: ['x', 'y'] }], + ['propsNumber', { type: 'test:deg', id: 'd', props: 42 }], + ['propsTrue', { type: 'test:deg', id: 'e', props: true }], + ['propsObject', { type: 'test:deg', id: 'f', props: { title: 'T', data: '${data.customers}' } }], + ['propertiesArray', { type: 'test:deg', id: 'g', properties: ['x', 'y'] }], +]; + +const BASE_READING: Record = JSON.parse(`{ + "propsString": {"propKeys":["0","1","2","3","4","5","6","7","8","className","data-obj-id","data-obj-type","disabled","id","props"],"indexedPropKeys":["0","1","2","3","4","5","6","7","8"],"propsReactProp":{"0":"n","1":"o","2":"t","3":"-","4":"a","5":"-","6":"b","7":"a","8":"g"},"propertiesReactProp":null,"schemaProps":{"0":"n","1":"o","2":"t","3":"-","4":"a","5":"-","6":"b","7":"a","8":"g"},"schemaProperties":null}, + "propertiesString": {"propKeys":["0","1","2","3","4","5","6","7","8","className","data-obj-id","data-obj-type","disabled","id","properties"],"indexedPropKeys":["0","1","2","3","4","5","6","7","8"],"propsReactProp":null,"propertiesReactProp":"not-a-bag","schemaProps":null,"schemaProperties":"not-a-bag"}, + "propsArray": {"propKeys":["0","1","className","data-obj-id","data-obj-type","disabled","id","props"],"indexedPropKeys":["0","1"],"propsReactProp":{"0":"x","1":"y"},"propertiesReactProp":null,"schemaProps":{"0":"x","1":"y"},"schemaProperties":null}, + "propsNumber": {"propKeys":["className","data-obj-id","data-obj-type","disabled","id","props"],"indexedPropKeys":[],"propsReactProp":{},"propertiesReactProp":null,"schemaProps":{},"schemaProperties":null}, + "propsTrue": {"propKeys":["className","data-obj-id","data-obj-type","disabled","id","props"],"indexedPropKeys":[],"propsReactProp":{},"propertiesReactProp":null,"schemaProps":{},"schemaProperties":null}, + "propsObject": {"propKeys":["className","data","data-obj-id","data-obj-type","disabled","id","props","title"],"indexedPropKeys":[],"propsReactProp":{"title":"T","data":["ada","grace"]},"propertiesReactProp":null,"schemaProps":{"title":"T","data":["ada","grace"]},"schemaProperties":null}, + "propertiesArray": {"propKeys":["0","1","className","data-obj-id","data-obj-type","disabled","id","properties"],"indexedPropKeys":["0","1"],"propsReactProp":null,"propertiesReactProp":["x","y"],"schemaProps":null,"schemaProperties":["x","y"]} +}`); + +describe('objectui#6752 — a degenerate `props` bag is no longer spread into indexed React props', () => { + beforeEach(() => { + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + __resetDroppedPropsBagWarnings(); + for (const key of Object.keys(captured)) delete captured[key]; + for (const [label, schema] of CASES) { + ComponentRegistry.register('deg', makeProbe(label), { + namespace: 'test', + skipFallback: true, + }); + const { unmount } = renderWithData(schema); + unmount(); + } + }); + + afterEach(() => { + vi.restoreAllMocks(); + ComponentRegistry.unregister?.('deg', 'test'); + }); + + /* ---------------------------------------------------------------- * + * 1 — the defect, and that it was really there + * ---------------------------------------------------------------- */ + + it('pins the pre-fix shape the card reported: nine React props named `0`…`8`', () => { + // Not an assertion about today's tree — it states what BASE_READING holds, + // so a future edit that quietly rewrites this constant to match a changed + // renderer has to lie in public rather than in passing. + expect(BASE_READING.propsString.indexedPropKeys).toEqual([ + '0', '1', '2', '3', '4', '5', '6', '7', '8', + ]); + expect(BASE_READING.propsString.propsReactProp).toEqual({ + 0: 'n', 1: 'o', 2: 't', 3: '-', 4: 'a', 5: '-', 6: 'b', 7: 'a', 8: 'g', + }); + }); + + it('a string `props` now reaches the element with NO indexed props', () => { + expect(captured.propsString.indexedPropKeys).toEqual([]); + // The exact shape that must not come back, named rather than implied. + for (const k of ['0', '1', '2', '3', '4', '5', '6', '7', '8']) { + expect(captured.propsString.propKeys).not.toContain(k); + } + }); + + it('an array, a number and a boolean `props` contribute no keys either', () => { + expect(captured.propsArray.indexedPropKeys).toEqual([]); + expect(captured.propsNumber.indexedPropKeys).toEqual([]); + expect(captured.propsTrue.indexedPropKeys).toEqual([]); + }); + + it('the authored value survives instead of the spread’s reading of it', () => { + // `props` is passed through as a React prop of that name (unchanged by this + // card); what changed is that it now carries what the author wrote. + expect(captured.propsString.propsReactProp).toBe('not-a-bag'); + expect(captured.propsArray.propsReactProp).toEqual(['x', 'y']); + expect(captured.propsNumber.propsReactProp).toBe(42); + expect(captured.propsTrue.propsReactProp).toBe(true); + // …and a `({ schema })` renderer reads the same authored value. + expect(captured.propsString.schemaProps).toBe('not-a-bag'); + }); + + /* ---------------------------------------------------------------- * + * 2 — what must NOT move (this is what catches an over-broad guard) + * ---------------------------------------------------------------- */ + + it('a normal object `props` is byte-for-byte what it was before this card', () => { + expect(captured.propsObject).toEqual(BASE_READING.propsObject); + // Spelled out, because this is the leg that fails if the guard is too wide: + // the bag is still evaluated (`${data.customers}` collapsed) and still + // spread as individual React props. + expect(captured.propsObject.propKeys).toContain('title'); + expect(captured.propsObject.propKeys).toContain('data'); + expect(captured.propsObject.propsReactProp).toEqual({ + title: 'T', + data: ['ada', 'grace'], + }); + }); + + it('the `properties` channel is untouched — this card changed one bag', () => { + expect(captured.propertiesString).toEqual(BASE_READING.propertiesString); + expect(captured.propertiesArray).toEqual(BASE_READING.propertiesArray); + // Recorded, not fixed: the hoist still enumerates a degenerate `properties` + // into `0`…`8`. That is the hoist's own `Object.entries` walk, out of this + // card's scope (which forbids changing hoist behaviour) and filed + // separately. Pinned so the asymmetry is visible rather than forgotten. + expect(captured.propertiesString.indexedPropKeys).toEqual([ + '0', '1', '2', '3', '4', '5', '6', '7', '8', + ]); + }); + + /* ---------------------------------------------------------------- * + * 3 — objectui#6708's diagnostic is undisturbed + * ---------------------------------------------------------------- */ + + describe('objectui#6708’s dropped-`props` diagnostic still behaves exactly as it did', () => { + const warnings = (): string[] => + ((console.warn as unknown as { mock?: { calls: unknown[][] } }).mock?.calls ?? []) + .map(c => String(c[0])) + .filter(m => m.startsWith(DROPPED_PROPS_BAG_PREFIX)); + + it('stays silent on a degenerate bag — it reads the AUTHORED bag', () => { + // It was already silent here before this card (it reads `schema.props`, + // not the rebuilt one). It must still be silent, and for the same reason: + // a string is not a config bag, so there is no key to point at. + vi.mocked(console.warn).mockClear(); + __resetDroppedPropsBagWarnings(); + const { unmount } = renderWithData({ type: 'test:deg', id: 'z1', props: 'not-a-bag' }); + expect(warnings()).toEqual([]); + unmount(); + }); + + it('still fires on a real bag a `schema`-reading renderer would drop', () => { + vi.mocked(console.warn).mockClear(); + __resetDroppedPropsBagWarnings(); + const { unmount } = renderWithData({ type: 'test:deg', id: 'z2', props: { title: 'T' } }); + const lines = warnings(); + expect(lines).toHaveLength(1); + expect(lines[0]).toContain('`title`'); + unmount(); + }); + }); +});