Feat: theme - Support alias method#272
Conversation
🦋 Changeset detectedLatest commit: 4a185b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 |
|
Triggered from #272 by @black7375. Checking if we can fast forward Target branch ( commit 50145b3eca703e9884113b58eecd0c6ecb8631d3 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Sep 25 00:00:00 2025 +0900
Feat: `theme` - Support `raw` method #268Pull request ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (pull_request/theme-alias-api)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270It is possible to fast forward |
WalkthroughA changeset documents a minor version bump for Changes
Sequence Diagram(s)sequenceDiagram
participant Pass1 as Pass 1: Token Marking
participant Context as TokenProcessingContext
participant Pass2 as Pass 2: Token Resolution
participant Output as CSS Output
Pass1->>Context: Process semantic token getter
Pass1->>Context: Call alias(varReference) if applicable
Context->>Context: Mark path in aliasMap
Pass1->>Context: Store currentProcessingPath
Pass2->>Context: Resolve semantic token
Context->>Context: Check if path in aliasMap
alt Is Alias
Context->>Output: Skip CSS variable creation
Context->>Output: Use unhashed token reference
else Not Alias
Context->>Output: Create CSS variable
Context->>Output: Set var() reference
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ 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
🧹 Nitpick comments (2)
packages/css/src/theme/index.ts (2)
541-557: Consider refactoring to avoid module-level mutable state.The module-level
currentProcessingPathvariable works for the current synchronous execution model but is a code smell that could cause issues if the code is ever called in parallel contexts or if execution order changes. Additionally, there's no validation thatalias()is called within the correct context (inside a getter during pass 2).Consider passing the current path through the context instead. For example, extend the context to include a
getCurrentPath()function that's only available during getter execution:interface TokenProcessingContext { prefix: string; path: string[]; parentPath: string; cssVarMap: CSSVarMap; aliasMap: Set<string>; currentPath?: string[]; // Add current processing path to context } function createAliasFunction(context: TokenProcessingContext) { return function alias(varReference: unknown): string { // Validate that we're in a getter context if (!context.currentPath) { throw new Error('alias() can only be called within a token getter'); } const currentPathStr = context.currentPath.join("."); context.aliasMap.add(currentPathStr); return String(varReference); }; }Then update the getter execution in
resolveSemanticTokens:if (typeof descriptor.get === "function") { // Set the current path in context const updatedContext = { ...context, currentPath }; try { const computedValue = descriptor.get.call(resolvedTokens); // ... rest of logic } finally { // Context is naturally cleaned up as it goes out of scope } continue; }However, this would require passing the context to the getter functions, which may not fit the current API design. The current approach is acceptable for now.
541-557: Consider adding JSDoc for the alias mechanism.The
createAliasFunctionandcurrentProcessingPathpattern could benefit from more detailed documentation explaining:
- When and how
alias()should be used- The constraint that it only works within getters during pass 2
- Why module-level state is used (coupling with resolveSemanticTokens)
For example:
+/** + * Creates a function that marks semantic tokens as aliases. + * + * When a token is marked as an alias, it references another token's CSS variable + * directly without creating its own CSS variable. This is useful for semantic + * tokens that should simply point to base tokens. + * + * Note: alias() must be called within a token getter during pass 2 resolution. + * The current processing path is tracked via module-level state that's set by + * resolveSemanticTokens before each getter execution. + * + * @param context - The token processing context containing the aliasMap + * @returns A function that marks the current token path as an alias + * + * @example + * ```typescript + * semantic: { + * get primary(): string { + * return this.alias(this.color.base.blue); + * } + * } + * ``` + */ function createAliasFunction(context: TokenProcessingContext) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/honest-dingos-push.md(1 hunks)packages/css/src/theme/index.ts(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
.changeset/honest-dingos-push.md (1)
packages/css/src/theme/types.ts (2)
myTheme(331-415)it(223-416)
🪛 markdownlint-cli2 (0.18.1)
.changeset/honest-dingos-push.md
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (7)
packages/css/src/theme/index.ts (7)
39-39: LGTM!The alias method addition follows the same pattern as the existing
raw()utility and integrates cleanly with the ThemeSubFunctions interface.
129-138: LGTM!The context initialization properly sets up the aliasMap tracking structure for the two-pass token resolution algorithm.
157-162: LGTM!The alias utility is properly configured as a non-enumerable helper function, following the same pattern as the existing
rawutility.
170-177: LGTM!The stripHash utility correctly removes hash suffixes from CSS variable names using an appropriate regex pattern.
241-242: LGTM!The aliasMap is correctly propagated through all nested token processing contexts, ensuring consistent alias tracking across the entire token tree.
Also applies to: 304-305, 379-380, 392-393
338-359: Verify the getter execution context is always properly cleaned up.The currentProcessingPath is set before getter execution and cleared after. While this works, the pattern is fragile—if an exception occurs during getter execution or if code is added between lines 342 and 344, the path might not be cleared properly.
Consider wrapping the getter execution in a try-finally block:
// Handle getters (semantic tokens) if (typeof descriptor.get === "function") { // Set the current path for alias() to access currentProcessingPath = currentPath; - // Call getter with resolvedTokens as this context - // This allows semantic tokens to reference other tokens - const computedValue = descriptor.get.call(resolvedTokens); - // Clear the current path after getter execution - currentProcessingPath = []; + try { + // Call getter with resolvedTokens as this context + // This allows semantic tokens to reference other tokens + const computedValue = descriptor.get.call(resolvedTokens); + - // Check if this path is marked as an alias - const currentPathStr = currentPath.join("."); + // Check if this path is marked as an alias + const currentPathStr = currentPath.join("."); - if (context.aliasMap.has(currentPathStr)) { - // Don't create a CSS variable for aliases - // Just update resolvedTokens with the aliased reference (unhashed) - const unhashedValue = stripHash(computedValue); - setByPath(resolvedTokens, currentPath, unhashedValue); - } else { - // Normal flow: create CSS variable - const cssVar = getCSSVarByPath(varPath, context); - vars[cssVar] = computedValue; - setByPath(resolvedTokens, currentPath, computedValue); + if (context.aliasMap.has(currentPathStr)) { + // Don't create a CSS variable for aliases + // Just update resolvedTokens with the aliased reference (unhashed) + const unhashedValue = stripHash(computedValue); + setByPath(resolvedTokens, currentPath, unhashedValue); + } else { + // Normal flow: create CSS variable + const cssVar = getCSSVarByPath(varPath, context); + vars[cssVar] = computedValue; + setByPath(resolvedTokens, currentPath, computedValue); + } + } finally { + // Clear the current path after getter execution + currentProcessingPath = []; } continue; }
946-1000: LGTM!The test case comprehensively validates the alias functionality:
- Confirms aliased tokens don't generate CSS variables
- Confirms non-aliased tokens continue to generate CSS variables as expected
- Validates all resolved tokens use correct var() references
| "@mincho-js/css": minor | ||
| --- | ||
|
|
||
| **theme** |
There was a problem hiding this comment.
Use a proper heading instead of bold emphasis.
Markdownlint flags this line because **theme** uses emphasis rather than a heading. For better document structure, use a proper heading level.
Apply this diff:
-**theme**
+## theme📝 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.
| **theme** | |
| ## theme |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🤖 Prompt for AI Agents
.changeset/honest-dingos-push.md around line 5: the line uses bold emphasis
"**theme**" instead of a markdown heading; replace it with an appropriate
heading (e.g., "## theme" or "# theme" depending on desired level) so the file
uses a proper heading syntax and satisfies markdownlint.
|
/fast-forward |
|
Triggered from #272 (comment) by @black7375. Trying to fast forward Target branch ( commit 50145b3eca703e9884113b58eecd0c6ecb8631d3 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Sep 25 00:00:00 2025 +0900
Feat: `theme` - Support `raw` method #268Pull request ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (pull_request/theme-alias-api)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270Fast forwarding $ git push origin 4a185b470a8d24ecc6badcb46ea4c0408e486a07:main
To https://github.com/mincho-js/mincho.git
50145b3..4a185b4 4a185b470a8d24ecc6badcb46ea4c0408e486a07 -> main |
1 participant
Description
Implement
aliasmethod for themeRelated Issue
Summary by CodeRabbit
Additional context
Checklist