diff --git a/.changeset/properties-wins-both-channels-5123.md b/.changeset/properties-wins-both-channels-5123.md new file mode 100644 index 000000000..15bc9e058 --- /dev/null +++ b/.changeset/properties-wins-both-channels-5123.md @@ -0,0 +1,29 @@ +--- +'@object-ui/react': patch +--- + +A node writing both `properties` and `props` now gets ONE answer per key, and it is the canonical `properties` one — on both read channels. + +`properties` is the spec spelling of a node's config bag and `props` is the +annotated legacy alias, but which one actually reached the screen depended on +how the receiving renderer happened to read it — and the two channels disagreed +in opposite directions: + +- **config bag** (`schema.properties.x`) — the `element:*` family's + `readProps()` merges `{ ...schema.props, ...schema.properties }`, so + `properties` won. +- **React prop** (`x` arriving as a prop) — `SchemaRenderer`'s `createElement` + spread the hoisted `properties.*` values first and then + `...(evaluatedSchema.props || {})` last, which overwrote them, so `props` won. + +Measured on one render of one such node: the bag read `FROM_PROPERTIES` while +the same key read as a React prop gave `FROM_PROPS`. Which value rendered was +decided by nothing an author can see — only by whether their chosen component +belonged to the `readProps()` family. + +The React-prop channel now declines to let the legacy alias override a key the +canonical bag also declares; the config-bag order was already correct and is +unchanged. Scope is co-occurrence only: a key that only `props` declares still +works exactly as before, and a node that writes one spelling is untouched. The +`props` alias is not retired here — only its precedence against a co-present +canonical spelling is settled. diff --git a/packages/components/src/__tests__/alias-precedence-cross-channel.test.tsx b/packages/components/src/__tests__/alias-precedence-cross-channel.test.tsx new file mode 100644 index 000000000..13294852a --- /dev/null +++ b/packages/components/src/__tests__/alias-precedence-cross-channel.test.tsx @@ -0,0 +1,144 @@ +/** + * 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#5123 — one key, one answer, through REAL production renderers. + * + * The companion pin in + * `packages/react/src/__tests__/SchemaRenderer.aliasPrecedenceCrossChannel.test.tsx` + * states the invariant against a probe, because `@object-ui/react` by design + * does not depend on `@object-ui/components`. This file is the other half: the + * same node shape driven through the two renderer FAMILIES the defect actually + * split, with no stand-in for either read path. + * + * - `element:text` is the `readProps()` family — it reads its config out of + * the CONFIG BAG (`{ ...schema.props, ...schema.properties }`). The five + * members on `main` are `elements.tsx`, `text-input.tsx`, + * `record-picker.tsx`, `data-list.tsx`, `metadata-viewer.tsx`; a sixth, + * `plugin-detail/src/renderers/record-alert.tsx`, merges + * `{ ...schema, ...schema.properties }` — a different expression of the + * same `properties`-wins order. + * - `badge` is the SPREAD path — it takes what `createElement` hands it as + * React props (`{ schema, ...props }`) and puts it on the DOM. Everything + * outside the `readProps()` family reads this way. + * + * Before the fix these two families disagreed about the SAME key on the SAME + * node, in opposite directions: the bag read `properties`, the prop read + * `props`. Which one reached the screen depended only on which family the + * author's chosen component happened to belong to. Ruled 2026-08-18: + * `properties` wins on BOTH — the bag order was already right and is untouched. + * + * Requirement (2) of that ruling is why this file exists at all: pinning only + * the channel the fix edited would let the two drift apart again, which is the + * defect CLASS rather than the one spread order. So the assertions below are + * cross-family equalities, not per-file readings. + */ + +import { describe, it, expect } from 'vitest'; +import { render } from '@testing-library/react'; +import { SchemaRenderer } from '@object-ui/react'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there the +// cold transform is billed to `hookTimeout`. See +// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). +import '../renderers'; + +describe('alias precedence across the two renderer families (objectui#5123)', () => { + it('`element:text` (readProps family) renders the canonical `properties` value', () => { + // Unchanged by the fix — this channel was already correct and the ruling + // explicitly keeps it. Pinned so a future "unify the alias" change cannot + // satisfy the invariant by flipping THIS side instead, which would be the + // opposite of what was ruled. + const { container } = render( + + ); + + expect(container.querySelector('.xchan-text')?.textContent).toBe('FROM_PROPERTIES'); + }); + + it('`badge` (spread path) receives the canonical `properties` value as its React prop', () => { + // The half the fix moved. `title` is an ordinary pass-through prop: the + // hoist puts `properties.title` on the node, `badge` spreads whatever it is + // handed onto the DOM. Before the fix the alias spread last and this read + // FROM_PROPS. + const { container } = render( + + ); + + const badge = container.querySelector('[data-obj-type="badge"]'); + expect(badge).toBeTruthy(); + expect(badge?.getAttribute('title')).toBe('FROM_PROPERTIES'); + }); + + it('the two families agree on one node — the invariant, stated as an equality', () => { + // The actual regression guard. Same key, same authored pair of bags, one + // node shape, read through both families; the assertion is that the two + // readings are EQUAL, so re-inverting either side fails here even if the + // per-family expectations above were edited to match a new reading. + const bags = { + props: { content: 'FROM_PROPS', title: 'FROM_PROPS' }, + properties: { content: 'FROM_PROPERTIES', title: 'FROM_PROPERTIES' }, + }; + + const bagChannel = render( + + ); + const bagRead = bagChannel.container.querySelector('.xchan-both')?.textContent; + + const propChannel = render( + + ); + const reactPropRead = propChannel.container + .querySelector('[data-obj-type="badge"]') + ?.getAttribute('title'); + + expect(bagRead).toBe(reactPropRead); + expect(bagRead).toBe('FROM_PROPERTIES'); + }); + + it('a node carrying only the legacy `props` bag still works on both families', () => { + // The blast radius is co-occurrence only. `props` remains a working alias + // wherever `properties` does not also claim the key — the ruling narrowed + // precedence, it did not retire the alias (that question is objectui#4795's + // pending ②, and is NOT decided here). + const text = render( + + ); + expect(text.container.querySelector('.xchan-legacy')?.textContent).toBe('ONLY_PROPS'); + + const badge = render( + + ); + expect( + badge.container.querySelector('[data-obj-type="badge"]')?.getAttribute('title') + ).toBe('ONLY_PROPS'); + }); +}); diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index 690cab0da..0bc7cefc9 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -118,6 +118,93 @@ function resolveAriaProps(schema: Record): Record | undefined, + 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) + ) { + return propsBag; + } + let narrowed: Record | null = null; + for (const key of Object.keys(propertiesBag)) { + if (HOIST_PROTECTED_KEYS.has(key)) continue; + if (!Object.prototype.hasOwnProperty.call(propsBag, key)) continue; + // Copy lazily: the overwhelmingly common node declares one bag or neither, + // and this runs on every render of every node. + if (!narrowed) narrowed = { ...propsBag }; + delete narrowed[key]; + } + return narrowed ?? propsBag; +} + /** * Per-component Error Boundary for SchemaRenderer. * Catches render errors in individual components, preventing one broken @@ -689,7 +776,12 @@ export const SchemaRenderer: ForwardRefExoticComponent< {React.createElement(Component, { schema: schemaForComponent, ...componentProps, // Spread non-metadata schema properties as props - ...(evaluatedSchema.props || {}), // Override with explicit props if provided + // The legacy `props` alias still overrides plain top-level keys, but no + // longer overrides the canonical `properties` bag: for a key BOTH bags + // declare, `properties` wins here exactly as it already wins in + // `readProps()`, so one key has one answer on both channels + // (objectui#5123, maintainer ruling 2026-08-18). + ...propsWithoutCanonicalKeys(evaluatedSchema.props, evaluatedSchema.properties), ...ariaProps, // Inject ARIA attributes from AriaPropsSchema ...debugAttrs, // Debug-mode data attributes disabled: __disabled || undefined, diff --git a/packages/react/src/__tests__/SchemaRenderer.aliasPrecedenceCrossChannel.test.tsx b/packages/react/src/__tests__/SchemaRenderer.aliasPrecedenceCrossChannel.test.tsx new file mode 100644 index 000000000..8ae4b29cb --- /dev/null +++ b/packages/react/src/__tests__/SchemaRenderer.aliasPrecedenceCrossChannel.test.tsx @@ -0,0 +1,259 @@ +/** + * 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#5123 — one key, one answer, whichever channel reads it. + * + * A node may spell its config bag `properties` (the spec spelling) or `props` + * (the annotated legacy alias). When a node writes BOTH, the renderer used to + * return a DIFFERENT value depending on which channel the reading renderer + * happened to use, and the two channels' precedence was exactly OPPOSITE: + * + * | channel | pre-fix winner | site | + * |------------------------------------|----------------|-----------------------------------------------| + * | config bag (`schema.properties.x`) | `properties` | `readProps()` = `{ ...props, ...properties }` | + * | React prop (`x` arriving as prop) | `props` | `createElement`: `...componentProps` then | + * | | | `...(evaluatedSchema.props \|\| {})` last | + * + * Measured on a single render before the fix, with one node carrying both: + * + * bagRead(properties.content)="FROM_PROPERTIES" reactPropRead(content)="FROM_PROPS" + * + * Ruled by the maintainer 2026-08-18: **`properties` wins on BOTH channels.** + * The config-bag order was already right and is untouched; the React-prop + * channel is the one that moved. That restores the declared posture (`props` is + * an annotated legacy alias) rather than changing it. + * + * WHY THIS FILE READS BOTH CHANNELS IN ONE RENDER (the point of the pin): + * a test that only exercised the channel the fix edited would let the two drift + * apart again — which is the actual defect class here, not the individual + * spread order. So every case below asserts the SAME key through BOTH read + * paths of the SAME node in the SAME render, and asserts they agree. + * + * Adjacent but NOT answered here: objectui#4795's pending question ② — whether + * the `properties` envelope is an official `ui:*` authoring channel at all. + * This ruling fixes precedence between two co-present spellings; it takes no + * position on whether either spelling should exist. Do not read these pins as + * blessing the envelope. + */ + +import { describe, it, expect, beforeEach, afterEach } 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'; + +const DATA = { total: 99, label: 'Widgets' }; + +/** + * Mirrors `readProps()` in `@object-ui/components` verbatim + * (`{ ...schema.props, ...schema.properties }`). Duplicated rather than + * imported because `@object-ui/react` by design does not depend on + * `@object-ui/components`; the real production renderers are pinned against the + * same node shape in + * `packages/components/src/__tests__/alias-precedence-cross-channel.test.tsx`, + * so this stand-in is never the only reading. + */ +function readProps(schema: any): Record { + return { ...(schema?.props ?? {}), ...(schema?.properties ?? {}) }; +} + +/** + * The issue's probe: reads BOTH channels of the same node in one render. + * Recorded from an effect, not during render — a render-phase write to a + * module-scope binding is an impurity (`react-hooks/globals`), and RTL's + * `render()` flushes effects inside `act()`, so this is settled by the time any + * assertion reads it. + */ +const seen: { bag: any; reactProps: any; schema: any } = { + bag: undefined, + reactProps: undefined, + schema: undefined, +}; + +const CrossChannelProbe = ({ schema, ...reactProps }: any) => { + const bag = readProps(schema); + React.useEffect(() => { + seen.bag = bag; + seen.reactProps = reactProps; + seen.schema = schema; + }); + return ( +

