Refactor: separate Main and Compat API layers #245#273
Refactor: separate Main and Compat API layers #245#273github-actions[bot] merged 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 1bb5010 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 |
WalkthroughThis PR splits Vanilla Extract compatibility code into new compat-impl modules, re-exports compatibility APIs from the compat entry, simplifies core css/rules APIs by removing data/mapping overloads, and converts compoundVariants to a function-driven shape in the compat and core layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CompatEntry as packages/css/src/compat.ts
participant CSSCompat as css/compat-impl.ts
participant RulesCompat as rules/compat-impl.ts
participant CSSCore as css/index.ts
participant RulesCore as rules/index.ts
Dev->>CompatEntry: import { styleVariants, recipe }
rect `#DFF4FF`
CompatEntry->>CSSCompat: styleVariants(map | data, mapFn?)
CSSCompat->>CSSCompat: detect mapFn vs static
alt static
CSSCompat->>CSSCore: css(style) per entry -> transform -> className
else mapping
CSSCompat->>CSSCompat: mapFn(value,key) -> rule
CSSCompat->>CSSCore: css(mapped rule) per entry
end
CSSCompat-->>Dev: Record<key, className>
end
rect `#F6EFFF`
CompatEntry->>RulesCompat: recipe(CompatPatternOptions)
RulesCompat->>RulesCompat: process base, props, toggles, variants, compoundVariants
RulesCompat->>CSSCore: css.multiple(variants)
RulesCompat-->>Dev: runtime function (className mapping)
end
rect `#FFF8E6`
Dev->>RulesCore: existing recipe APIs (core)
RulesCore->>RulesCore: function-based compoundVariants path
RulesCore->>CSSCore: css.multiple with normalized variants
RulesCore-->>Dev: runtime function
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related issues
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Triggered from #273 by @Jeong-jj. Checking if we can fast forward Target branch ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270Pull request ( commit ca1c6c625ba11bb5f16dd4c4a1abb7762d184a5a (pull_request/refactor/compatibility)
Merge: 878d71d 4a185b4
Author: JeongJun <rgfdds98@gmail.com>
Date: Thu Nov 6 00:30:18 2025 +0900
Merge branch 'main' into refactor/compatibilityIt is possible to fast forward |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/css/src/rules/index.ts (1)
228-263: Regression: cannot express togglefalsein new compoundVariants.
variantConditionsis built from the keys present inmergedVariants. For toggle definitions we only materialize a"true"entry (viatransformToggleVariants), so the helper injected into user code never exposes something likedisabled.false. With the legacy array/object form you could target{ disabled: false, color: "brand" }, but the function form no longer has a way to represent “toggle off”, removing a real-world compound case (e.g., “apply outline-only whendisabledis false andcoloris brand”).We should surface a branded
falsehandle for toggle groups—either by synthesizing it alongside thetrueentry here or by extendingVariantStringMapgeneration—so that consumers can still write({ disabled }) => [{ condition: [disabled.false, …], … }].Concrete tweak (one option) after computing
variantConditions:const variantConditions = mapValues( mergedVariants, (variantGroup, variantName) => mapValues(variantGroup, (_, optionKey) => ({ [variantName]: optionKey === "true" ? true : optionKey === "false" ? false : optionKey })) ) as VariantStringMap<CombinedVariants>; + +for (const variantName in variantConditions) { + const options = variantConditions[ + variantName as keyof typeof variantConditions + ] as Record<string, BrandValue<CombinedVariants>>; + if (!("false" in options) && "true" in options) { + options.false = { + [variantName]: false + } as BrandValue<CombinedVariants>; + } +}That restores the ability to express the negative toggle branch without forcing people back to the compat layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/css/src/compat.ts(2 hunks)packages/css/src/css/compat-impl.ts(1 hunks)packages/css/src/css/index.ts(5 hunks)packages/css/src/rules/compat-impl.ts(1 hunks)packages/css/src/rules/index.ts(6 hunks)packages/css/src/rules/types.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
packages/css/src/compat.tspackages/css/src/rules/types.tspackages/css/src/rules/compat-impl.tspackages/css/src/rules/index.tspackages/css/src/css/index.ts
📚 Learning: 2024-12-03T11:16:32.528Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 134
File: packages/transform-to-vanilla/src/transform.ts:49-51
Timestamp: 2024-12-03T11:16:32.528Z
Learning: In projects that target both CommonJS and ES modules, adjusting the TypeScript `module` option may not be feasible. Therefore, using `ts-ignore` comments to suppress TypeScript errors related to `import.meta` is acceptable.
Applied to files:
packages/css/src/css/index.ts
🧬 Code graph analysis (4)
packages/css/src/rules/compat-impl.ts (6)
packages/css/src/rules/types.ts (4)
RecipeStyleRule(31-31)PropVars(124-126)PatternResult(128-137)PropDefinition(80-85)packages/transform-to-vanilla/src/utils/object.ts (1)
mergeObject(5-11)packages/css/src/utils.ts (3)
getDebugName(13-15)getVarName(20-29)identifierName(5-11)packages/css/src/rules/createRuntimeFn.ts (3)
base(94-96)variants(98-102)createRuntimeFn(29-106)packages/css/src/rules/utils.ts (3)
transformToggleVariants(66-79)mapValues(9-20)transformVariantSelection(33-64)packages/transform-to-vanilla/src/types/style-rule.ts (2)
PureCSSVarKey(117-117)CSSPropertiesWithVars(47-50)
packages/css/src/css/compat-impl.ts (2)
packages/css/src/utils.ts (2)
getDebugName(13-15)identifierName(5-11)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)
packages/css/src/rules/index.ts (5)
packages/css/src/rules/utils.ts (1)
mapValues(9-20)packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/css/src/css/index.ts (1)
css(126-130)packages/css/src/rules/types.ts (3)
VariantObjectSelection(53-57)BrandValue(148-150)RecipeStyleRule(31-31)packages/css/src/rules/createRuntimeFn.ts (1)
variants(98-102)
packages/css/src/css/index.ts (2)
packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)
🔇 Additional comments (4)
packages/css/src/compat.ts (1)
28-49: Compat exports routed correctly.Re-exporting
styleVariants,recipe, andCompatPatternOptionsfrom the compat implementations cleanly scopes the Vanilla Extract surface without disturbing the streamlined main API. Looks solid.packages/css/src/rules/types.ts (1)
152-162: Function-only CompoundVariants shape looks good.The narrowed alias to
CompoundVariantFnplus the accompanying test notes keeps the main API focused on condition-based flows while compat covers the legacy array form. 👍packages/css/src/css/index.ts (1)
198-236: Unified multiple pipeline looks solid.The refactored
processMultiplekeeps the transform/variant-reference loop in one place and matches the compat helper, which should make future tweaks much easier. Nice work keeping the tests in sync.packages/css/src/css/compat-impl.ts (1)
22-200: Compat styleVariants implementation reads well.The overloads plus mirrored
processMultiplegive VE users the richer mapping story without leaking back into the main API—great separation, and the tests cover the tricky paths.
| if (isMapDataFunction(mapDataOrDebugId)) { | ||
| const data = ruleMapOrData as Data; | ||
| const mapData = mapDataOrDebugId; | ||
| return recipeMultiple( | ||
| data, | ||
| (value: Parameters<typeof callback>[0], key: keyof Data) => | ||
| mapData(callback(value) as Parameters<MapData>[0], key), | ||
| debugId | ||
| ) as TransformDataMapping<Data, MapData>; |
There was a problem hiding this comment.
Fix recipeWithMultiple mapData input
mapData is declared (and users will type it) to accept the original record value (Data[keyof Data], i.e. the CSS rule), but at runtime we’re passing callback(value)—a CompatPatternOptions. Any existing mapData that accesses CSS properties will now receive the wrong shape, leading to runtime exceptions and corrupted recipes. Please either pass the untouched value into mapData, or adjust the overloads/TransformDataMapping so the first argument is explicitly the callback result and update callers accordingly. As written this is a breaking bug.
🤖 Prompt for AI Agents
In packages/css/src/rules/compat-impl.ts around lines 146-154, the code
currently calls mapData(callback(value)...) which passes the callback result
(CompatPatternOptions) into mapData; change this to pass the original record
value instead by calling mapData(value, key) so mapData receives Data[keyof
Data] as declared. Ensure the parameters and return casting remain consistent
(adjust the runtime call only), and run type checks to confirm no further
overload changes are required.
|
Triggered from #273 by @Jeong-jj. Checking if we can fast forward Target branch ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270Pull request ( commit 9538760024f8320a0f09883cc8f91a807238d416 (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Sat Nov 8 00:57:55 2025 +0900
fix: resolve build type errorIt is possible to fast forward |
Jeong-jj
force-pushed
the
refactor/compatibility
branch
from
9538760 to
9a9ea16
Compare
|
Triggered from #273 by @Jeong-jj. Checking if we can fast forward Target branch ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270Pull request ( commit 9a9ea16fbcb7b2fbf9ac9b5a2ec62e8d99c6e193 (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Sat Nov 8 00:57:55 2025 +0900
fix: resolve type errors with children prop and component compositionIt is possible to fast forward |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react/src/index.ts (1)
128-143: Consider simplifying the conditional return type.The conditional
Component extends ElementType ? Component : ElementTypeon line 139 appears potentially redundant, since bothComponentType<Props>andForwardRefExoticComponent<Props>should satisfyElementType. The conditional may be defensive programming for TypeScript edge cases, but if it always resolves toComponent, you could simplify by directly returningComponentas the second type parameter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
examples/react-babel/src/App.tsx(2 hunks)packages/react/src/index.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
packages/react/src/index.ts
🧬 Code graph analysis (2)
examples/react-babel/src/App.tsx (3)
packages/css/src/rules/index.ts (2)
variants(523-658)variants(959-1065)packages/babel/src/styled.ts (1)
styledComponentPlugin(13-103)packages/css/src/rules/types.ts (1)
CompoundVariant(139-142)
packages/react/src/index.ts (1)
packages/react/src/runtime.ts (1)
$$styled(17-81)
⏰ 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). (1)
- GitHub Check: typecheck
🔇 Additional comments (7)
examples/react-babel/src/App.tsx (2)
3-3: LGTM: Improved naming.The rename from
BaseComponenttoContaineris more semantic and better describes the component's role.
34-47: LGTM: Function-based compound variants improve type safety.The migration to function-based
compoundVariantsis correctly implemented and aligns with the new API pattern demonstrated in the test suite. This approach provides:
- Better type safety and intellisense through typed property access (e.g.,
size.small,color.blue)- More direct and intuitive condition syntax via the
conditionarray- Consistent alignment with the broader refactor objective
All variant references are valid and the styles remain unchanged as expected.
packages/react/src/index.ts (5)
26-92: Well-structured type definitions for polymorphic component support.The helper types provide solid foundations for the "as" prop pattern and variant-aware styled components. The
NonNeverutility correctly uses the[T] extends [never]pattern, and theStyledComponenttype properly composesComponentWithAsandForwardRefExoticComponentwith merged props.
94-109: LGTM - First overload handles ComponentWithAs correctly.This overload specifically targets components that already implement the
ComponentWithAsinterface, which is the most specific case. The positioning as the first overload ensures it's matched before more general overloads.
111-126: LGTM - ForwardRefExoticComponent overload is correctly positioned.This overload handles
ForwardRefExoticComponentspecifically and is correctly placed before the more general third overload. Compatible with React 19's ref handling.
145-159: LGTM - Intrinsic element overload correctly extracts JSX props.This overload properly handles intrinsic HTML elements using
IntrinsicProps<Component>to extract the correct prop types fromJSX.IntrinsicElements. The positioning after component overloads is appropriate.
161-178: LGTM - Implementation correctly delegates to runtime.This implementation serves as the fallback overload that should be removed by the build process. The error message clearly indicates a build configuration issue. The actual runtime implementation is properly exported from
./runtime.jsas$$styled.
Jeong-jj
force-pushed
the
refactor/compatibility
branch
from
9a9ea16 to
7e9a766
Compare
|
Triggered from #273 by @Jeong-jj. Checking if we can fast forward Target branch ( commit 4a185b470a8d24ecc6badcb46ea4c0408e486a07 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Sep 27 00:00:00 2025 +0900
Feat: `theme` - Support `alias` method #270Pull request ( commit 7e9a76671d358b58213a1b5af70f1b89dd389c9e (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Sat Nov 8 00:57:55 2025 +0900
Fix: resolve type errors with children prop and component compositionIt is possible to fast forward |
black7375
force-pushed
the
refactor/compatibility
branch
from
7e9a766 to
90d48cb
Compare
|
Triggered from #273 by @black7375. Checking if we can fast forward Target branch ( commit 4504956736658a23a6f0a5d9510bf066a43e614c (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 11 00:00:00 2025 +0900
Feat: `styled` - Support `styled.div` like shorthand APIPull request ( commit 90d48cb332a224a368e16e5c6057b036a71a97c9 (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Sun Nov 9 20:23:10 2025 +0900
Refactor: restrict `rules()` to function-style compoundVariants #245It is possible to fast forward |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/react-babel/src/App.tsx(1 hunks)packages/css/src/compat.ts(2 hunks)packages/css/src/css/compat-impl.ts(1 hunks)packages/css/src/css/index.ts(5 hunks)packages/css/src/rules/compat-impl.ts(1 hunks)packages/css/src/rules/index.ts(8 hunks)packages/css/src/rules/types.ts(7 hunks)packages/css/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/react-babel/src/App.tsx
- packages/css/src/compat.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
packages/css/src/rules/types.tspackages/css/src/css/compat-impl.tspackages/css/src/rules/compat-impl.tspackages/css/src/rules/index.ts
📚 Learning: 2024-09-29T14:13:13.977Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 110
File: packages/css/src/rules/types.ts:25-29
Timestamp: 2024-09-29T14:13:13.977Z
Learning: For `ToggleVariantMap`, only the `true` state is needed for toggle variants.
Applied to files:
packages/css/src/rules/types.ts
🧬 Code graph analysis (5)
packages/css/src/rules/types.ts (3)
packages/css/src/compat.ts (8)
VariantGroups(35-35)PatternOptions(36-36)VariantDefinitions(40-40)ComplexPropDefinitions(38-38)PropTarget(39-39)CSSRule(54-54)VariantSelection(37-37)ConditionalVariants(41-41)packages/css/src/index.ts (8)
VariantGroups(31-31)PatternOptions(32-32)VariantDefinitions(36-36)ComplexPropDefinitions(34-34)PropTarget(35-35)CSSRule(5-5)VariantSelection(33-33)ConditionalVariants(37-37)packages/transform-to-vanilla/src/types/style-rule.ts (2)
CSSRule(33-33)CSSPropertiesWithVars(47-50)
packages/css/src/css/index.ts (3)
packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)packages/css/src/utils.ts (1)
identifierName(5-11)
packages/css/src/css/compat-impl.ts (2)
packages/css/src/utils.ts (2)
getDebugName(13-15)identifierName(5-11)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)
packages/css/src/rules/compat-impl.ts (7)
packages/css/src/rules/types.ts (14)
VariantGroups(52-52)VariantSelection(65-67)RecipeStyleRule(31-31)VariantDefinitions(43-43)ComplexPropDefinitions(77-79)PropTarget(75-75)ConditionalVariants(159-168)RuntimeFn(192-199)PropVars(124-126)PropDefinitionOutput(87-93)PatternResult(128-137)VariantObjectSelection(53-57)Serializable(25-29)PropDefinition(80-85)packages/transform-to-vanilla/src/utils/object.ts (1)
mergeObject(5-11)packages/css/src/utils.ts (3)
getDebugName(13-15)getVarName(20-29)identifierName(5-11)packages/css/src/rules/createRuntimeFn.ts (3)
base(94-96)variants(98-102)createRuntimeFn(29-106)packages/css/src/css/index.ts (1)
css(126-130)packages/css/src/rules/utils.ts (3)
transformToggleVariants(66-79)mapValues(9-20)transformVariantSelection(33-64)packages/transform-to-vanilla/src/types/style-rule.ts (1)
PureCSSVarKey(117-117)
packages/css/src/rules/index.ts (5)
packages/css/src/rules/utils.ts (1)
mapValues(9-20)packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/css/src/css/index.ts (1)
css(126-130)packages/css/src/utils.ts (2)
getDebugName(13-15)identifierName(5-11)packages/css/src/rules/types.ts (1)
VariantObjectSelection(53-57)
🔇 Additional comments (8)
packages/css/src/rules/compat-impl.ts (8)
37-66: LGTM! Well-structured compatibility types.The CompatCompoundVariant and CompatPatternOptions interfaces provide a clean Vanilla Extract-compatible API surface with proper generic constraints and clear documentation.
71-109: LGTM! Props processing is robust.The function correctly handles both array and object forms of props, with appropriate defaults and type narrowing. The use of
PurePropsensures type safety for non-nullable props.
111-122: LGTM! Base style handling is comprehensive.The logic correctly handles all forms of base styles (undefined, string, object, array) with appropriate merging and className generation.
133-148: LGTM! Variant transformation is correct.The logic properly transforms variant style rules into the format expected by
css.multiple, handling both string and object forms.
150-161: LGTM! Compound variants processing is sound.The logic correctly transforms compound variant specifications into the runtime format, with appropriate style processing for both string and object forms.
163-184: LGTM! Proper runtime function creation and serialization.The PatternResult config is correctly constructed and wrapped with
addFunctionSerializerfor Vanilla Extract compatibility, with appropriate serialization metadata.
186-226: LGTM! Helper functions are well-implemented.The helper functions are straightforward and correct:
setCSSPropertypragmatically bypasses type checking for dynamic property assignment (acceptable in this controlled context)processPropObjectsafely handles optional values with proper chainingprocessCompoundStyleappropriately handles both string and object styles
228-1197: LGTM! Excellent test coverage.The test suite is comprehensive and well-structured, covering:
- Type-level validation for interfaces
- Base, variant, and compound style processing
- Both object and array forms of variant specifications
- Props handling with various configurations
- Edge cases and composition scenarios
This provides strong confidence in the implementation's correctness.
| const styleMap = styleMapOrData as StyleMap; | ||
| const debugId = mapDataOrDebugId; | ||
| return processMultiple(styleMap, (style) => style, debugId); | ||
| } |
There was a problem hiding this comment.
Restore debugId when mapper is omitted.
Inside the static-variant branch we redeclare const debugId = mapDataOrDebugId;, so whenever callers forward an optional mapper (which may be undefined) plus an explicit third-argument debugId, the debugId is discarded. That regresses debug-prefixed class names and breaks consumers/tests that pass styleVariants(map, maybeMapper, debugId) when the mapper is absent. Please keep the third parameter in play by resolving the debugId via both arguments.
- const debugId = mapDataOrDebugId;
- return processMultiple(styleMap, (style) => style, debugId);
+ const resolvedDebugId =
+ typeof mapDataOrDebugId === "string" ? mapDataOrDebugId : debugId;
+ return processMultiple(styleMap, (style) => style, resolvedDebugId);📝 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.
| const styleMap = styleMapOrData as StyleMap; | |
| const debugId = mapDataOrDebugId; | |
| return processMultiple(styleMap, (style) => style, debugId); | |
| } | |
| const styleMap = styleMapOrData as StyleMap; | |
| const resolvedDebugId = | |
| typeof mapDataOrDebugId === "string" ? mapDataOrDebugId : debugId; | |
| return processMultiple(styleMap, (style) => style, resolvedDebugId); | |
| } |
🤖 Prompt for AI Agents
In packages/css/src/css/compat-impl.ts around lines 46 to 49, the code
unconditionally assigns debugId = mapDataOrDebugId which discards a passed
third-argument debugId when the optional mapper is omitted; restore use of the
third parameter by resolving debugId from both possible positions (i.e., if
mapDataOrDebugId is a debugId and styleMapOrData is the mapper/data, pick the
non-undefined debugId regardless of whether the mapper argument is present), and
pass that resolved debugId through to processMultiple so callers that provide an
explicit debugId with an omitted mapper still get their debug prefix preserved.
| // @ts-expect-error - Temporarily ignoring the error as the PatternResult type is not fully defined | ||
| const variantClassNames: PatternResult<CombinedVariants>["variantClassNames"] = |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete the PatternResult type parameters.
The PatternResult type requires both Variants and Props generic parameters. Provide both for type safety and to remove the ts-expect-error suppression.
Apply this diff:
- // @ts-expect-error - Temporarily ignoring the error as the PatternResult type is not fully defined
- const variantClassNames: PatternResult<CombinedVariants>["variantClassNames"] =
+ const variantClassNames: PatternResult<CombinedVariants, PureProps>["variantClassNames"] =📝 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.
| // @ts-expect-error - Temporarily ignoring the error as the PatternResult type is not fully defined | |
| const variantClassNames: PatternResult<CombinedVariants>["variantClassNames"] = | |
| const variantClassNames: PatternResult<CombinedVariants, PureProps>["variantClassNames"] = |
🤖 Prompt for AI Agents
In packages/css/src/rules/compat-impl.ts around lines 131-132, the PatternResult
generic is missing its second type parameter which caused the ts-expect-error;
replace the current declaration with a fully-typed generic such as
PatternResult<CombinedVariants, CombinedProps>["variantClassNames"] (or the
actual Props type used in this module — import/define CombinedProps if needed),
and remove the // @ts-expect-error line so the compiler enforces correct typing.
90d48cb to
1bb5010
Compare
|
Triggered from #273 by @black7375. Checking if we can fast forward Target branch ( commit 4504956736658a23a6f0a5d9510bf066a43e614c (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 11 00:00:00 2025 +0900
Feat: `styled` - Support `styled.div` like shorthand APIPull request ( commit 1bb50106844d162d26f1f470ef0cae0565461708 (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Wed Oct 15 00:00:00 2025 +0900
Refactor: restrict `rules()` to function-style compoundVariants #245It is possible to fast forward |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/css/src/css/compat-impl.ts (1)
47-49: Restore debugId passthrough when mapper is omitted.When callers do
styleVariants(map, undefined, "button"), the debug prefix should flow through, but this branch still shadows the third-argument debugId with the undefined mapper slot. That breaks debug-prefixed class names—the same regression flagged earlier and still unresolved. Please resolve the debugId from both optional positions before passing it toprocessMultiple.- const debugId = mapDataOrDebugId; - return processMultiple(styleMap, (style) => style, debugId); + const resolvedDebugId = + typeof mapDataOrDebugId === "string" ? mapDataOrDebugId : debugId; + return processMultiple( + styleMap, + (style) => style, + resolvedDebugId + );packages/css/src/rules/compat-impl.ts (1)
132-148: Remove thets-expect-errorby supplying the missing PatternResult generic.
Line 132 is still suppressing a real type error. We should provide the second generic parameter so TypeScript can validate the shape ofvariantClassNamesinstead of silencing it.- // @ts-expect-error - Temporarily ignoring the error as the PatternResult type is not fully defined - const variantClassNames: PatternResult<CombinedVariants>["variantClassNames"] = + const variantClassNames: PatternResult<CombinedVariants, PureProps>["variantClassNames"] =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/fresh-rice-sip.md(1 hunks).changeset/tame-carpets-post.md(1 hunks)examples/react-babel/src/App.tsx(1 hunks)packages/css/src/compat.ts(2 hunks)packages/css/src/css/compat-impl.ts(1 hunks)packages/css/src/css/index.ts(5 hunks)packages/css/src/rules/compat-impl.ts(1 hunks)packages/css/src/rules/index.ts(8 hunks)packages/css/src/rules/types.ts(7 hunks)packages/css/src/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/tame-carpets-post.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/css/src/compat.ts
- packages/css/src/utils.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-09T13:00:38.449Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 113
File: packages/css/src/index.ts:33-33
Timestamp: 2024-10-09T13:00:38.449Z
Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
Applied to files:
.changeset/fresh-rice-sip.mdpackages/css/src/rules/index.tspackages/css/src/rules/types.tspackages/css/src/rules/compat-impl.ts
📚 Learning: 2024-09-29T14:13:13.977Z
Learnt from: black7375
Repo: mincho-js/mincho PR: 110
File: packages/css/src/rules/types.ts:25-29
Timestamp: 2024-09-29T14:13:13.977Z
Learning: For `ToggleVariantMap`, only the `true` state is needed for toggle variants.
Applied to files:
packages/css/src/rules/types.ts
🧬 Code graph analysis (7)
.changeset/fresh-rice-sip.md (1)
packages/transform-to-vanilla/src/types/style-rule.ts (4)
CSSMergeProperties(147-149)CSSComplexProperties(52-54)CSSPropertiesWithVars(47-50)CSSRule(33-33)
packages/css/src/css/compat-impl.ts (2)
packages/css/src/utils.ts (2)
getDebugName(13-15)identifierName(5-11)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)
packages/css/src/rules/index.ts (5)
packages/css/src/rules/utils.ts (1)
mapValues(9-20)packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/css/src/css/index.ts (1)
css(126-130)packages/css/src/utils.ts (1)
getDebugName(13-15)packages/css/src/rules/types.ts (1)
VariantObjectSelection(53-57)
packages/css/src/rules/types.ts (1)
packages/transform-to-vanilla/src/types/style-rule.ts (2)
CSSRule(33-33)CSSPropertiesWithVars(47-50)
packages/css/src/css/index.ts (3)
packages/transform-to-vanilla/src/types/style-rule.ts (1)
ComplexCSSRule(26-26)packages/transform-to-vanilla/src/transform.ts (1)
transform(16-42)packages/css/src/utils.ts (1)
identifierName(5-11)
examples/react-babel/src/App.tsx (2)
packages/react/src/runtime.ts (1)
$$styled(17-81)packages/babel/src/styled.ts (2)
styledComponentPlugin(13-103)enter(18-99)
packages/css/src/rules/compat-impl.ts (7)
packages/css/src/rules/types.ts (15)
VariantGroups(52-52)VariantSelection(65-67)RecipeStyleRule(31-31)VariantDefinitions(43-43)ComplexPropDefinitions(77-79)PropTarget(75-75)ConditionalVariants(159-168)RuntimeFn(192-199)PropVars(124-126)PropDefinitionOutput(87-93)PatternResult(128-137)VariantObjectSelection(53-57)Serializable(25-29)PropDefinition(80-85)VariantStyle(33-40)packages/transform-to-vanilla/src/utils/object.ts (1)
mergeObject(5-11)packages/css/src/utils.ts (3)
getDebugName(13-15)getVarName(20-29)identifierName(5-11)packages/css/src/rules/createRuntimeFn.ts (3)
base(94-96)variants(98-102)createRuntimeFn(29-106)packages/css/src/css/index.ts (1)
css(126-130)packages/css/src/rules/utils.ts (3)
transformToggleVariants(66-79)mapValues(9-20)transformVariantSelection(33-64)packages/transform-to-vanilla/src/types/style-rule.ts (1)
PureCSSVarKey(117-117)
| const isBaseValue = propValue?.base !== undefined; | ||
| propValue?.targets.forEach((target) => { | ||
| setCSSProperty( | ||
| propStyles, | ||
| target, | ||
| isBaseValue ? fallbackVar(propVar, `${propValue.base}`) : propVar | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Guard the targets array before calling forEach.
If a JS consumer omits targets, propValue?.targets.forEach will still dereference .forEach on undefined and crash. Add ?. on the array access to keep this defensive.
- propValue?.targets.forEach((target) => {
+ propValue?.targets?.forEach((target) => {📝 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.
| const isBaseValue = propValue?.base !== undefined; | |
| propValue?.targets.forEach((target) => { | |
| setCSSProperty( | |
| propStyles, | |
| target, | |
| isBaseValue ? fallbackVar(propVar, `${propValue.base}`) : propVar | |
| ); | |
| }); | |
| const isBaseValue = propValue?.base !== undefined; | |
| propValue?.targets?.forEach((target) => { | |
| setCSSProperty( | |
| propStyles, | |
| target, | |
| isBaseValue ? fallbackVar(propVar, `${propValue.base}`) : propVar | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In packages/css/src/rules/compat-impl.ts around lines 207 to 214, the code calls
propValue?.targets.forEach(...) which will throw if targets is undefined; change
the call to guard the array with optional chaining
(propValue?.targets?.forEach(...)) so the loop is skipped when targets is
missing, preserving the existing logic and preventing a runtime crash.
black7375
left a comment
black7375
left a comment
There was a problem hiding this comment.
It seems well-made.
I adjusted some of the test cases and types.
|
/fast-forward |
|
Triggered from #273 (comment) by @black7375. Trying to fast forward Target branch ( commit 4504956736658a23a6f0a5d9510bf066a43e614c (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Sat Oct 11 00:00:00 2025 +0900
Feat: `styled` - Support `styled.div` like shorthand APIPull request ( commit 1bb50106844d162d26f1f470ef0cae0565461708 (pull_request/refactor/compatibility)
Author: JeongJun <rgfdds98@gmail.com>
Date: Wed Oct 15 00:00:00 2025 +0900
Refactor: restrict `rules()` to function-style compoundVariants #245Fast forwarding $ git push origin 1bb50106844d162d26f1f470ef0cae0565461708:main
To https://github.com/mincho-js/mincho.git
4504956..1bb5010 1bb50106844d162d26f1f470ef0cae0565461708 -> main |
2 participants
Description
This PR refactors the API structure to separate the Main API (streamlined, function-based only) from the Compat API (Vanilla Extract compatible).
Fix Merging Type Error
Below Error was caused at ca1c6c6
examples/react-babel/src/App.tsx:9:26 - error TS2769: No overload matches this call. The last overload gave the following error. Argument of type 'StyledComponent<DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>, "div", undefined, undefined, undefined>' is not assignable to parameter of type 'keyof IntrinsicElements'. 9 const Container = styled(BaseComponent, { ~~~~~~~~~~~~~ packages/react/src/index.ts:78:17 78 export function styled< ~~~~~~ The last overload is declared here. examples/react-babel/src/App.tsx:61:6 - error TS2322: Type '{ children: string; size: "large"; color: "blue"; }' is not assignable to type 'IntrinsicAttributes & Omit<unknown, "color" | "size" | keyof RefAttributes<unknown>> & WithAsProp<ForwardRefExoticComponent<unknown>> & RefAttributes<...> & { ...; }'. Property 'children' does not exist on type 'IntrinsicAttributes & Omit<unknown, "color" | "size" | keyof RefAttributes<unknown>> & WithAsProp<ForwardRefExoticComponent<unknown>> & RefAttributes<...> & { ...; }'. 61 <Container size="large" color="blue">So, Fixes TypeScript compilation errors by improving type system robustness and migrating to function-based compoundVariants API.
Key Changes:
Added ForwardRefExoticComponent intersection to StyledComponent type for standard React props support (fixes missing children prop)
Reorganized styled() overloads: prioritized ComponentWithAs for better type matching
Relaxed ComponentWithAs constraints and removed complex type inference utilities
Updated example to use function-based compoundVariants syntax (relates to #245)
Result: All TypeScript errors resolved, improved type inference, better React ecosystem alignment
Related Issue
#245
Summary by CodeRabbit
New Features
Changes
Examples
Additional context
Checklist