Handle backwards compatibility for content theme from JS configs
#19381
Conversation
WalkthroughThe patch updates the Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
integrations/upgrade/js-config.test.ts (1)
1843-1923: Solid regression coverage for migratedtheme.contentutilitiesThis test exercises the full upgrade path: JS config with
theme.content, HTML usingafter:content-*classes, and the upgraded CSS defining@utility content-*with--tw-contentandcontent: var(--tw-content). That directly guards against the originalcontent-slash/linting regression. As an optional enhancement, you could add a similar case wherecontentlives undertheme.extendto make that behavior explicit in the test suite, even thoughresolveConfigshould already normalize it.packages/tailwindcss/src/compat/content-compat.test.ts (1)
1-33: Runtime compat test correctly validatescontent-*utility generationThe test cleanly verifies that a JS config with
theme.content.slashproduces acontent-slashutility with the expected--tw-content+content: var(--tw-content)declarations via the compat path. As a minor enhancement, you could add an entry with a non‑string value (e.g.content: { slash: '"/"', foo: 42 }) to assert that non‑string values are skipped, matching the guard inregisterContentCompat.packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts (1)
7-7: Content migration logic is correct but could mirror compat guards for robustnessThe new
theme.contentmigration block correctly:
- Converts each key into a
@utility content-<key>rule using--tw-content+content: var(--tw-content).- Emits them in the
utilitybucket and removestheme.contentso it’s not also turned into--content-*vars.That lines up with the comment about no longer reading from
--content-*in v4 and with the runtime behavior inregisterContentCompat. Two small tweaks would make this more robust and consistent with the compat path:
- Guard the shape of
resolvedConfig.theme.content
Right now theif ('content' in resolvedConfig.theme)check will happily accept non‑object values;Object.entrieson a string, for example, will “work” but produce nonsense utilities. Mirroring the compat check would avoid that:- if ('content' in resolvedConfig.theme) { - let rules: AstNode[] = [] - - for (let [key, value] of Object.entries(resolvedConfig.theme.content)) { - rules.push( - atRule('@utility', `content-${key}`, [ - decl('--tw-content', `${value}`), - decl('content', `var(--tw-content)`), - ]), - ) - } + if ( + 'content' in resolvedConfig.theme && + typeof resolvedConfig.theme.content === 'object' && + resolvedConfig.theme.content !== null + ) { + let rules: AstNode[] = [] + + for (let [key, value] of Object.entries(resolvedConfig.theme.content)) { + if (typeof value !== 'string') continue + + rules.push( + atRule('@utility', `content-${key}`, [ + decl('--tw-content', value), + decl('content', 'var(--tw-content)'), + ]), + ) + }
- Filter to string values only
As shown above, filtering non‑string values matchesregisterContentCompatand avoids generating obviously invalidcontentrules from misconfigured theme values.These are defensive improvements; the current implementation will work for the intended, string‑only
theme.contentuse case.Also applies to: 145-166
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
integrations/upgrade/js-config.test.ts(1 hunks)packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts(2 hunks)packages/tailwindcss/src/compat/apply-compat-hooks.ts(2 hunks)packages/tailwindcss/src/compat/content-compat.test.ts(1 hunks)packages/tailwindcss/src/compat/content-compat.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/tailwindcss/src/compat/content-compat.ts (2)
packages/tailwindcss/src/design-system.ts (1)
DesignSystem(33-65)packages/tailwindcss/src/ast.ts (1)
decl(95-102)
packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts (1)
packages/tailwindcss/src/ast.ts (3)
AstNode(68-68)atRule(78-85)decl(95-102)
packages/tailwindcss/src/compat/apply-compat-hooks.ts (1)
packages/tailwindcss/src/compat/content-compat.ts (1)
registerContentCompat(5-18)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Linux
- GitHub Check: Linux / postcss
- GitHub Check: Linux / vite
- GitHub Check: Linux / upgrade
- GitHub Check: Linux / webpack
- GitHub Check: Linux / cli
🔇 Additional comments (2)
packages/tailwindcss/src/compat/apply-compat-hooks.ts (1)
13-13: Content compat wiring aligns with existing compat hooksImporting and invoking
registerContentCompatalongsideregisterContainerCompat/registerScreensConfigusingresolvedUserConfigkeeps all JS-config‑driven compat in one place and ensurestheme.contentfrom JS configs registerscontent-*utilities during compat compilation. The integration matches the existing pattern and looks solid.Also applies to: 362-362
packages/tailwindcss/src/compat/content-compat.ts (1)
1-18: Content compat helper correctly mapstheme.contenttocontent-*utilities
registerContentCompatsensibly guardstheme.content’s shape, skips non‑string values, and registerscontent-<key>statics that set--tw-contentpluscontent: var(--tw-content). That aligns with the migration behavior and the new tests, and integrates cleanly with the existingDesignSystem.utilities.staticpattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/tailwindcss/src/compat/content-compat.ts (2)
8-8: Add array check to type validation.Arrays are also objects in JavaScript, so the current check allows arrays to pass through. If
theme.contentis an array,Object.entries()would use numeric indices as keys, resulting in utilities likecontent-0,content-1, which is likely unintended.Apply this diff:
- if (typeof content !== 'object' || content === null) return + if (typeof content !== 'object' || content === null || Array.isArray(content)) return
17-23: Consider defining@propertyonce to avoid duplication.Each
content-*utility registers the same@property --tw-contentdeclaration. If multiple content utilities are used (e.g.,content-slashandcontent-dot), the CSS output will contain duplicate@propertyblocks, causing CSS bloat.Consider one of these approaches:
- Define the
@propertyglobally once whenregisterContentCompatis called, outside the per-utility registration- Use a lazy initialization pattern to register the
@propertyonly on first useThis would reduce redundant CSS output while maintaining the same functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
integrations/upgrade/js-config.test.ts(1 hunks)packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts(2 hunks)packages/tailwindcss/src/compat/content-compat.test.ts(1 hunks)packages/tailwindcss/src/compat/content-compat.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tailwindcss/src/compat/content-compat.test.ts
- packages/@tailwindcss-upgrade/src/codemods/config/migrate-js-config.ts
- integrations/upgrade/js-config.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Linux
- GitHub Check: Linux / upgrade
| @utility content-slash { | ||
| --tw-content: '/'; | ||
| content: var(--tw-content); | ||
| } | ||
| @utility content-slashslash { | ||
| --tw-content: '//'; | ||
| content: var(--tw-content); | ||
| } | ||
| @property --tw-content { | ||
| syntax: '*'; | ||
| inherits: false; | ||
| initial-value: ''; | ||
| } |
There was a problem hiding this comment.
It does expose a little bit of internal information, but I don't hate this.
As mentioned before, reading from --content could also work. Not sure what's best 🤔
There was a problem hiding this comment.
Yeah, those are our two options. The one where we read from --content is a much smaller change. So it depends on which one we want to do.
thecrypticace
force-pushed
the
fix/issue-19343
branch
from
f7762cc to
2fc57c2
Compare
|
I've updated this to read from |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/tailwindcss/src/utilities.test.ts (1)
23261-23275: LGTM! Test setup validates core theme-based content functionality.The test correctly validates the new behavior where
contentutility supports theme keys. The setup covers both theme-based values (content-slash) and arbitrary values (content-["hello_world"]), which aligns with the PR objectives for handling migration and backwards compatibility.Consider adding test coverage for variants to ensure the full migration path works as expected:
// Example of additional test coverage ['after:content-slash', 'before:content-slash', 'content-slash']This would validate that the utilities work correctly with pseudo-element variants, which was part of the original issue context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/tailwindcss/src/utilities.test.ts(2 hunks)packages/tailwindcss/src/utilities.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/tailwindcss/src/utilities.test.ts (2)
packages/tailwindcss/src/test-utils/run.ts (1)
compileCss(4-11)integrations/utils.ts (1)
css(519-519)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Linux
- GitHub Check: Linux / vite
- GitHub Check: Linux / cli
- GitHub Check: Linux / upgrade
- GitHub Check: Linux / postcss
🔇 Additional comments (2)
packages/tailwindcss/src/utilities.ts (1)
4691-4693: LGTM! Backwards compatibility support for theme-based content values.The change correctly enables theme resolution for the
contentutility to support migration from v3 configs wheretheme.contentvalues were defined. The comment appropriately documents this as a backwards compatibility feature and recommends arbitrary values for new usage.packages/tailwindcss/src/utilities.test.ts (1)
23281-23297: LGTM! Expected output correctly validates backwards compatibility.The expected output demonstrates that:
- Theme variable
--content-slashis properly hoisted to:root, :host- The
content-slashutility generates valid CSS usingvar(--content-slash)via the--tw-contentvariable- Arbitrary values like
content-["hello_world"]work correctly with proper escaping and space handling- The
@property --tw-contentdeclaration ensures proper CSS variable behaviorThis aligns perfectly with the PR objectives, ensuring that migrated
contenttheme entries produce valid utility classes.
thecrypticace
changed the title
content theme from JS configscontent theme from JS configs
Fixes #19343
This PR makes it so the
content-*utilities read from the--content-*theme namespace. This change is purely for backwards compatibility with Tailwind CSS v3. It is recommended you use arbitrary values with thecontent-*utility instead.