-
Notifications
You must be signed in to change notification settings - Fork 0
Review 5512
#5512 build(themes): emit standalone icons as ESM only by jiunshinn (bucket: collaborator)
42c9e9232e7c46c98e58dc3b172b862fe6abd430 <- every claim below was verified at this commit
LOOP VERSION: 1.4.0 AUDIT RUBRIC: 1.12
LANE: full WHY: This changes the published artifact graph for seven public theme packages and expands an executable export verifier on a fork-owned head. It therefore has public-package, compatibility, and trust implications even though it changes no component runtime source.
PRIOR REVIEW: none — the review and inline-comment endpoints are empty, so there is nothing to extend or contradict.
TITLE CHECK: honest — build(...) accurately describes build-output and verification changes with no component behavior claim.
OVERLAP: #5099 is the same author's documentation-only sidecar follow-up; it touches four CLI doc files and does not compete with this implementation.
A builder installing any maintained theme receives a standalone CommonJS icon artifact that no package export or generated theme module can reach. In the published 0.5.0 butter tarball that file alone is 4,214 bytes; the package export map exposes only the root and /built, the root bundles its registry, and /built imports ./icons.mjs.
VERDICT: clear
The package root still needs both ESM and CommonJS builds, but the generated built-theme module has only one standalone icon dependency: the ESM companion. Each theme now builds the root in both formats and the icon companion only as ESM. The repository verifier then loads advertised Node import and require targets and rejects maintained themes that lose icons.mjs or regain icons.js.
SOLUTION (2 build-system decisions · 0 component-runtime lines of 204 additions)
- Split all seven theme build graphs:
source.tsremains CJS+ESM;icons.tsxbecomes ESM-only. [stated problem] - Extend export verification to load require targets and enforce the icon artifact invariant. [stated problem]
Both decisions serve the root cause and invariant in the PR body. Nothing is marked [NO STATED PROBLEM]; two decisions are below the split smell.
VERDICT: clear
OWNER: each theme package owns its emitted formats; the repository export verifier owns the cross-package invariant
TIER 1: package export maps and Node's import/require loaders
TIER 2: none
SEAMS: package root import, package root require, /built import, /built require, declaration resolution, and packed files
BEHAVIOR UNIT: inline build configuration plus one standalone verifier — appropriate for a repository-wide packaging invariant
| seam | driven result |
|---|---|
root import
|
exact-head CI emitted dist/source.mjs for all seven; verifier included it among 31 clean import targets |
root require
|
exact-head CI emitted dist/source.js for all seven; each is one of 14 clean require targets |
/built import |
generated <theme>.js still imports ./icons.mjs; exact-head verifier loaded all advertised import targets |
/built require |
unchanged export maps still point to generated <theme>.js; exact-head Node 24 CI loaded all seven, contributing the other seven require targets |
| declarations | each package still runs its unchanged declaration-only tsc build; types paths and public declarations are unchanged |
| packed files | exact-head build logs show seven icons.mjs, seven theme builds with zero icons.js, and the verifier enforces both facts; the uploaded dists archive had expired before review |
The implementation lives with the output owner, derives the maintained-theme set from package names rather than a hand-maintained list, and keeps the existing export maps unchanged.
VERDICT: clear
Theme-package builders keep the same root and /built import/require entry points and types. Installations stop carrying one unreachable CommonJS icon sidecar per maintained theme; no app-rendered behavior changes. The exact packed-size delta was not independently measured because the exact-head dists archive expired, so no size claim beyond removal of the file is made.
VERDICT: clear
No supported API change. No export-map key, condition, source export, type, default, or accepted value changes. dist/icons.js is not exported; with an exports map present, package-name deep imports to it are not supported. The public root remains source.mjs for import and source.js for require; /built remains the generated theme module for both conditions.
OSSIFICATION: nothing new is exposed. The removed artifact is an unreachable implementation by-product, and restoring it later would not require a consumer migration.
VERDICT: clear
No theme token, target, variable, icon registry value, selector, component override, or theme source changes. The seven changed theme files are build configs only and are byte-identical after the change.
VERDICT: clear
BEHAVIOR: no — no theme definition, icon registry, condition, default, or component state changes API: no — every advertised export condition and declaration path is unchanged; exact-head Node CI loaded 31 import and 14 require targets VISUAL: no — no rendered source, CSS, DOM, theme value, or icon mapping changed THEME: no — existing targets, tokens, overrides, and registries are unchanged
The removed icons.js cannot be reached through the package export map. Root CommonJS consumers keep source.js, whose icon registry remains bundled; /built keeps the same target and now consumes the retained ESM sidecar.
VERDICT: clear
EFFECTS: zero; no React source changed RENDER: no reachable runtime-work change; the same theme objects and icon registries remain LISTENERS/OBSERVERS: zero LAYOUT: no DOM, style, or layout work BUNDLE: one unexported CommonJS sidecar is removed from each published theme. CI shows the remaining root CJS, root ESM, and icon ESM outputs for all seven. No independent consumer-bundle delta is claimed.
The build now invokes two tsup configurations instead of one combined graph. Exact-head CI completed the full build successfully; no comparative build-time measurement was available, so none is claimed.
VERDICT: clear
VISUAL CHECK: not applicable
WHY: the complete nine-file diff contains one changeset, seven tsup.config.ts files, and one repository verifier. It changes no component/theme source, CSS, DOM, geometry, color, or visible state. PR Analysis found no modified components and exact-head pr-visual was skipped by path.
No frames: this is a genuinely non-rendering package-build change.
VERDICT: clear
No interactive element, role, ARIA state, focus/keyboard path, visible/AT-facing runtime string, locale, or direction behavior changes. Exact-head pr-a11y and pr-rtl were skipped because no component path changed; that skip is consistent with the static path proof rather than used as proof by itself.
VERDICT: clear
UNSAFE TO RUN ON THE REVIEWER MACHINE. The fork-owned scripts/verify-exports.mjs executes every collected runtime export in a child Node process (lines 224–264) with the reviewer's filesystem and environment available; the seven modified tsup configs are also executable build inputs. Per STEP 0, no dependency install, build, test, or repository script was run locally. The checkout used hooks disabled. Review evidence is static inspection plus exact-head hosted CI logs.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | clear |
| ARCHITECTURE | clear |
| IMPACT | clear |
| API | clear |
| THEMING | clear |
| BREAKING | clear |
| PERFORMANCE | clear |
| VISUAL | clear — not applicable, non-rendering paths |
| A11Y & I18N | clear — untouched |
GOAL: met — exact-head CI emitted icons.mjs and no icons.js for all seven themes, preserved both root formats, and loaded 31 import plus 14 require targets.
DISPOSITION: no code findings. The failed Vercel deployment is a currently-red gate; its logs were not accessible without project access, so whether it relates to this diff's build split is unverified — treat it as open, not cleared.
ADVICE: omitted — no corrective change is supported by the evidence.
AUTHOR CAN PROCEED: yes — no code change is requested; merge still requires the Vercel status and review-required gate to clear.
WORST OUTCOME: none found → approve.
JUDGEMENT NEEDED: none — this removes no supported surface and introduces no API/design concept.
approve (merge remains CI-held)
Thanks, this looks good. The split preserves every advertised import and require path while dropping only the unreachable CommonJS icon artifacts.
[Reviewed by Robohands]
None.
- Exact-head
build-storybookran the fullpnpm build: every maintained theme emittedsource.js,source.mjs, andicons.mjs; noicons.jsappeared. Each theme's fully-specified check reported 4 runtime files clean. - Exact-head export verification: 10 public packages checked; 31 import and 14 require targets loaded cleanly.
- Exact-head test job: 580 test files passed, 1 skipped; 12,223 tests passed, 5 skipped.
check:repopassed; ESLint finished with 0 errors and 80 repository warnings. - Exact-head check runs: 14 success, 4 skipped (
Anchor review-clear chain,pr-a11y,pr-rtl,pr-visual), none failed. Separate commit statuses make the combined statefailure:review-requiredis pending and Vercel is failed. - Static checks: complete 438-line diff and 307-line verifier read; all seven configs read and byte-identical;
git diff --checkclean; no prior reviews or inline comments; GitHub reportsMERGEABLE; currentmainhas not touched these files since the PR base.
- Published 0.5.0 tarballs confirm the old shape; butter ships a 4,214-byte
dist/icons.jsthat its export map never names. - The 14 require targets are exactly root plus
/builtfor each of the seven maintained themes. - Open #5099 documents the icon-sidecar contract; it is same-author, docs-only, and does not compete with this implementation.
TIME total 28m setup 3m safety inventory, isolated exact-head worktree, fresh wiki clones (warm main reused: no) reading 14m full kit/rubric/wiki, description, test plan, diff, source, package history measuring 4m exact-head CI/log and published-artifact inspection; 0 local executions, 0 re-measures writing 7m draft, two self-critiques, independent critic correction, wiki record waste 2m attempted the expired exact-head dists artifact and one malformed log-fetch loop
- No local install/build/test/script execution was permitted after the safety gate failed.
- The exact-head
dists-42c9e92artifact expired; its build and verifier logs remain available and were inspected. Vercel exposes the failure state but not its logs without project access.
Not posted. The first self-critique added explicit prior-review, title, and overlap checks. The independent critic found one R14 overclaim about the inaccessible Vercel failure; the rewrite now leaves causality unverified and the merge gate open. The final self-critique found no remaining R1–R15 violation. The verdict and 21-word public review did not change.