Feat: theme() base API implement #260#261
Conversation
|
|
Triggered from #261 by @black7375. Checking if we can fast forward Target branch ( commit 51133c5b8bc0f9e42d2fe8172000a9e5e6487a75 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sun Sep 14 00:00:00 2025 +0900
Chore: refactor dependency ciPull request ( commit c1226f284266fc1d0fa9865e486dd8f2a3a7ab9e (pull_request/theme-base-api)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Sep 16 00:00:00 2025 +0900
Feat: `theme()` support dtcg format #260It is possible to fast forward |
WalkthroughReplaces vanilla-extract's theme creation functions with a new custom theming system using CSS variables. Introduces globalTheme and theme functions, comprehensive token type definitions supporting color, dimension, typography, animation, and composite tokens, and adds a camelToKebab utility for CSS variable naming conventions. Changes
Sequence DiagramsequenceDiagram
participant User
participant theme as theme()
participant globalTheme as globalTheme()
participant CSS as globalCss()
participant VE as Vanilla Extract
User->>theme: theme(tokens, debugId?)
activate theme
theme->>theme: Generate unique className
theme->>globalTheme: Call with selector=className
deactivate theme
activate globalTheme
globalTheme->>globalTheme: assignTokens(tokens)<br/>Extract CSS vars & resolve values
globalTheme->>globalTheme: Handle @layer if present
globalTheme->>CSS: Apply global CSS rule<br/>with vars & selectors
CSS->>VE: Register CSS rule
globalTheme-->>theme: Return resolvedTokens
deactivate globalTheme
theme-->>User: Return [className, tokens]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces substantial new logic across multiple files including a complete theming system with recursive token resolution, CSS variable generation, type definitions for diverse token categories, and test scaffolding. While individual functions follow clear patterns, the overall architectural scope and interaction between components require careful verification. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/css/src/utils.ts (1)
31-33: Deduplicate camelToKebab across packages and align vendor prefix handling.This implementation is correct, but we now have similar logic in transform-to-vanilla. Consider centralizing to a shared util to avoid drift and add cases like "MsTransform" → "-ms-transform" to mirror your kebabToCamel special-case.
packages/css/src/theme/index.ts (3)
132-141: Loosen TokenDimensionValue guard (avoid brittle key-count check).Object.keys(value).length === 2 can misclassify valid objects (e.g., metadata). Check shape only.
function isTokenUnitValue(value: unknown): value is TokenDimensionValue { - return ( - typeof value === "object" && - value != null && - "value" in value && - "unit" in value && - typeof value.value === "number" && - typeof value.unit === "string" && - Object.keys(value).length === 2 - ); + return ( + typeof value === "object" && + value != null && + "value" in (value as any) && + "unit" in (value as any) && + typeof (value as any).value === "number" && + typeof (value as any).unit === "string" + ); }
210-226: Color extractor is too generic for modern color spaces.Commas won’t be valid for color() spaces (e.g., srgb, oklch). Prefer:
- rgb()/hsl(): comma or space forms
- others:
color(${space} ${components.join(' ')} / alpha)
Add tests for TokenColorValue objects.
281-283: dimension case calls extractDurationValue with wrong type; add dedicated extractor.Types mismatch; create an extractDimensionValue and use it here.
- case "dimension": - // TokenDimensionValue is already handled by existing logic - return extractDurationValue($value as TokenDurationValue); // Same structure + case "dimension": + return extractDimensionValue($value as TokenDimensionValue);Add near other extractors:
function extractDimensionValue(value: TokenDimensionValue): CSSVarValue { return `${value.value}${value.unit}` as CSSVarValue; }packages/css/src/theme/types.ts (1)
50-64: Consider narrowing units to specific string unions.TokenDimensionValue.unit and TokenDurationValue.unit are plain string; narrowing to "px" | "rem" and "ms" | "s" would improve safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/css/src/index.ts(1 hunks)packages/css/src/theme/index.ts(1 hunks)packages/css/src/theme/types.ts(1 hunks)packages/css/src/utils.ts(1 hunks)packages/transform-to-vanilla/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/css/src/utils.ts (2)
packages/css-additional-types/codegen/utils.ts (1)
kebabToCamel(2-13)packages/transform-to-vanilla/src/utils/string.ts (2)
camelToKebab(2-4)convertToCSSVar(6-10)
packages/css/src/theme/index.ts (4)
packages/css/src/theme/types.ts (12)
Theme(9-11)ResolveTheme(152-158)TokenPrimitiveValue(148-148)TokenDimensionValue(55-64)TokenCompositeValue(139-142)TokenDefinition(19-23)ThemeValue(13-17)TokenFontFamilyValue(71-71)TokenDurationValue(114-123)TokenColorValue(30-47)TokenFontWeightValue(78-107)TokenValue(138-138)packages/css/src/types.ts (1)
Resolve(1-3)packages/transform-to-vanilla/src/types/style-rule.ts (3)
PureCSSVarKey(117-117)PureCSSVarFunction(122-124)CSSVarValue(119-119)packages/css/src/utils.ts (1)
camelToKebab(31-33)
packages/css/src/theme/types.ts (2)
packages/css/src/types.ts (1)
Resolve(1-3)packages/transform-to-vanilla/src/types/style-rule.ts (1)
PureCSSVarFunction(122-124)
🔇 Additional comments (2)
packages/css/src/index.ts (1)
42-60: Review comment is based on incorrect assumptions; no breaking changes detected.The code snippet (lines 42-60) shows custom mincho-js theming exports (
globalTheme,theme), not removal of vanilla-extract theme utilities. The vanilla-extract functions likecreateTheme,createGlobalTheme, etc. are not exported from the mainindex.tsand were never removed—they remain available incompat.ts. No codebase usage expects these functions from@mincho-js/css, and no breaking change occurs here.Likely an incorrect or invalid review comment.
packages/transform-to-vanilla/src/index.ts (1)
9-12: Type alias re-exports look fine; confirm upstream types exist.Aliasing NonNullableString/NonNullableNumber to “Strict” is clear. Just ensure @mincho-js/csstype indeed exports both names to avoid CI/type breaks.
| export function theme<ThemeTokens extends Theme>( | ||
| tokens: WithOptionalLayer<ThemeTokens>, | ||
| debugId?: string | ||
| ): [string, Resolve<ResolveTheme<ThemeTokens>>] { | ||
| const themeClassName = generateIdentifier(debugId); | ||
| registerClassName(themeClassName, getFileScope()); | ||
|
|
||
| const resolvedTokens = globalTheme(themeClassName, tokens); | ||
|
|
||
| return [themeClassName, resolvedTokens]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
theme(): ensure selector format passed to globalTheme is correct.
You pass the raw class name. If globalCss expects a CSS selector, this should be .${themeClassName}.
🏁 Script executed:
#!/bin/bash
# Inspect globalCss signature and usage to confirm expected selector format
rg -nP -C3 'export\s+function\s+globalCss\s*\(\s*([a-zA-Z_$][\w$]*)\s*:' packages/css/src/css
rg -nP -C2 '\bglobalCss\s*\(' --type=tsLength of output: 4012
---
🏁 Script executed:
```shell
#!/bin/bash
# Find globalTheme function definition and understand its selector parameter
rg -nP 'export\s+function\s+globalTheme\s*\(' packages/css/src --type=ts -A5
# Also look for how selector is used inside globalTheme
rg -nP '\bglobalTheme\s*\(' packages/css/src --type=ts -B2 -A2
Length of output: 334
---
🏁 Script executed:
```shell
#!/bin/bash
# Check imports and broader search for globalTheme
head -20 packages/css/src/theme/index.ts
rg -nP 'globalTheme' packages/css/src --type=ts -B1 -A3
Length of output: 1465
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full globalTheme implementation
sed -n '32,54p' packages/css/src/theme/index.ts
Length of output: 555
theme(): format the class name as a CSS selector before passing to globalTheme.
The themeClassName is a generated identifier like "m1a2b3c", which is not a valid CSS selector. It should be passed as .${themeClassName} to properly target the class. The selector is passed through globalTheme() directly to globalCss(), which expects valid CSS selector syntax.
🤖 Prompt for AI Agents
In packages/css/src/theme/index.ts around lines 56–66, the generated
themeClassName (e.g. "m1a2b3c") is not a valid CSS selector when passed to
globalTheme; prefix the class name with a dot (i.e. pass `.${themeClassName}` to
globalTheme) while keeping registerClassName(themeClassName, ...) unchanged so
registration uses the raw identifier, and ensure you don't double-prefix if the
value is already a selector.
| for (const [propKey, propValue] of Object.entries(value)) { | ||
| if (propKey === "resolved") continue; | ||
|
|
||
| const propPath = `${varPath}-${camelToKebab(propKey)}`; | ||
| const propCssVar = pathToCSSVar(propPath); | ||
| vars[propCssVar] = extractCSSValue(propValue as TokenValue); | ||
| resolvedComposite[propKey] = pathToVarReference(propPath); | ||
| } |
There was a problem hiding this comment.
Arrays inside TokenCompositeValue properties will throw. Handle them explicitly.
Currently extractCSSValue(array) throws. Support arrays on composite props like you do for top-level arrays.
- for (const [propKey, propValue] of Object.entries(value)) {
+ for (const [propKey, propValue] of Object.entries(value)) {
if (propKey === "resolved") continue;
-
- const propPath = `${varPath}-${camelToKebab(propKey)}`;
- const propCssVar = pathToCSSVar(propPath);
- vars[propCssVar] = extractCSSValue(propValue as TokenValue);
- resolvedComposite[propKey] = pathToVarReference(propPath);
+ const propPath = `${varPath}-${camelToKebab(propKey)}`;
+ if (Array.isArray(propValue)) {
+ // index each array item
+ propValue.forEach((item, idx) => {
+ const indexedPath = `${propPath}-${idx}`;
+ const cssVar = pathToCSSVar(indexedPath);
+ vars[cssVar] = extractCSSValue(item as TokenValue);
+ });
+ // Expose array of var()s to mirror shape
+ resolvedComposite[propKey] = propValue.map((_, idx) =>
+ pathToVarReference(`${propPath}-${idx}`)
+ ) as unknown as PureCSSVarFunction;
+ } else {
+ const propCssVar = pathToCSSVar(propPath);
+ vars[propCssVar] = extractCSSValue(propValue as TokenValue);
+ resolvedComposite[propKey] = pathToVarReference(propPath);
+ }
}Add tests for arrays within composites.
| type ResolveCompositeValue<T extends TokenCompositeValue> = Resolve< | ||
| { resolved: PureCSSVarFunction } & { | ||
| [K in Exclude<keyof T, "resolved">]: PureCSSVarFunction; | ||
| } | ||
| >; |
There was a problem hiding this comment.
Make resolved readonly in ResolveCompositeValue to match getter semantics.
Aligns with TokenCompositeValue’s getter and your test expectations.
-type ResolveCompositeValue<T extends TokenCompositeValue> = Resolve<
- { resolved: PureCSSVarFunction } & {
+type ResolveCompositeValue<T extends TokenCompositeValue> = Resolve<
+ { readonly resolved: PureCSSVarFunction } & {
[K in Exclude<keyof T, "resolved">]: PureCSSVarFunction;
}
>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type ResolveCompositeValue<T extends TokenCompositeValue> = Resolve< | |
| { resolved: PureCSSVarFunction } & { | |
| [K in Exclude<keyof T, "resolved">]: PureCSSVarFunction; | |
| } | |
| >; | |
| type ResolveCompositeValue<T extends TokenCompositeValue> = Resolve< | |
| { readonly resolved: PureCSSVarFunction } & { | |
| [K in Exclude<keyof T, "resolved">]: PureCSSVarFunction; | |
| } | |
| >; |
🤖 Prompt for AI Agents
In packages/css/src/theme/types.ts around lines 180 to 184, the
ResolveCompositeValue type should mark the resolved property as readonly to
match the getter semantics on TokenCompositeValue and tests; change the
intersection so that the resolved property is declared readonly (e.g. readonly
resolved: PureCSSVarFunction) while leaving the other indexed properties as-is.
|
/fast-forward |
|
Triggered from #261 (comment) by @black7375. Trying to fast forward Target branch ( commit 51133c5b8bc0f9e42d2fe8172000a9e5e6487a75 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sun Sep 14 00:00:00 2025 +0900
Chore: refactor dependency ciPull request ( commit c1226f284266fc1d0fa9865e486dd8f2a3a7ab9e (pull_request/theme-base-api)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Sep 16 00:00:00 2025 +0900
Feat: `theme()` support dtcg format #260Fast forwarding $ git push origin c1226f284266fc1d0fa9865e486dd8f2a3a7ab9e:main
To https://github.com/mincho-js/mincho.git
51133c5..c1226f2 c1226f284266fc1d0fa9865e486dd8f2a3a7ab9e -> main |
1 participant
Description
theme base api
Related Issue
Summary by CodeRabbit
New Features
Refactor
Additional context
Checklist