Don't crash when theme.keyframes is explicitly set to null - #20425
Don't crash when theme.keyframes is explicitly set to null#20425koreahghg wants to merge 2 commits into
Conversation
Every sibling module (container.ts, screens-config.ts, etc.) defensively falls back to an empty object when a resolved theme namespace is missing or nullish. `keyframesToRules` used `'keyframes' in resolvedConfig.theme` instead, which is true even when the value itself is `null` (the documented way to clear a theme key), so `Object.entries(null)` threw and crashed the whole build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "Correct changelog wording and add extend..." | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe keyframe conversion guard now processes Merge Risk: ⚪ Minimal · up to This localized fix prevents builds from crashing when theme.keyframes is explicitly null, with regression coverage and no actionable merge-blocking risk remaining after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0619246-a2f1-47bf-8ae7-86a5a15c392e
📒 Files selected for processing (3)
CHANGELOG.mdpackages/tailwindcss/src/compat/apply-keyframes-to-theme.test.tspackages/tailwindcss/src/compat/apply-keyframes-to-theme.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
theme.extend.keyframes: null never actually crashed: deepMerge skips
null/undefined sources entirely, so the resolved value there is `{}`,
not `null`. Drop the inaccurate "(or theme.extend.keyframes)" changelog
claim and add a dedicated test for that path so a future change to
deepMerge/resolveConfig can't silently reintroduce a crash there too.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the review! Re: the suggestion to also cover `theme.extend.keyframes: null` — good catch that it wasn't covered, but worth noting that path never actually hit the crash: `deepMerge` skips `null`/`undefined` sources entirely (see `compat/config/deep-merge.ts`), so the resolved value there is always `{}`, not `null`. I've pushed a follow-up commit that:
Full suite still green (5009/5010, 1 pre-existing skip). |
Summary
Setting
theme.keyframes(ortheme.extend.keyframes) tonullin a JS config crashes the entire build.keyframesToRulesinpackages/tailwindcss/src/compat/apply-keyframes-to-theme.tschecks'keyframes' in resolvedConfig.themebefore callingObject.entries(resolvedConfig.theme.keyframes). ButresolveConfig(seepackages/tailwindcss/src/compat/config/resolve-config.ts) resolves every theme value withvalue ?? null, so a theme key explicitly set tonull— the documented way to clear a theme key, e.g.theme.extend.colors: null— still exists on the object viain, just with anullvalue. That meanstheme: { keyframes: null }reachesObject.entries(null), which throwsTypeError: Cannot convert undefined or null to objectand crashes the whole build.Every sibling module that reads from the resolved theme this way (
container.ts,screens-config.ts, etc.) already guards against this with a falsy/|| {}check — this file was the one outlier still usingin.Fix
Swap the
'keyframes' in resolvedConfig.themecheck for a truthy check onresolvedConfig.theme.keyframesitself, matching the pattern used elsewhere incompat/.Test plan
apply-keyframes-to-theme.test.ts:theme: { keyframes: null }no longer throws and correctly results in no keyframes.pnpm vitest run— all 5008 tests in thetailwindcsspackage pass (5007 existing + 1 new), no regressions.tsc --noEmitshows no new type errors introduced by this change (pre-existing, unrelated errors exist elsewhere in the monorepo from packages that require a built@tailwindcss/oxidenative module).