Conversation
🦋 Changeset detectedLatest commit: 6fe971c The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +51 B (+0.04%) Total Size: 129 kB 📦 View Changed
ℹ️ View Unchanged
|
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (40d5e83) and published all packages with changesets to npm. You can install the packages in ./dev/tools/deploy_wonder_blocks.js --tag="PR3071"Packages can also be installed manually by running: pnpm add @khanacademy/wonder-blocks-<package-name>@PR3071 |
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-cweohnpjhw.chromatic.com/ Chromatic results:
|
jandrade
changed the title
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| "**/*.css" | ||
| ], | ||
| "exports": { | ||
| ".": { | ||
| "import": "./dist/es/index.js", | ||
| "require": "./dist/index.js", | ||
| "types": "./dist/index.d.ts" |
There was a problem hiding this comment.
🔴 The new conditional exports map lists the types condition last, but per the TypeScript handbook it must come first — Node-style condition resolution is order-sensitive and the first match wins, so TS consumers on moduleResolution: "node16"/"nodenext" will resolve import → ./dist/es/index.js and never reach the types key. The top-level types field is ignored once exports is set under modern resolution, so this is a real published-surface bug. wonder-blocks-tokens has the same broken pattern (which is presumably where this was copied from), but since this PR introduces the first exports map of the migration and more packages will be migrated using it as the template, it's worth fixing the precedent here before it propagates. Trivial fix: move "types" to be the first key inside the "." object.
Extended reasoning...
What the bug is
packages/wonder-blocks-icon/package.json adds a new conditional exports map:
"exports": {
".": {
"import": "./dist/es/index.js",
"require": "./dist/index.js",
"types": "./dist/index.d.ts"
},
"./css": "./dist/index.css"
}The types condition is listed last. Per the TypeScript handbook on package.json exports:
...the
"types"condition should always come first in"exports".
This is because Node-style conditional resolution is order-sensitive — the resolver walks the object's keys in declaration order and returns on the first matching condition. import and require are both "always-on" for the relevant module systems, so once one of them matches the resolver returns the .js path and never reaches types.
Step-by-step proof
Consider a downstream consumer with tsconfig.json moduleResolution: "node16" (or "nodenext") running import {Icon} from '@khanacademy/wonder-blocks-icon':
- The TS resolver reads
packages/wonder-blocks-icon/package.jsonand finds theexportskey. Per modern resolution, onceexportsis set, the top-leveltypesfield is ignored for this subpath. - The resolver enters the
"."entry and walks the conditions in declaration order:import,require,types. - The consumer's module system is ESM, so the
importcondition matches first → resolves to./dist/es/index.js. - Resolution returns immediately. The
typescondition is never evaluated. - TS now looks for declarations adjacent to
./dist/es/index.js— i.e../dist/es/index.d.ts. Buttsconfig-build.jsonsetsoutDir: "./dist"/rootDir: "src", so declarations are actually emitted to./dist/index.d.ts, not./dist/es/index.d.ts. - Result: under strict node16/nodenext resolution, the consumer fails to find types for
@khanacademy/wonder-blocks-icon.
If the order were types → import → require, step 2 would return ./dist/index.d.ts immediately and the consumer would get correct typings.
Why existing code doesn't prevent it
The top-level "types": "dist/index.d.ts" field is still present, which rescues old resolvers (moduleResolution: "node"/"node10") — but modern resolution modes (node16, nodenext, bundler) prefer the exports map and skip the top-level types field once exports is defined. This is exactly the failure mode publint / @arethetypeswrong/cli flag.
Why wonder-blocks-tokens having the same pattern isn't a defense
wonder-blocks-tokens/package.json (lines 21-28) has the same broken order, and yes — the new code was clearly patterned after it (the PR description even calls this out: "Published-surface change patterned after wonder-blocks-tokens."). But propagating a bug isn't a reason to keep doing it. Two reasons to fix here:
- This PR is Phase 0.1 of a multi-phase migration. The description and changeset both make clear that many more packages will get
exportsmaps copied from this template. Fixing the precedent now means future packages get it right by default; leaving it means the same bug ships in every future PR in the chain. wonder-blocks-tokenshaving shipped without obvious breakage doesn't prove the issue is harmless — it proves either (a) most downstream consumers happen to use the lenientbundlerresolution mode which falls back gracefully, or (b) consumers who hit it work around it silently. Neither is a reason to enshrine the pattern.
Impact
For TS consumers on moduleResolution: "node16" or "nodenext" (the recommended modes for modern Node-targeted packages), types may fail to resolve, producing implicit-any imports or hard errors depending on strict settings. Bundler-mode consumers are mostly OK because that resolver is lenient about types, but they'd still flag this with publint/are-the-types-wrong.
Fix
Move "types" to be the first key inside the "." exports object:
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/es/index.js",
"require": "./dist/index.js"
},
"./css": "./dist/index.css"
}Worth applying the same fix to wonder-blocks-tokens/package.json in a follow-up so the pattern is consistent across the codebase.
|
|
||
| return ( | ||
| <StyledDiv | ||
| style={[getSize(size), style]} | ||
| className={styles[size]} | ||
| style={style} | ||
| id={id} |
There was a problem hiding this comment.
🟡 nit: className={styles[size]} returns undefined for any size outside the IconSize union, leaving the icon with no width/height class and collapsing it to 0×0. The previous getSize used case "small": default: so invalid values silently fell back to small. TypeScript blocks this for typed consumers, but JS-only consumers (or anyone casting to any) lose the fallback — a one-line preservation of prior behavior would be className={styles[size] ?? styles.small}.
Extended reasoning...
What changes
The old getSize had a case "small": default: arm, so any size value that wasn't one of the four recognized strings silently rendered as small. The new code uses direct property access:
<StyledDiv className={styles[size]} ... />For any size not in {small, medium, large, xlarge}, styles[size] is undefined. React strips an undefined className, so the div renders with no class. Since width/height come only from the size classes (the cloned child gets width: 100%; height: 100%), the icon collapses to 0×0.
Why the existing safeguards don't catch this
- The destructuring default
size = "small"(icon.tsx:45) only fires whensize === undefined. It does not fire fornull,"", or any other invalid string. - The
IconSizeunion inpackages/wonder-blocks-icon/src/types.ts:11blocks invalid values at compile time for TypeScript consumers, but is erased at runtime — a JS consumer or anyone usingas any/@ts-ignorereaches the bad path.
Step-by-step proof
- JS consumer calls
<Icon size={someDynamicValue}>wheresomeDynamicValueis"xxlarge"(perhaps a refactor leftover, a value from a server response, or a typo). - The destructuring
size = "small"does not apply because"xxlarge"is notundefined. styles["xxlarge"]evaluates toundefined(no such export from the CSS module).- React renders
<div className={undefined}>→<div>with no width/height class. - The child element has
width: 100%; height: 100%— but the parent has no intrinsic size, so the percentage resolves to 0. - Icon renders as 0×0 instead of the pre-PR behavior (silently small).
Addressing the refutation
The refutation argues this is intentional and that visible failure beats silent fallback. Both are reasonable in isolation, but two things weaken that case:
- The PR's own changeset explicitly says "same props, same DOM output" — invalid runtime input is one place where that claim no longer holds, and that's worth either fixing or noting in the changeset.
- The fix is a single character (
??and a fallback):className={styles[size] ?? styles.small}. Preserving the prior contract costs almost nothing.
If the team prefers the new visible-failure behavior as a deliberate design choice, that's a defensible call — but it should be documented as an intentional behavior change in the changeset rather than presented as a pure DOM-equivalent refactor.
Severity
nit — TypeScript users are protected; only affects JS consumers or type-bypass cases. Not blocking, but flagging because the fix is trivial and the PR description currently overclaims behavioral equivalence.
| // Root PostCSS config — picked up by Vite (Storybook) and Rollup | ||
| // (`rollup-plugin-postcss` in `build-settings/rollup.config.mjs`). | ||
| // | ||
| // Plugin order matters: | ||
| // 1. postcss-import — inline @import (incl. cross-package paths | ||
| // resolved through the workspace). | ||
| // 2. @csstools/postcss-mixins — expand @define-mixin / @mixin before any | ||
| // downstream transform sees them. | ||
| // 3. wrap-in-layer — move every remaining top-level rule into | ||
| // `@layer shared { … }` so authors never | ||
| // write the wrap by hand. Layer order is | ||
| // declared once in | ||
| // `.storybook/preview-head.html` | ||
| // (`@layer reset, tokens, shared, overrides`). | ||
| // | ||
| // CSS Modules class-name hashing is handled by the consumer: | ||
| // - Vite handles `*.module.css` natively (its built-in CSS Modules pass | ||
| // runs *after* this PostCSS chain). | ||
| // - Rollup uses `rollup-plugin-postcss`'s `modules` option. |
There was a problem hiding this comment.
🟡 Comments on lines 2 and 19 of postcss.config.cjs reference rollup-plugin-postcss, but build-settings/rollup.config.mjs imports a different package — rollup-plugin-styles — and that's also what's listed in package.json devDependencies. Pure docs nit, no runtime impact, but will mislead a maintainer grepping for the plugin name. Suggested fix: s/rollup-plugin-postcss/rollup-plugin-styles/g in those two comment lines.
Extended reasoning...
What the bug is
The comment block at the top of postcss.config.cjs (introduced in this PR) names the wrong Rollup plugin in two places:
- Line 2:
// (rollup-plugin-postcssinbuild-settings/rollup.config.mjs). - Line 19:
// - Rollup usesrollup-plugin-postcss'smodulesoption.
The plugin actually wired in by this PR is rollup-plugin-styles, not rollup-plugin-postcss. These are two different npm packages by different authors (Anidetrix/rollup-plugin-styles vs. egoist/rollup-plugin-postcss) with different APIs and option shapes.
Proof — walk through the repo
build-settings/rollup.config.mjsline 8:import styles from "rollup-plugin-styles";package.jsonline 161 (in the diff):"rollup-plugin-styles": "^4.0.0"— listed indevDependencies.pnpm-lock.yamlresolvesrollup-plugin-styles@4.0.0(notrollup-plugin-postcss).- Grepping the repo for
rollup-plugin-postcssturns up only the two comment lines inpostcss.config.cjs— no actual import, no dependency entry, no usage.
So the comment is internally inconsistent with the very file (build-settings/rollup.config.mjs) it points at.
Impact
Zero runtime impact — it's a comment block. The harm is purely cognitive: a future maintainer reading postcss.config.cjs to understand how the chain is wired will look for rollup-plugin-postcss in the rollup config, won't find it, and will waste time. The line 19 reference (rollup-plugin-postcss's modules option) is also slightly misleading because both plugins happen to expose a modules option but with different shapes, so a maintainer cross-referencing docs could end up reading the wrong project's README.
Fix
s/rollup-plugin-postcss/rollup-plugin-styles/g in those two lines of postcss.config.cjs. No code change needed.
beaesguerra
left a comment
beaesguerra
left a comment
There was a problem hiding this comment.
First component to use CSS Modules, nice! Left some more questions 😄
| return ( | ||
| <StyledDiv | ||
| style={[getSize(size), style]} | ||
| className={styles[size]} |
There was a problem hiding this comment.
I was curious to see if the typecheck would catch if there was a mismatch between the class names and size names, and it did 🎉
I tested this by renaming the xlarge typed string to extraLarge and also tried renaming the class name. When I renamed the class name though, I had to re-run pnpm gen:css-types in order to update the types for the stlyes (which makes sense!).
Would it make sense to include pnpm gen:css-types as part of the pnpm dev script so that the css types are updated automatically? (Let me know too if there's already something that should be doing that!)
There was a problem hiding this comment.
It is already running as part of the prebuild script, which is invoked at some point by dev :)
There was a problem hiding this comment.
Oh cool! I was going to try it out again, but am running into errors now when running pnpm dev:
turbo 2.5.3
• Packages in scope: //, @khanacademy/eslint-plugin-wonder-blocks, @khanacademy/wb-codemod, @khanacademy/wb-dev-build-settings, @khanacademy/wonder-blocks-accordion, @khanacademy/wonder-blocks-announcer, @khanacademy/wonder-blocks-badge, @khanacademy/wonder-blocks-banner, @khanacademy/wonder-blocks-birthday-picker, @khanacademy/wonder-blocks-breadcrumbs, @khanacademy/wonder-blocks-button, @khanacademy/wonder-blocks-card, @khanacademy/wonder-blocks-cell, @khanacademy/wonder-blocks-clickable, @khanacademy/wonder-blocks-core, @khanacademy/wonder-blocks-data, @khanacademy/wonder-blocks-date-picker, @khanacademy/wonder-blocks-dropdown, @khanacademy/wonder-blocks-form, @khanacademy/wonder-blocks-grid, @khanacademy/wonder-blocks-icon, @khanacademy/wonder-blocks-icon-button, @khanacademy/wonder-blocks-labeled-field, @khanacademy/wonder-blocks-layout, @khanacademy/wonder-blocks-link, @khanacademy/wonder-blocks-modal, @khanacademy/wonder-blocks-pill, @khanacademy/wonder-blocks-popover, @khanacademy/wonder-blocks-progress-spinner, @khanacademy/wonder-blocks-search-field, @khanacademy/wonder-blocks-styles, @khanacademy/wonder-blocks-switch, @khanacademy/wonder-blocks-tabs, @khanacademy/wonder-blocks-testing, @khanacademy/wonder-blocks-testing-core, @khanacademy/wonder-blocks-theming, @khanacademy/wonder-blocks-timing, @khanacademy/wonder-blocks-tokens, @khanacademy/wonder-blocks-toolbar, @khanacademy/wonder-blocks-tooltip, @khanacademy/wonder-blocks-typography, eslint-plugin-wonder-blocks-demo
• Running build:css in 42 packages
• Remote caching disabled
× Failed to connect to daemon.
╰─▶ unable to connect to daemon after 3 retries
I made sure to run pnpm i first to make sure deps are up to date!
| output. Consumers using `@khanacademy/wonder-blocks-icon` automatically | ||
| pick up the bundled `dist/index.css` via a side-effect import in the | ||
| JS entry; SSR consumers that can't process CSS imports may need a CSS |
There was a problem hiding this comment.
Do consuming apps/libraries need to update anything in their project for these changes to work? If so, should this be considered a breaking change?
There was a problem hiding this comment.
Good question! this should be transparent for consumers :) as they are still importing the JS bundles that reference these CSS files.
There was a problem hiding this comment.
Sounds good!
Will there be a major version release for something in WB? Since consuming apps would need to set up postcss/bundling before using new versions of certain packages? Maybe just the styles package? Or maybe not since the new css mixin is additive!
There was a problem hiding this comment.
My plan is the following:
- Configure WB tooling
- Start migrating WB components
- Configure consumer repos tooling
- Install WB versions with CSS modules support in consumer repos
#3 can be done before #2, so I think minor/patch versions would work transparently for consumer apps as I'd expect that at that point (#4) all the tooling/infra should be configured in advance. Agree about the styles package, as mixins are additive, and I'm planning to configure mixins in consumer repos as part of #3 as well.
|
The integration with consumers can be seen here: https://github.com/Khan/frontend/pull/11572 This includes the WB For reference on the config, see: https://github.com/Khan/wonder-blocks/blob/WB-2324.1/postcss.config.cjs#L76 
cc @Khan/wonder-blocks @Khan/frontend-infra-web |
| return ( | ||
| <StyledDiv | ||
| style={[getSize(size), style]} | ||
| className={sizeStyles} |
There was a problem hiding this comment.
I wonder if you're going to want to add support for something like clsx https://www.npmjs.com/package/clsx to our addStyle logic so that we can handle cases like className={["class1", condition && "class2"]}.
There was a problem hiding this comment.
oh yeah, thanks for reminding me of that! I've considered clsx for that, but I'm also exploring other alternatives around that problem.
As a short term solution around the addStyle logic, my goal is to refactor the internals of that function so we can incrementally migrate to CSS modules using the existing style prop. You can see how this is going to work here: b1cb814#diff-56c6348fe785a54784ccb372f1a9f9f946e2520366035f7957f2b2177544fcd5
As for supporting libraries like clsx, I'm planning to write a new ADR to evaluate different tools: so far, I've contemplated clsx, classnames and cva.
You can see some research I quickly did with ChatGPT some weeks ago: https://chatgpt.com/share/e/69f3ab89-9440-800f-b0fc-a5ee94613fd5
…n to CSS Modules Wires the build side of the CSS Modules pipeline and migrates the first real component as the end-to-end proof. Closes the last remaining sub-task of Phase 0. Builds on top of WB-2324.4 (PR #3069); first PR in the chain that has visible runtime behavior changes. Reviewers: #frontend-infra-web Issue: WB-2324 1. `pnpm build` succeeds end-to-end. 2. `cat packages/wonder-blocks-icon/dist/index.css` shows the size rules wrapped in `@layer shared { … }` with hashed selectors (`wb-icon_module-small-<hash>`, etc.). 3. `head -3 packages/wonder-blocks-icon/dist/es/index.js` shows `import "./index.css";` as the first line; `head -3 …/dist/index.js` shows `require("./index.css");` after `'use strict'`. 4. `pnpm jest packages/wonder-blocks-icon` passes — Icon tests assert behavior (ref forwarding, ARIA, accessibility), not class names. 5. `pnpm typecheck` passes; the new `icon.module.css.d.ts` is generated by the existing `pretypecheck` hook. 6. `pnpm lint:css` clean (the spike CSS continues to pass; the new `icon.module.css` passes too). 7. (Manual) Open Storybook `Icon` stories: small / medium / large / xlarge variants render at the expected sizes, no visual regression vs. main. Please review these risky changes 1. 🚨 `build-settings/rollup.config.mjs`: adds `rollup-plugin-styles` to the plugin chain before `swc`. Emits per-package `dist/index.css` for any package that contains a `*.module.css` file. Injects a side-effect `import "./index.css"` (ESM) / `require("./index.css")` (CJS) at the top of those packages' JS bundles so npm consumers get styles automatically. SSR consumers that can't process CSS imports may need a loader / mock — standard webpack / Vite / Next.js setups handle this out of the box. 2. 🚨 `postcss.config.cjs`: adds a `wrap-in-layer` PostCSS plugin that moves every top-level rule into `@layer shared { … }`. Detects already-wrapped CSS and skips to avoid `@layer shared { @layer shared { … } }` nesting. Shared by Vite (Storybook) and Rollup (build) so authoring stays consistent. 3.⚠️ `packages/wonder-blocks-icon/package.json`: adds `exports` map (`.` + `./css`) and `sideEffects: ["**/*.css"]`. Published-surface change patterned after `wonder-blocks-tokens`. 4.⚠️ `packages/wonder-blocks-icon/src/components/icon.tsx`: migrates from `StyleSheet.create` + `getSize` to `className={styles[size]}`. The component still uses `addStyle("div")` so the `style` prop continues to accept Aphrodite `StyleType` values; only the *internal* size styles moved to CSS Modules. 5. 🔷 `packages/wonder-blocks-icon/src/components/icon.module.css` (new): the size classes themselves. Authors don't write `@layer shared { … }` here — the build wraps it. 6. 🔷 `__docs__/css-modules-spike/spike.module.css`: dropped the manual `@layer shared { … }` wrap since the build now does it. The PostCSS plugin's already-wrapped detection would have skipped re-wrapping anyway, but cleaning the source keeps authoring consistent across the codebase. 🤖 Built using Claude Code #ai-generated
3 participants
Summary
Wires the build side of the CSS Modules pipeline and migrates the first real component (
wonder-blocks-icon) as the end-to-end proof. Closes the last remaining sub-task of Phase 0.Builds on top of #3069 (Phase 0.4 — Stylelint). First PR in the chain that has visible runtime behavior changes.
Issue: WB-2324
Test plan
pnpm buildsucceeds end-to-end.cat packages/wonder-blocks-icon/dist/index.cssshows the size rules wrapped in@layer shared { … }with hashed selectors (.wbIcon_moduleSmall<hash>, etc.).head -3 packages/wonder-blocks-icon/dist/es/index.jsshowsimport "./index.css";as the first line;head -3 …/dist/index.jsshowsrequire("./index.css");after'use strict'.pnpm jest packages/wonder-blocks-iconpasses — Icon tests assert behavior (ref forwarding, ARIA, accessibility), not class names.pnpm typecheckpasses; the newicon.module.css.d.tsis generated by the existingpretypecheckhook.pnpm lint:cssclean — the newicon.module.csspassesmax-nesting-depth,color-no-hex,selector-class-pattern, andvalue-no-unknown-custom-propertiesfrom PR 🚨 [WB-2324.3] Phase 0.4 — Stylelint with token-strict-value + CI wiring #3069.Iconstories: small / medium / large / xlarge variants render at the expected sizes, no visual regression vs. main.Review plan
Please review these risky changes:
build-settings/rollup.config.mjs: addsrollup-plugin-stylesto the plugin chain beforeswc. Emits per-packagedist/index.cssfor any package that contains a*.module.cssfile. Injects a side-effectimport "./index.css"(ESM) /require("./index.css")(CJS) at the top of those packages' JS bundles so npm consumers get styles automatically. SSR consumers that can't process CSS imports may need a loader / mock — standard webpack / Vite / Next.js setups handle this out of the box.postcss.config.cjs: adds awrap-in-layerPostCSS plugin that moves every top-level rule into@layer shared { … }. Detects already-wrapped CSS and skips to avoid@layer shared { @layer shared { … } }nesting. Shared by Vite (Storybook) and Rollup (build) so authoring stays consistent.packages/wonder-blocks-icon/package.json: addsexportsmap (.+./css) andsideEffects: ["**/*.css"]. Published-surface change patterned afterwonder-blocks-tokens.packages/wonder-blocks-icon/src/components/icon.tsx: migrates fromStyleSheet.create+getSizetoclassName={styles[size]}. The component still usesaddStyle("div")so thestyleprop continues to accept AphroditeStyleTypevalues; only the internal size styles moved to CSS Modules.packages/wonder-blocks-icon/src/components/icon.module.css(new): the size classes themselves. Authors don't write@layer shared { … }here — the build wraps it.__docs__/css-modules-spike/spike.module.css: dropped the manual@layer shared { … }wrap since the build now does it. The PostCSS plugin's already-wrapped detection would have skipped re-wrapping anyway, but cleaning the source keeps authoring consistent across the codebase.cc: @Khan/frontend-infra-web
🤖 Built using Claude Code #ai-generated