diff --git a/.changeset/6761-one-config-bag-predicate.md b/.changeset/6761-one-config-bag-predicate.md new file mode 100644 index 000000000..93e3b24c1 --- /dev/null +++ b/.changeset/6761-one-config-bag-predicate.md @@ -0,0 +1,22 @@ +--- +--- + +Internal refactor only; no published behaviour changes. "Is this value a real config bag +(an object, not an array, not null)?" was asked in `packages/react` in six places in four +spellings — `SchemaRenderer`'s module-private `isConfigBag`, its `winningVisibilityKey` +inline, `utils/propsBagDiagnostic.ts`'s private twin, and TWO in +`utils/unevaluatedExpression.ts` (`scanBag`'s negated early return and the `hoisted` +dedupe read). All six now read one exported predicate, `utils/configBag.ts` +(objectui#6761). + +They agreed, which was never the cost: a copy that drifts produces no error, only a +different answer on one channel — the failure mode `hasDeclaredPredicate` was created to +end for "is a gate DECLARED?" (objectui#3850). So the convergence ships with a pin, +`utils/configBag.pin.test.ts`, that fails when a seventh spelling appears anywhere in +`packages/react/src`. The two sites that ask the same SHAPE question about a data ROW +(`SchemaRenderer`'s `boundRecord`, `usePredicateRecordContext`) are deliberately NOT +merged and are named in the pin's allowlist with that reason: their "no row → bind +NOTHING" rule is a different ruling, and it must not move if "config bag" ever narrows. + +Each of the six sites was ablated individually to bare truthiness and measured, rather +than assumed equivalent because the lines look alike. diff --git a/packages/react/src/SchemaRenderer.tsx b/packages/react/src/SchemaRenderer.tsx index b3bdabd0c..e95eb4372 100644 --- a/packages/react/src/SchemaRenderer.tsx +++ b/packages/react/src/SchemaRenderer.tsx @@ -29,6 +29,7 @@ import { useRecordContext } from './context/RecordContext.js'; import { usePredicateScope } from './hooks/useExpression.js'; import { usePageVariables } from './hooks/usePageVariables.js'; import { resolveKeyedI18nLabel } from './utils/i18n.js'; +import { isConfigBag } from './utils/configBag.js'; import { reportUnevaluatedExpressions } from './utils/unevaluatedExpression.js'; import { reportDroppedPropsBag } from './utils/propsBagDiagnostic.js'; import { @@ -139,27 +140,6 @@ 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). @@ -356,8 +336,7 @@ const visibilityGateKind = (key: VisibilityChainKey): PredicateGateKind => */ function winningVisibilityKey(node: Record): VisibilityChainKey | undefined { const propertiesBag = node.properties; - const hasPropertiesBag = - propertiesBag != null && typeof propertiesBag === 'object' && !Array.isArray(propertiesBag); + const hasPropertiesBag = isConfigBag(propertiesBag); const effective = (key: string): unknown => hasPropertiesBag && Object.prototype.hasOwnProperty.call(propertiesBag, key) ? (propertiesBag as Record)[key] diff --git a/packages/react/src/utils/configBag.pin.test.ts b/packages/react/src/utils/configBag.pin.test.ts new file mode 100644 index 000000000..cb4c115f2 --- /dev/null +++ b/packages/react/src/utils/configBag.pin.test.ts @@ -0,0 +1,128 @@ +/** + * 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#6761 — "is this a real config bag?" is asked in ONE place. + * + * ## Why a pin, and why this one is the point of the card + * + * The six occurrences this replaced all AGREED. Convergence alone therefore + * fixes today and not tomorrow: nothing stops a seventh from being written + * next week, and nothing would report it if it drifted — each spelling is a + * boolean expression, so a disagreement produces no error, only a different + * answer on one channel. `hasDeclaredPredicate` (`@object-ui/core`) is the + * repo's precedent for both halves of that lesson: three spellings with three + * different scopes, ended by one definition (objectui#3850). + * + * ## What it measures + * + * Every production source in `packages/react/src` (tests excluded — a test + * asserting on shapes is not a runtime answer that can drift on a channel), + * scanned for the `typeof x === 'object'` + `Array.isArray(x)` conjunction on + * the SAME operand, in either order and either polarity. That is the family + * every spelling of this question has been written in here — four of them + * across six occurrences, measured on `b98352a15`. A genuinely novel + * construction (`Object.prototype.toString.call`, a `constructor` test) would + * evade it; the job is to make the CHEAP path fail — copying a line that + * already exists — not to be a theorem about object-ness. + * + * ## The allowlist is a decision, not an escape hatch + * + * Two entries survive, and they are the SHAPE question asked about a data ROW + * rather than a config bag: "did a row bind?", whose answer carries its own + * pinned meaning (binding NOTHING rather than an empty row is what keeps a + * host-supplied `record` from being shadowed — `usePredicateRecordContext`). + * If "config bag" ever narrows, those two must not follow. Adding an entry + * here is how you say a new site asks a different question; importing + * {@link isConfigBag} is how you say it asks this one. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +const SRC_ROOT = path.resolve(here, '..'); + +/** Every production `.ts`/`.tsx` under `packages/react/src`. */ +function collectSources(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + collectSources(full, out); + continue; + } + if (!/\.tsx?$/.test(entry.name)) continue; + if (/\.test\.tsx?$/.test(entry.name)) continue; + out.push(full); + } + return out; +} + +/** + * The conjunction, on one operand, in either operand order. `[\s\S]*?` inside + * a bounded window rather than `\s*` so a multi-line `&&` chain with a comment + * between the halves still matches. + */ +const SPELLINGS: RegExp[] = [ + /typeof\s+([A-Za-z_$][\w$.]*)\s*[!=]==\s*['"]object['"]\s*(?:&&|\|\|)\s*!?\s*Array\.isArray\(\s*\1\s*\)/g, + /!?\s*Array\.isArray\(\s*([A-Za-z_$][\w$.]*)\s*\)\s*(?:&&|\|\|)\s*typeof\s+\1\s*[!=]==\s*['"]object['"]/g, +]; + +/** `: ` */ +function findSpellings(): string[] { + const found: string[] = []; + for (const file of collectSources(SRC_ROOT).sort()) { + const source = readFileSync(file, 'utf8'); + for (const pattern of SPELLINGS) { + pattern.lastIndex = 0; + let match: RegExpExecArray | null; + while ((match = pattern.exec(source)) !== null) { + const rel = path.relative(SRC_ROOT, file).split(path.sep).join('/'); + found.push(`${rel}: ${match[0].replace(/\s+/g, ' ')}`); + } + } + } + return found.sort(); +} + +/** + * The definition, plus the two ROW sites. Every entry names the question it + * answers; a new line here without one is the drift this file exists to stop. + */ +const ALLOWED = [ + // THE definition. Everything asking "is this a config bag?" reads this. + "utils/configBag.ts: typeof value === 'object' && !Array.isArray(value)", + // A data ROW, not a config bag: whether the node's bound record is a row to + // put in the evaluator scope at all (binding `{}` would SHADOW a + // host-supplied `record`). + "SchemaRenderer.tsx: typeof boundRecord === 'object' && !Array.isArray(boundRecord)", + // The same row question, one layer down — `usePredicateRecordContext`'s + // "no row → bind NOTHING" rule. + "hooks/useExpression.ts: typeof record !== 'object' || Array.isArray(record)", +].sort(); + +describe('objectui#6761 — one config-bag predicate', () => { + it('has no spelling outside the definition and the two row sites', () => { + expect(findSpellings()).toEqual(ALLOWED); + }); + + it('is read by every module that asks the question', () => { + for (const rel of [ + 'SchemaRenderer.tsx', + 'utils/propsBagDiagnostic.ts', + 'utils/unevaluatedExpression.ts', + ]) { + const source = readFileSync(path.resolve(SRC_ROOT, rel), 'utf8'); + expect(source, `${rel} no longer imports the shared predicate`).toMatch( + /import \{ isConfigBag \} from '\.(?:\/utils)?\/configBag\.js';/ + ); + } + }); +}); diff --git a/packages/react/src/utils/configBag.ts b/packages/react/src/utils/configBag.ts new file mode 100644 index 000000000..be08ebde7 --- /dev/null +++ b/packages/react/src/utils/configBag.ts @@ -0,0 +1,71 @@ +/** + * 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. + */ + +/** + * Is this value a real config bag — an object, and not an array pretending to + * be one? (objectui#6761) + * + * This is the ONE definition of that question in `@object-ui/react`, and it + * lives here rather than in whichever module happened to need it first, + * because every module that asks it is a CONSUMER of the same authored shape: + * the `properties` / `props` bags a node may carry. `typeof null === 'object'` + * is covered by the truthiness test. + * + * ## Why one definition, when all the copies agreed + * + * They agreed; that was never the cost. The cost is that disagreement between + * them produces NO error — each copy is a boolean expression, and a copy that + * drifts answers a different question on ONE channel while every other channel + * keeps the old answer. That is the same failure mode `hasDeclaredPredicate` + * (`@object-ui/core`) was created to end for "is a gate DECLARED?", which had + * three spellings with three different SCOPES before objectui#3850 — + * `disabled: null` greying a control out on one path and not another + * (objectui#3862). + * + * ## The spellings this replaces, measured on `b98352a15` + * + * Six occurrences, four spellings, all in `packages/react/src`: + * + * 1. `SchemaRenderer`'s module-private `isConfigBag` — itself already the + * merge of the `properties` evaluation guard and `propsWithoutCanonicalKeys` + * (objectui#6752, which unified those two because it had to touch both, + * and filed the rest as objectui#6761 rather than smuggling it in); + * 2. `SchemaRenderer`'s `winningVisibilityKey`, inline, spelled with + * `!= null` where the others spell truthiness; + * 3. `utils/propsBagDiagnostic.ts`'s module-private twin (objectui#6708); + * 4. `utils/unevaluatedExpression.ts`'s `scanBag` early return, spelled + * NEGATED (objectui#4795); + * 5. `utils/unevaluatedExpression.ts`'s `hoisted`, in the same file as (4) + * and spelled positively — this one is not in objectui#6761's inventory + * of five, which counted the file once; six occurrences is the count + * measured on this branch's base, and it was six on the card's base + * (`b76ca6764`) too. + * + * `!= null` (2) and truthiness (1, 3, 5) cannot disagree HERE, and the + * difference is worth naming rather than smoothing over: they part company + * only on a falsy value, and every falsy value except `document.all` fails + * `typeof === 'object'` in the very next conjunct. The texts differ; the + * answers cannot. + * + * ## What this is NOT + * + * Not a general "is this a plain object?" utility, and deliberately not shared + * with the two sites that ask the same SHAPE question about a data ROW — + * `SchemaRenderer`'s `boundRecord` scope entry and + * `usePredicateRecordContext` (`hooks/useExpression.ts`). Those answer "did a + * row bind?", and their answer has its own pinned meaning: binding NOTHING + * rather than an empty row is what keeps a host-supplied `record` from being + * shadowed. Should "config bag" ever narrow (rejecting a class instance, say), + * the row sites must NOT follow — merging them would make that a single edit + * with two rulings behind it. The pin in `configBag.pin.test.ts` names both + * sites with this reason, so the next reader finds a decision rather than an + * oversight. + */ +export function isConfigBag(value: unknown): value is Record { + return !!value && typeof value === 'object' && !Array.isArray(value); +} diff --git a/packages/react/src/utils/propsBagDiagnostic.ts b/packages/react/src/utils/propsBagDiagnostic.ts index d54ef23d1..db7beaa9b 100644 --- a/packages/react/src/utils/propsBagDiagnostic.ts +++ b/packages/react/src/utils/propsBagDiagnostic.ts @@ -6,6 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +import { isConfigBag } from './configBag.js'; + /** * Dev-build diagnostic: a `props` CONFIG BAG on a node whose renderer reads its * config from `schema` — so the keys inside it are evaluated, spread as React @@ -185,11 +187,6 @@ export function __resetDroppedPropsBagWarnings(): void { _warnedPropsBags.clear(); } -/** A real config bag: an object, and not an array pretending to be one. */ -function isConfigBag(value: unknown): value is Record { - return !!value && typeof value === 'object' && !Array.isArray(value); -} - /** * Which keys of the OUTGOING props bag are dropped by a `schema`-reading * renderer, or `null` when there is nothing to say. diff --git a/packages/react/src/utils/unevaluatedExpression.ts b/packages/react/src/utils/unevaluatedExpression.ts index 8db55f6c1..0c98a84a5 100644 --- a/packages/react/src/utils/unevaluatedExpression.ts +++ b/packages/react/src/utils/unevaluatedExpression.ts @@ -6,6 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +import { isConfigBag } from './configBag.js'; + /** * Dev-build diagnostic: an UNEVALUATED template expression reached the DOM. * @@ -102,8 +104,8 @@ function scanBag( into: UnevaluatedExpressionFinding[], skip?: (key: string, value: string) => boolean ): void { - if (!bag || typeof bag !== 'object' || Array.isArray(bag)) return; - for (const [key, value] of Object.entries(bag as Record)) { + if (!isConfigBag(bag)) return; + for (const [key, value] of Object.entries(bag)) { if (typeof value !== 'string') continue; if (skip?.(key, value)) continue; const expressions = findExpressionSources(value); @@ -141,10 +143,7 @@ export function collectUnevaluatedExpressions( // The hoist copies every `properties.*` value onto the node's top level, so // the same residue is visible twice. Report the authored spelling only. - const hoisted = - properties && typeof properties === 'object' && !Array.isArray(properties) - ? (properties as Record) - : undefined; + const hoisted = isConfigBag(properties) ? properties : undefined; scanBag(spread, 'schema', findings, (key, value) => hoisted?.[key] === value); scanBag(props, 'props', findings);