Conversation
|
|
Triggered from #277 by @black7375. Checking if we can fast forward Target branch ( commit 26546b1a08cecbbaba4a4895d09657bdc7e7a633 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Oct 16 00:00:00 2025 +0900
Chore: update eslint config as consistPull request ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (pull_request/theme-bug-fixes)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valueIt is possible to fast forward |
WalkthroughThis pull request modifies the theme token system to accept Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Token Input
participant Pass1 as Pass 1: Descriptor<br/>& Nesting
participant Pass2 as Pass 2: Resolution<br/>& Vars
participant Output as Output
Input->>Pass1: ThemeTokensInput
Note over Pass1: For composite values:<br/>- Derive compositeDescriptors<br/>- Create nested composite<br/>- Store resolved getter<br/>- Per-property descriptors
Pass1->>Pass2: Intermediate descriptors
Note over Pass2: For composite resolved property:<br/>- Evaluate getter in context<br/>- Emit CSS variable via vars<br/>- Or set resolvedTokens directly
Pass2->>Output: var(--...) references<br/>& resolved tokens
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/css/src/theme/index.ts(54 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/css/src/theme/index.ts (2)
packages/css/src/utils.ts (3)
camelToKebab(31-33)getVarName(20-29)identifierName(5-11)packages/css/src/theme/types.ts (1)
TokenValue(140-140)
| if (context.aliasMap.has(resolvedPathKey)) { | ||
| setByPath(resolvedTokens, resolvedPath, computedValue); | ||
| } else { | ||
| const cssVar = getCSSVarByPath(varPath, context); | ||
| vars[cssVar] = extractCSSValue(computedValue as TokenValue); | ||
| } |
There was a problem hiding this comment.
Composite alias assignment throws at runtime.
Line 385 routes the alias result through setByPath, but that helper assigns into the resolved accessor we defined in Pass 1. When a composite token does this.alias(...), that branch runs and strict-mode assignment to a getter-only accessor throws TypeError: Cannot set property resolved of #<Object> which has only a getter>. Example:
const shadow = compositeValue({
get resolved() {
return this.alias(this.shadow.color);
},
color: "#000"
});
assignTokens({ shadow });We need to replace the accessor with a data property via Object.defineProperty instead of assigning through it.
- if (context.aliasMap.has(resolvedPathKey)) {
- setByPath(resolvedTokens, resolvedPath, computedValue);
- } else {
+ if (context.aliasMap.has(resolvedPathKey)) {
+ let compositeTarget = resolvedTokens as Record<string, unknown>;
+ for (const segment of resolvedPath.slice(0, -1)) {
+ compositeTarget = compositeTarget[segment] as Record<string, unknown>;
+ }
+
+ Object.defineProperty(compositeTarget, "resolved", {
+ value: computedValue,
+ configurable: true,
+ enumerable: true
+ });
+ } else {
const cssVar = getCSSVarByPath(varPath, context);
vars[cssVar] = extractCSSValue(computedValue as TokenValue);
}🤖 Prompt for AI Agents
In packages/css/src/theme/index.ts around lines 384 to 389, the branch that
handles alias results calls setByPath which attempts to assign into the existing
resolved accessor and throws when that accessor is getter-only; instead, detect
if the target property at resolvedPath on resolvedTokens is currently an
accessor (has only a getter) and replace it with a data property using
Object.defineProperty so the computedValue is stored as a normal
writable/enumerable/configurable value. Implement this by walking to the parent
object and key for resolvedPath, check Object.getOwnPropertyDescriptor on that
key, and if descriptor && (descriptor.get || !descriptor.writable), call
Object.defineProperty(parent, key, { value: computedValue, writable: true,
enumerable: true, configurable: true }) rather than using setByPath.
There was a problem hiding this comment.
@coderabbitai Please provide test cases in the comments.
There was a problem hiding this comment.
@black7375, here are comprehensive test cases for the composite alias assignment fix:
describe.concurrent("composite alias handling", () => {
it("handles alias in composite resolved getter without throwing", () => {
// This test verifies the bug fix: composite alias assignment should not throw
const result = assignTokens(
compositeValue({
shadow: {
get resolved() {
// Using alias() in a composite resolved getter should not throw
return this.alias(this.shadow.color);
},
color: "#00000080"
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
// Aliased composite resolved should not create a CSS variable
expect(normalized["--shadow"]).toBeUndefined();
// But the color property should still have a CSS variable
expect(normalized["--shadow-color"]).toBe("#00000080");
// The resolved property should reference the color variable
expect(normalizeResolvedTokens(result.resolvedTokens).shadow.resolved).toBe(
"var(--shadow-color)"
);
});
it("handles alias referencing another composite in resolved getter", () => {
const result = assignTokens(
compositeValue({
shadow: {
base: {
get resolved() {
return `${this.shadow.base.color} ${this.shadow.base.blur}`;
},
color: "#00000080",
blur: { value: 1, unit: "rem" }
},
themed: {
get resolved() {
// Alias the entire base shadow
return this.alias(this.shadow.base.resolved);
},
get color() {
return this.shadow.base.color;
},
get blur() {
return this.shadow.base.blur;
}
}
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
// Base shadow should have CSS variable for resolved
expect(stripHash(normalized["--shadow-base"])).toBeDefined();
// Themed shadow resolved should not (it's aliased)
expect(normalized["--shadow-themed"]).toBeUndefined();
// Both should reference the base resolved variable
const normalizedTokens = normalizeResolvedTokens(result.resolvedTokens);
expect(normalizedTokens.shadow.base.resolved).toBe("var(--shadow-base)");
expect(normalizedTokens.shadow.themed.resolved).toBe("var(--shadow-base)");
});
it("handles mixed alias and non-alias properties in composite", () => {
const result = assignTokens(
compositeValue({
button: {
primary: {
get resolved() {
return `${this.button.primary.bg} ${this.button.primary.border}`;
},
bg: "#007bff",
border: { value: 1, unit: "px" }
},
secondary: {
get resolved() {
// Non-aliased: creates CSS var
return `${this.button.secondary.bg} ${this.button.secondary.border}`;
},
get bg() {
// Aliased: references primary
return this.alias(this.button.primary.bg);
},
border: { value: 1, unit: "px" }
}
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
// Primary should have all CSS variables
expect(normalized["--button-primary-bg"]).toBe("#007bff");
expect(normalized["--button-primary-border"]).toBe("1px");
expect(stripHash(normalized["--button-primary"])).toBeDefined();
// Secondary bg should not have CSS var (aliased)
expect(normalized["--button-secondary-bg"]).toBeUndefined();
// But border should
expect(normalized["--button-secondary-border"]).toBe("1px");
// And resolved should (not aliased)
expect(stripHash(normalized["--button-secondary"])).toBeDefined();
const normalizedTokens = normalizeResolvedTokens(result.resolvedTokens);
expect(normalizedTokens.button.secondary.bg).toBe(
"var(--button-primary-bg)"
);
});
it("handles nested composite with alias at multiple levels", () => {
const result = assignTokens(
compositeValue({
theme: {
light: {
shadow: {
get resolved() {
return `${this.theme.light.shadow.color} ${this.theme.light.shadow.blur}`;
},
color: "#00000020",
blur: { value: 0.5, unit: "rem" }
}
},
dark: {
shadow: {
get resolved() {
// Alias the light theme's shadow blur
return `${this.theme.dark.shadow.color} ${this.alias(
this.theme.light.shadow.blur
)}`;
},
color: "#ffffff40",
get blur() {
// Also alias at property level
return this.alias(this.theme.light.shadow.blur);
}
}
}
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
// Light theme should have all vars
expect(normalized["--theme-light-shadow-color"]).toBe("#00000020");
expect(normalized["--theme-light-shadow-blur"]).toBe("0.5rem");
// Dark theme color should have var
expect(normalized["--theme-dark-shadow-color"]).toBe("#ffffff40");
// But blur should not (aliased)
expect(normalized["--theme-dark-shadow-blur"]).toBeUndefined();
const normalizedTokens = normalizeResolvedTokens(result.resolvedTokens);
expect(normalizedTokens.theme.dark.shadow.blur).toBe(
"var(--theme-light-shadow-blur)"
);
});
it("handles composite with raw and alias together", () => {
const result = assignTokens(
compositeValue({
elevation: {
base: {
get resolved() {
return `${this.elevation.base.color} ${this.elevation.base.blur}`;
},
color: "#00000033",
blur: { value: 1, unit: "rem" }
},
inset: {
get resolved() {
// Mix raw and alias
return `inset ${this.raw(this.elevation.base.color)} ${this.alias(
this.elevation.base.blur
)}`;
},
get color() {
return this.raw(this.elevation.base.color);
},
get blur() {
return this.alias(this.elevation.base.blur);
}
}
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
// Inset color should have raw value
expect(normalized["--elevation-inset-color"]).toBe("#00000033");
// Inset blur should not exist (aliased)
expect(normalized["--elevation-inset-blur"]).toBeUndefined();
const normalizedTokens = normalizeResolvedTokens(result.resolvedTokens);
expect(normalizedTokens.elevation.inset.color).toBe("#00000033");
expect(normalizedTokens.elevation.inset.blur).toBe(
"var(--elevation-base-blur)"
);
});
it("handles self-referential composite with alias", () => {
const result = assignTokens(
compositeValue({
spacing: {
base: {
get resolved() {
return `${this.spacing.base.top} ${this.spacing.base.right}`;
},
top: { value: 1, unit: "rem" },
right: { value: 2, unit: "rem" }
},
symmetric: {
get resolved() {
// Reference own properties with alias
return `${this.spacing.symmetric.value} ${this.alias(
this.spacing.symmetric.value
)}`;
},
value: { value: 1.5, unit: "rem" }
}
}
})
);
validateHashFormatForVar(result.vars);
validateHashFormatForResolved(result.resolvedTokens);
const normalized = normalizeVars(result.vars);
expect(normalized["--spacing-symmetric-value"]).toBe("1.5rem");
const normalizedTokens = normalizeResolvedTokens(result.resolvedTokens);
expect(normalizedTokens.spacing.symmetric.value).toBe(
"var(--spacing-symmetric-value)"
);
});
});These test cases cover:
- Basic composite alias - Verifies the core bug fix where alias in resolved getter doesn't throw
- Cross-composite alias - Tests aliasing between different composite objects
- Mixed alias/non-alias - Ensures alias and normal references can coexist in one composite
- Nested composites - Validates alias works at multiple nesting levels
- Raw + alias combination - Tests that raw() and alias() can be used together
- Self-referential alias - Edge case where composite aliases its own properties
Each test validates both the CSS variable generation and the resolved token structure to ensure correctness.
There was a problem hiding this comment.
Basically, I'am only considering it within the get method. I'll be building a separate behavior for compositeValue, which separates the scope.
There was a problem hiding this comment.
@black7375, thank you for clarifying your approach! I understand you're planning to handle compositeValue with a separate behavior that has its own scope separation, rather than addressing it within the current getter-based flow.
That makes sense architecturally—separating concerns between regular getter-based semantic tokens and composite values with their own evaluation context. This would likely provide cleaner semantics and avoid the conflicts between accessor properties and value assignments.
If you'd like, I can help when you're ready to implement the separate compositeValue behavior. For example, I could:
- Review the implementation once you've sketched it out
- Help generate test cases for the new separate scope behavior
- Validate that the new approach handles the alias/raw/fallbackVar utilities correctly within the composite context
Just let me know when you're working on that part, and feel free to tag me if you'd like input on the design or implementation! 🚀
|
/fast-forward |
|
Triggered from #277 (comment) by @black7375. Trying to fast forward Target branch ( commit 26546b1a08cecbbaba4a4895d09657bdc7e7a633 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Oct 16 00:00:00 2025 +0900
Chore: update eslint config as consistPull request ( commit 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff (pull_request/theme-bug-fixes)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 18 00:00:00 2025 +0900
Fix: `theme` - composite valueFast forwarding $ git push origin 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff:main
To https://github.com/mincho-js/mincho.git
26546b1..236463b 236463ba62ad15c5ebb0bda3dc89e9d1e36e4eff -> main |
1 participant
Description
fix theme implement with more test codes.
Related Issue
Summary by CodeRabbit
Refactor
Tests
Additional context
Checklist