+ ); +}; + +const renderWithData = (schema: any) => + render( + + + + ); + +/** Both channels of one node, as the issue's probe printed them. */ +const readBothChannels = (schema: any) => { + const { container } = renderWithData(schema); + const el = container.querySelector('[data-testid="probe"]')!; + return { + bagRead: el.getAttribute('data-bag-read'), + reactPropRead: el.getAttribute('data-react-prop-read'), + }; +}; + +describe('alias precedence is the same on both read channels (objectui#5123)', () => { + beforeEach(() => { + seen.bag = undefined; + seen.reactProps = undefined; + seen.schema = undefined; + ComponentRegistry.register('xchan', CrossChannelProbe, { + namespace: 'element', + skipFallback: true, + }); + }); + + afterEach(() => { + ComponentRegistry.unregister?.('xchan', 'element'); + }); + + // ------------------------------------------------------------------------- + // (a) the invariant itself — the whole point of the card + // ------------------------------------------------------------------------- + describe('a node carrying BOTH spellings', () => { + it('gives ONE answer for the key, and it is the canonical `properties` one', () => { + const { bagRead, reactPropRead } = readBothChannels({ + type: 'element:xchan', + props: { content: 'FROM_PROPS' }, + properties: { content: 'FROM_PROPERTIES' }, + }); + + // The invariant, stated as the equality that must never break again. + // Before the fix this line read FROM_PROPERTIES !== FROM_PROPS. + expect(bagRead).toBe(reactPropRead); + // ...and the ruling says WHICH answer the single answer is. + expect(bagRead).toBe('FROM_PROPERTIES'); + expect(reactPropRead).toBe('FROM_PROPERTIES'); + }); + + it('keeps the losing `props` bag intact on the schema (it is shadowed, not deleted)', () => { + readBothChannels({ + type: 'element:xchan', + props: { content: 'FROM_PROPS' }, + properties: { content: 'FROM_PROPERTIES' }, + }); + + // The fix shadows the legacy value on the way out to React; it must not + // mutate the node. A renderer that deliberately inspects `schema.props` + // (or a future migration that wants to report the alias) still sees it, + // and `readProps()`'s own merge stays meaningful rather than becoming a + // merge of a bag with itself. + expect(seen.schema.props).toEqual({ content: 'FROM_PROPS' }); + expect(seen.schema.properties).toEqual({ content: 'FROM_PROPERTIES' }); + }); + + it('resolves per key: a key only `props` declares still arrives', () => { + const { container } = renderWithData({ + type: 'element:xchan', + props: { content: 'FROM_PROPS', onlyInProps: 'KEPT' }, + properties: { content: 'FROM_PROPERTIES' }, + }); + expect(container.querySelector('[data-testid="probe"]')).toBeTruthy(); + + // Overlapping key -> canonical wins on both channels. + expect(seen.bag.content).toBe('FROM_PROPERTIES'); + expect(seen.reactProps.content).toBe('FROM_PROPERTIES'); + // Non-overlapping key -> the alias is untouched on both channels. The fix + // narrows the alias per KEY, it does not switch the alias off. + expect(seen.bag.onlyInProps).toBe('KEPT'); + expect(seen.reactProps.onlyInProps).toBe('KEPT'); + }); + + it('holds for expression values too — both bags evaluate, then canonical wins', () => { + const { bagRead, reactPropRead } = readBothChannels({ + type: 'element:xchan', + props: { content: '${data.label}' }, + properties: { content: '${data.total}' }, + }); + + // Not the raw source on either side (objectui#4799 evaluates both bags), + // and the same evaluated value through both channels. + expect(bagRead).toBe(reactPropRead); + expect(bagRead).toBe('99'); + // Both bags are still evaluated in place — the loser is shadowed, not + // skipped, so nothing that reads `schema.props` gets raw `${…}` back. + expect(seen.schema.props.content).toBe('Widgets'); + expect(seen.schema.properties.content).toBe(99); + }); + }); + + // ------------------------------------------------------------------------- + // (b) single-spelling nodes are untouched — the blast radius is co-occurrence + // only. These are the readings that must NOT move. + // ------------------------------------------------------------------------- + describe('a node carrying only one spelling is unaffected', () => { + it('`properties` only: both channels read it (unchanged)', () => { + const { bagRead, reactPropRead } = readBothChannels({ + type: 'element:xchan', + properties: { content: 'ONLY_PROPERTIES' }, + }); + expect(bagRead).toBe('ONLY_PROPERTIES'); + expect(reactPropRead).toBe('ONLY_PROPERTIES'); + }); + + it('`props` only: both channels read it (unchanged — the alias keeps working)', () => { + const { bagRead, reactPropRead } = readBothChannels({ + type: 'element:xchan', + props: { content: 'ONLY_PROPS' }, + }); + expect(bagRead).toBe('ONLY_PROPS'); + expect(reactPropRead).toBe('ONLY_PROPS'); + }); + + it('a degenerate (non-object) `properties` leaves the alias spread alone', () => { + // The hoist and `readProps()` both treat a non-object `properties` by + // object-spreading it; there is no canonical bag to prefer, so the fix + // declines to reinterpret it and today's reading stands. Pinned so the + // narrowing is a decision on the record rather than an accident. + const { reactPropRead } = readBothChannels({ + type: 'element:xchan', + props: { content: 'FROM_PROPS' }, + properties: 'not-a-bag', + }); + expect(reactPropRead).toBe('FROM_PROPS'); + }); + }); + + // ------------------------------------------------------------------------- + // (c) the carve-out the hoist already owns + // ------------------------------------------------------------------------- + describe('the outer component descriptor still wins over an inner `type`/`id`', () => { + it('does not let `properties.id` shadow the alias, because the hoist never copies it', () => { + // `type`/`id` are the two keys the hoist deliberately refuses to copy up + // (they identify WHICH renderer to dispatch to; an inner `type` is a + // renderer-specific prop such as a tab's visual style). Since the hoist + // never puts `properties.id` on the node, there is no canonical value on + // this channel to win with — dropping the alias here would delete the + // prop rather than replace it. So these two keys keep today's reading, + // and the narrowing skips them. + readBothChannels({ + type: 'element:xchan', + id: 'OUTER', + props: { id: 'FROM_PROPS' }, + properties: { id: 'INNER', content: 'c' }, + }); + expect(seen.reactProps.id).toBe('FROM_PROPS'); + // The outer descriptor is intact — that is what the hoist protects. + expect(seen.schema.type).toBe('element:xchan'); + expect(seen.schema.id).toBe('OUTER'); + }); + }); +});