CORE-2736: Extract the CSS colour audit engine into a publishable module - #148
Conversation
The audit added in CORE-2720 is ~250 lines of pure CSS parsing that knows nothing about ui-components. REX needed exactly the same thing and, because this lived inside a spec file where nothing could import it, got a second hand-written copy instead — and the two had diverged before either merged. Moves the parsing engine to src/theme/cssColors.ts so it compiles into dist and resolves as @openstax/ui-components/theme/cssColors. tokens.spec.ts keeps the ui-components layer: the palette index, the file walk, KNOWN_OFF_PALETTE and the assertions. The published surface is REX's shape rather than this repo's, because REX's is the superset: it carries the selector `context` its baseline ratchet identifies an occurrence by, and returns structured channels rather than a formatted key. Adopting it means the REX side is a delete-and-import with no call-site changes; the reverse would have left REX forking `declarations` to get `context` back, which is the duplication this ticket exists to remove. Behaviour here is unchanged — same 90 assertions over the same stylesheets, plus the property-context and hex-grammar cases the two copies' reviews turned up. The engine's own tests come across with it so the published surface is the tested surface.
There was a problem hiding this comment.
🟢 Approval recommended
The extraction is self-contained, maintains the existing consumer-facing behavior in tokens.spec.ts, and adds comprehensive unit coverage for the new published module.
Pull request overview
Extracts the shared CSS colour-audit “engine” into a publishable, dependency-free module (src/theme/cssColors.ts) so other consumers (notably REX) can reuse the same parsing and scanning logic rather than maintaining a divergent copy inside test code.
Changes:
- Added a standalone CSS colour parsing/scanning module (
cssColors.ts) that finds color literals in CSS declarations and normalizes resolvable values to RGBA. - Added dedicated engine tests (
cssColors.spec.ts) to cover noise stripping, declaration parsing (incl. context), literal discovery, and normalization/keying. - Updated
tokens.spec.tsto consume the extracted engine while keeping ui-components-specific policy (theme token set, allowlist, and error wording) in the spec.
File summaries
| File | Description |
|---|---|
| src/theme/tokens.spec.ts | Switches the palette/token policy tests to use the extracted cssColors engine and adds a few contract-level cases. |
| src/theme/cssColors.ts | New publishable CSS colour audit engine: noise stripping, declaration/context scanning, literal discovery, and color normalization/keying. |
| src/theme/cssColors.spec.ts | New unit tests for the engine’s parsing, scanning, and normalization behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
abb77e5 to
733c905
Compare
This comment was marked as resolved.
This comment was marked as resolved.
88a4922 to
733c905
Compare
"module": "index.js" names a file that does not exist — there is no index.js at the package root, only dist/esm/index.js and dist/cjs/index.js. Bundlers that read the exports map never notice, because exports already routes browser/import to dist/esm. Webpack 4 does not read exports, so it falls through the missing module target to main and bundles dist/cjs, and CommonJS does not tree-shake: REX measures chunk 519 at 5.4 MB that way, over the 5 MB workbox precache limit, versus 2.4 MB through the ESM entry. Collateral damage of a types fix rather than an original sin. 1e4b311 (#116) introduced the esm/cjs split with the correct "./dist/esm/index.js"; 7ac2a8a (#118) rewrote module and types together in one edit. The types half had to change — typesVersions rewrites "*" into dist/esm/, so a full path there resolves twice — but module is not subject to typesVersions and did not. So types stays "index.d.ts". Verified after the change: tsc resolves the package to dist/esm/index.d.ts and the subpath REX imports to dist/esm/theme/cssColors.d.ts, require() still gets cjs and import still gets esm through the exports map, and a webpack-4 mainFields resolver (['browser','module','main']) moves from dist/cjs/index.js to dist/esm/index.js. Asked for in review, as item 1 of "What this needs" on openstax/rex-web#3137. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Done — 7be1a72, checks green. One thing worth saying plainly, since it changes what the fix is: the build already creates Verified three ways
The middle row is the one that could have bitten. I resolved Provenance is slightly sharper than "a regression": Tag whenever you like — no back-out this timePushed Item 2 is still yours to callI did item 1 only, since that is what you asked for and it stands on its own. Item 2 is a judgement call I shouldn't make unilaterally, so here is the scope: Worth knowing that item 1 makes item 2 sharper rather than softer. Per REX's own findings, the broken I left 🤖 Generated with Claude Code |
| * Published deliberately, not incidentally. This engine knows nothing about | ||
| * ui-components; REX needed exactly the same thing and, because the first copy lived | ||
| * inside a spec file where nothing could import it, got a second hand-written one | ||
| * instead. The two had already diverged before either merged. See CORE-2736. |
There was a problem hiding this comment.
This block can be moved to the PR description, I don't think this context is important to keep long term
4424b6f
into
CORE-2720-global-css-theme-tokens
CORE-2736
Gets the shared half of the CSS colour audit out of
tokens.spec.tsand into something REX can import, so rex-web#3133 can delete its copy. First half of converging the two audits; the REX side is CORE-2737.Why
#143 and rex-web#3133 each added a colour audit. They are the same design implemented twice, and the duplication already cost us: Copilot's review of the REX PR found four defects, three of them in the shared engine, and they did not line up. Percentage channels were scaled by the decimal
2.55in REX (sorgb(50%, 50%, 50%)and#808080could never match) but not here; the hex-grammar and property-context defects were in both. Nothing would have caught the divergence, because the engine lived inside a spec file where the other copy could not reach it.What moves
src/theme/cssColors.ts— the parsing engine: noise stripping, the declaration scanner, the colour-literal walk,describeColor, the 148-entry named-colour table, the colour-function set.tokens.spec.tskeeps everything that knows about this repo: the palette index, the file walk,KNOWN_OFF_PALETTE, the assertions.It imports nothing at all — no
fs, nopath, no node built-ins, verified on the built output. That is the constraint that makes publishing it safe: it resolves through the existing"./*"wildcard subpath, which carries abrowsercondition in a library that is otherwise browser-only, so a node import here is something a consumer's bundler could try to follow. The file walk belongs to whoever owns the file tree.No
package.jsonor build-script change was needed.dist/esm/theme/cssColors.js,dist/cjs/theme/cssColors.jsand a.d.tsfor each are emitted, and the specs stay excluded.One decision worth review
The published surface is REX's shape, not this repo's —
describeColorreturns structuredRgba | nullrather than a pre-formatted key,declarationscarries the selectorcontext, and there is astylesheetColorswalk over both.The ticket's plan assumed this repo's engine would be the one published. Reading both copies side by side, REX's is the superset, and the difference is not cosmetic: its baseline ratchet identifies a colour occurrence by the declaration it was written in, so it needs
contextfromdeclarations. Publishing this repo's narrower version would have left REX forkingdeclarationsto getcontextback — the duplication this ticket exists to remove, minus the part that already worked. Adopting REX's shape makes the REX side a delete-and-import with no call-site changes.Two small departures from REX's version, both to keep this repo's behaviour:
colorKeyreturns the hex form (#cccccc, or#000000/0.2when translucent) rather than comma-separated channels, because that is what an allowlist entry and a failure message have to be recognisable as; andCOLOR_FUNCTIONS/COLOR_SHORTHANDSare the union of the two lists.Behaviour
Unchanged, deliberately. Both engine defects had already been fixed in #143 by the time this was picked up, so this is extraction only — same 90 assertions over the same stylesheets, verified by diffing the test-name list before and after. The four deltas are the malformed-hex cases moving to the engine spec, and four additions covering the acceptance criteria:
#gggresolves to null rather than a bogus key, andunresolvableColorsis now a function of its entries so the guard can be shown to fail on a malformed palette value rather than only ever running over a sound theme.animation-name: redandfont-family: whitestay quiet whilecolor: red,border: 1px solid redand a named colour in a custom property are still reported — tested in both directions, here and in the engine spec.rgb(100%, 100%, 100%)matches--ox-color-white, the percentage-channel bug that was REX-only.The engine's own tests come across with it, so the published surface is the tested surface:
src/theme/cssColors.spec.ts, 105 assertions.Also here: the
modulefield7be1a7221points"module"at./dist/esm/index.js. It namedindex.js, which does not exist at the package root, so bundlers that skip theexportsmap (webpack 4, so CRA 4) fell through tomainand bundleddist/cjs— CommonJS, no tree-shaking, 3 MB of REX bundle and a build over workbox's precache limit. Unrelated to the colour audit; asked for in review as item 1 of "What this needs" on rex-web#3137, and it stands on its own.typesdeliberately stays"index.d.ts"—typesVersionsrewrites it, and a full path there resolves twice. Detail in this comment.Still stacked on #143
Based on
CORE-2720-global-css-theme-tokensrather thanmain—tokens.spec.tsdoes not exist onmainyet. Retarget once #143 merges.After it merges we need a version tag, since consumers reference ui-components by git tag rather than npm; the number goes on CORE-2737, which has to bump REX's reference to it. Ahead of that, branch
CORE-2736-dist(ec72991ba) carries a builtdist/on top of this branch so a test tag can be cut for REX without putting build output on the PR branch. Tag it and delete it; nothing needs backing out.🤖 Generated with Claude Code