diff --git a/.changeset/5993-button-shared-icon-resolver.md b/.changeset/5993-button-shared-icon-resolver.md new file mode 100644 index 0000000000..3d17aa1e88 --- /dev/null +++ b/.changeset/5993-button-shared-icon-resolver.md @@ -0,0 +1,44 @@ +--- +'@object-ui/components': patch +--- + +`ui:button` resolves its authored `icon` through the shared `resolveIcon` instead of a +byte-equivalent copy of it (objectui#5993). + +`renderers/form/button.tsx` carried its own `toPascalCase`, its own `iconNameMap` holding +the single `Home -> House` entry, and its own index into lucide's runtime `icons` record — +the same algorithm as `renderers/action/resolve-icon.ts`, but not the same function. The +`action:*` family, `complex/data-table.tsx` and both menu renderers already import the +shared one. The hazard was drift, not rendering: an alias added to `resolve-icon.ts` to +absorb a lucide retirement (the objectui#5586 / #5622 mechanism) reached every one of those +sites and silently missed `ui:button`, which would have gone on resolving the retired +spelling to nothing while the rest of the repo resolved it correctly. + +**No behaviour changes, and that is measured rather than asserted.** The two +implementations were compared over 3547 names — every one of lucide's 1767 record keys in +both spellings, plus kebab-case probes (`arrow-right`, `dollar-sign`, `user-plus`), the +`Home` alias, retired spellings and `undefined`: 3539 identical by object identity, 8 +differing only in the nullish flavour returned for a miss (the copy indexed the record and +got `undefined`; the shared resolver `?? null`s it), zero genuine forks. That one +difference cannot reach the DOM — `Icon` is consumed at exactly two sites, both +`{!isLoading && Icon && }` truthiness tests, and React renders nothing for +`null` and `undefined` alike. Icon identity, `h-4 w-4` sizing, `iconPosition`, the loading +state and the `Loader2` spinner are unchanged, and are pinned by +`renderers/form/__tests__/button-shared-icon-resolver.test.tsx`. + +Because behaviour is unchanged, the usual red-before ablation does not exist for this +change and none was manufactured. The one row in that suite that discriminates is +structural: it spies on the shared module and fails when the glyph does not come out of it, +which is red on the copy and green on the import. + +`scripts/check-lucide-icon-record-names.mjs` drops `form/button.tsx` from +`DECLARED_RECORD_READERS` in the same commit — that gate rediscovers record readers from +source on every run and fails on drift in both directions, so the removal is verified by +the gate rather than declared. It is also what now guards the dedupe: a re-inlined copy +would be discovered as an undeclared record reader and fail. The census entry for the +`button` *type* stays, its resolver re-pointed at `resolve-icon.ts`, so `ui:button`'s +authored icon names are still judged against the live record. + +`renderers/basic/icon.tsx` keeps its own copy deliberately and is untouched: `ui:icon` +draws a `SquareDashed` placeholder and warns on an unresolvable name (objectui#5631), which +the shared resolver does not do. diff --git a/packages/components/src/renderers/form/__tests__/button-shared-icon-resolver.test.tsx b/packages/components/src/renderers/form/__tests__/button-shared-icon-resolver.test.tsx new file mode 100644 index 0000000000..20090a0eb4 --- /dev/null +++ b/packages/components/src/renderers/form/__tests__/button-shared-icon-resolver.test.tsx @@ -0,0 +1,196 @@ +/** + * 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. + */ + +/** + * `ui:button` resolves its authored `icon` through the SHARED resolver + * (objectui#5993). + * + * ## What was on the tree, and what this suite can and cannot prove + * + * `renderers/form/button.tsx` carried a byte-equivalent reimplementation of + * `renderers/action/resolve-icon.ts` — its own `toPascalCase`, its own + * `iconNameMap` with the single `Home -> House` entry, its own index into + * lucide's runtime `icons` record. Same algorithm, not the same function. + * + * ⚠️ The two implementations were behaviourally EQUIVALENT, so this suite must + * not pretend otherwise. Measured over 3547 names before the dedupe (every one + * of lucide's 1767 record keys in both spellings, plus kebab-case probes, the + * `Home` alias, retired spellings and `undefined`): 3539 identical by object + * identity, 8 differing ONLY in the nullish flavour returned for a miss (the + * copy indexed the record and got `undefined`; the shared resolver `?? null`s + * it), and ZERO genuine forks. `Icon` is consumed at exactly two sites, both + * `{!isLoading && Icon && }` truthiness tests, so that one + * difference cannot reach the DOM. + * + * That has a consequence for how these rows read, and it is stated here rather + * than left for a reviewer to discover: + * + * - The BEHAVIOUR rows below are GREEN IN BOTH WORLDS. They are not evidence + * that the dedupe is correct — they are the guard that it changed nothing + * (icon identity, size, `iconPosition`, the loading state). A green run + * here proves NO-CHANGE, which is the whole contract of this card. + * - Exactly one row DISCRIMINATES, and it is a structural one: the ROUTING + * row. Before the dedupe `resolveIcon` was never called by this renderer, + * so the spy below records zero calls and that row is RED. It is the only + * honest red-before this card has, and it pins the thing that actually + * changed: which function the glyph came out of. + * + * ## Why the shared module is spied rather than replaced + * + * The factory delegates to `importOriginal`, so every behaviour row still runs + * the REAL resolver against the REAL lucide record. A stub returning a fixed + * component would have deleted the half that matters — that a RETIRED spelling + * resolves to NOTHING rather than degrading to a wrong glyph (`edit` is that + * control: a deprecated lucide export whose key is absent from the runtime + * record, measured on lucide-react 1.31.0). Only the CALL is observed. + * + * ## Why the renderer is invoked DIRECTLY + * + * `ComponentRegistry.get('button')` returns the component the registry actually + * renders; driving through `SchemaRenderer` injects its own props around it and + * can be green in both directions (PR #4603's toggle case, restated by #4580). + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { ComponentRegistry } from '@object-ui/core'; + +vi.mock('../../action/resolve-icon', async (importOriginal) => { + const actual = await importOriginal(); + return { resolveIcon: vi.fn(actual.resolveIcon) }; +}); + +import { resolveIcon } from '../../action/resolve-icon'; +// Module scope, not `beforeAll` (objectui#3010/#3021). +import '../../../renderers'; + +const shared = vi.mocked(resolveIcon); + +beforeEach(() => shared.mockClear()); +afterEach(() => cleanup()); + +/** + * DOM ORDER inside the button, read off `childNodes`. + * + * ⚠️ NOT `compareDocumentPosition` against `screen.getByText('Go')`: the label + * is a bare TEXT NODE, so that query returns its closest ELEMENT — the + * `