CORE-2737: Use the published ui-components CSS colour audit in REX - #3137
CORE-2737: Use the published ui-components CSS colour audit in REX#3137OpenStaxClaude wants to merge 3 commits into
Conversation
…nd copy CORE-2731 hand-wrote ~380 lines of CSS colour parsing that already existed in ui-components, and the two copies had diverged before either merged: of the four defects review found here, one was REX-only and two were shared. CORE-2736 published the engine -- as REX's shape, since it was the superset -- so there is one copy to fix. What is left in src/test/cssColors.ts is the policy: which colours count as theme values (themeColorIndex), which files are in scope (stylesheetFiles), what is tolerated (KNOWN_OFF_PALETTE), how an occurrence is identified in the baseline (occurrence) and how a violation is worded. None of that is publishable, and all of it is REX's answer rather than a shared one. 587 lines to 181. The engine's own spec cases went with it, so cssColors.spec.ts keeps only themeColorIndex and tokenChoices. A second copy of the assertions would drift the way the second copy of the code did. theme.baseline.json regenerates byte-identically against the imported engine, which is what shows the swap is behaviour-preserving rather than merely compiling. The engines were not quite identical -- ui-components had since added device-cmyk() and `scrollbar` to the property lists, a dotall flag on the rgba() grammar, and hex rather than raw channels for colorKey -- and none of it moves a line of the baseline: the key format is internally consistent because the index and its lookups share the function, and KNOWN_OFF_PALETTE is empty. jest needs the subpath named in moduleNameMapper for the same reason ts-utils does: ui-components exposes it through an `exports` map, and CRA's jest 28 does not read those.
RoyEJohnson
left a comment
There was a problem hiding this comment.
Try building using tag test-core-2736-1
Roy cut test-core-2736-1, which carries a dist, so the audit no longer needs a hand-built one to resolve. The five snapshots left failing last time are regenerated here. They were held back because the tag was undecided and they would only have to be redone; the tag now pins the component source, so they are the migration's real output rather than churn. All of it is the styled-components to plain-CSS move: ManageCookiesLink renders an inline <style> instead of a createGlobalStyle, and ToastContainer carries className="toast-container" in place of a generated .c0 with its rules inlined into the snapshot. The toast is still styled. The declarations that used to be in the .c0 block -- fixed position, right 2rem, grid, centred, 1vh gap -- are now in the package's ToastContainer.css, which the built module imports itself and which sideEffects: ["**/*.css"] keeps webpack from dropping. A snapshot cannot show this, since jest maps *.css to a style mock. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
test-core-2736-1's "Add dist" commit force-added dist/esm only, so anything reaching the package through require resolved to nothing. -2 is built with `yarn dist` from the same source commit and ships both halves plus the CSS. No other change: the baseline still regenerates byte-identically against it, and the five migration snapshots taken against -1 are unchanged, because -1 and -2 are the same source tree with a different build committed.
This comment was marked as resolved.
This comment was marked as resolved.
"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>
…ule (#148) * CORE-2736: Extract the CSS colour audit engine into a publishable module 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. * Point the module field at the ESM entry that the build emits "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> * Remove unnecessary explanation from comment --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Roy Johnson <roy.e.johnson@rice.edu>
|
Heads-up: the base branch moved under this one. #3133 has been split in two at Roy's request, so That is the boundary this PR needed anyway: swapping in the published engine is now "delete Still a draft and still blocked by CORE-2736 either way. |
Second half of converging the two CSS colour audits, on top of CORE-2736.
Jira: https://openstax.atlassian.net/browse/CORE-2737
Based on
CORE-2731-global-css-theme-tokens(#3133), which is where the REX copy of the engine lives. Review that first; the diff here is only the deduplication.Stacking
This needs two parents, and they are independent of each other — a diamond rather than a chain:
yarn installon node 14.2731 and 3138 share only
package.json, in unrelated parts of it, and each is green without the other. So the suggestion is to land #3138 onmainon its own, then rebase 2731 ontomain— it picks up node 22 for free — and leave this PR on 2731 where it already sits. That gets both parents with nothing re-targeted. Stacking 3138 on 2731 would instead hold an already-green EOL-runtime upgrade behind a CSS refactor still in review.Note that stacking alone will not make this PR green: the
modulefix in item 2 below is the binding constraint.What this does
src/test/cssColors.tsgoes from 587 lines to 181. The parsing engine —NAMED_COLORS,stripNoise,declarations,takesColor,describeColor,findColors,stylesheetColors,colorKey,opaqueKey— is imported from@openstax/ui-components/theme/cssColorsrather than declared. What stays is the policy half, which is genuinely REX's:themeColorIndexover REX's nested theme,stylesheetFileswith its two exclusions,KNOWN_OFF_PALETTE,occurrence,colorViolations, andtokenChoices.The engine's spec cases went to ui-components with the engine, so
cssColors.spec.tskeeps onlythemeColorIndexandtokenChoices. Keeping a second copy of the assertions would drift the way the second copy of the code did.theme.spec.tsnow takesdescribeColorandstripNoisestraight from the package instead of re-exporting them throughcssColors.ts.The load-bearing check
theme.baseline.jsonregenerates byte-identically against the imported engine — same md5, emptygit diff. That is what shows the swap is behaviour-preserving rather than merely compiling.It was not a foregone conclusion: the published engine had since added
device-cmyk()toCOLOR_FUNCTIONSandscrollbartoCOLOR_SHORTHANDS, put a dotall flag on thergba()grammar, and switchedcolorKey/opaqueKeyfrom raw channels to hex. None of it moves a baseline line — REX's stylesheets contain none of those constructs, and the key format change is invisible becausethemeColorIndexand its lookups share the function.The deduplication is done. The dependency bump is where the work is.
Pinned at
openstax/ui-components#test-core-2736-2, whose dist is complete (-1force-addeddist/esmonly, so anything reaching the package throughrequireresolved to nothing).Three things stand between this and a green CI, none of them about CSS colours.
1. Sentry 10 needs node ≥ 18; REX pins 14.15.4
ui-components 1.23.x depends on
@sentry/react@^10.60.0. All CI jobs die inyarn installbefore anything compiles. Filed as CORE-2853 (node upgrade), where the investigation lives: the only real obstacle is webpack 4's md4 hashing under OpenSSL 3, fixable withNODE_OPTIONS=--openssl-legacy-provider; everything else — unit, lint, sourcemap, prerender, browser suites — already passes on node 18.Roy asked whether an earlier Sentry could avoid the bump. Tested on a real node 14.15.4: yes, with a ui-components change. Eleven of the twelve Sentry APIs ui-components uses exist in 7.120.4 (7.119+ backported the functional-integration API). The twelfth,
Sentry.captureReactException, is v9+. Withresolutions: {"@sentry/react": "^7.120.3"}, node 14 givesyarn installexit 0 with no--ignore-engines,yarn buildexit 0, 242/242 suites and 1942/1942 tests at 100% coverage, and a byte-identical baseline. Engine floors for reference: v7>=8, v8>=14.18(REX misses by a patch release), v9/v10>=18.2.⚠️ ui-components'
modulefield points at a file that does not exist — and it costs 3 MB"module": "index.js", with noindex.jsat the package root; it only resolves through theexportsmap. Webpack 4 (CRA 4) does not readexports, so it falls back tomainand bundlesdist/cjs. CommonJS means no tree-shaking:maintodayCI=trueis a hard build failure"module": "./dist/esm/index.js"A regression, not an original sin:
1e4b31136(#116) introduced the split with"./dist/esm/index.js";7ac2a8a57(#118) changed it to"index.js". Invisible to every consumer whose bundler readsexports.Fix
moduleonly —typesmust stay"index.d.ts", becausetypesVersionsrewrites it intodist/esm/and a full path there gets double-prefixed.3. The two interact, and that is the interesting part
Fix the packaging so webpack takes the ESM entry, and the Sentry 7 pin stops being silently survivable:
The pin only "works" today because the broken
modulefield forces the CommonJS path, where a missing export is invisible until it is called — which is when an error boundary catches, i.e. exactly when you least want aTypeError. Two bugs cancelling into a passing build. REX does not use ui-components'ErrorBoundary(it has its own), so the dead code is harmless here, but that is luck rather than design.Patching that one call to
Sentry.captureExceptionlocally, both fixes together give a clean node 14 build at 2.4 MB with everything else still green.What this needs
"module": "./dist/esm/index.js"in ui-components. Worth doing regardless of how anything else lands — straight regression, 3 MB of bundle.captureReactExceptionand widens@sentry/react(ideally to a peerDependency, so noresolutionsentry is needed here), or REX does CORE-2853 first. The former only defers CORE-2853 — node 14 has been EOL since April 2023.🤖 Generated with Claude Code