Makes more typesafe for hoistSelectors#248
Conversation
🦋 Changeset detectedLatest commit: f1ca15c The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 #248 by @black7375. Checking if we can fast forward Target branch ( commit d9445d541aae580053755bfc3a5f9e07620241d0 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Jul 31 00:00:00 2025 +0900
Feat: `selector()` API implement #236Pull request ( commit 540931907d8a0498b57d45243e33192e317cd478 (pull_request/type-fixes)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Aug 7 00:00:00 2025 +0900
Refactor: `hoistSelectors` makes to type-safeIt is possible to fast forward |
WalkthroughAdds a changeset and public API update introducing a selectors property on CSSConditions, refactors hoistSelectors to a type-safe, path-based traversal with new AtRulePath/TypeSafeHoistResult types and type guards, and updates transform-to-vanilla selector-related type declarations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Hoist as hoistSelectors
participant Types as Type Guards / AtRulesKeywords
participant Result as TypeSafeHoistResult
Caller->>Hoist: hoistSelectors(input)
Hoist->>Types: isAtRuleKey(key)? / isAtRuleObject(val)?
alt At-rule encountered
Hoist->>Hoist: Recurse with AtRulePath ([@rule,...])
Hoist->>Hoist: Merge nested style objects along path
else Selector encountered
Hoist->>Hoist: Collect selectors into selectors map
end
Hoist->>Result: assemble { selectors, hoisted, ... }
Result-->>Caller: return TypeSafeHoistResult
note right of Types: type guards validate shapes during traversal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/css/src/css/index.ts (1)
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: 0
🧹 Nitpick comments (2)
packages/css/src/css/index.ts (1)
109-119: Consider making the at-rule check more maintainable.The current implementation manually checks each at-rule type. This could be refactored to iterate over the
AtRulesKeywordstype for better maintainability.Apply this diff to make the function more maintainable:
function isAtRuleKey( key: string ): key is `@${AtRulesKeywords}` | `@${AtRulesKeywords} ${string}` { - return ( - key.startsWith("@") && - (key.startsWith("@media") || - key.startsWith("@supports") || - key.startsWith("@container") || - key.startsWith("@layer")) - ); + if (!key.startsWith("@")) return false; + const atRules: AtRulesKeywords[] = ["media", "supports", "container", "layer"]; + return atRules.some(rule => key.startsWith(`@${rule}`)); }.changeset/vast-apples-cry.md (1)
5-5: Fix markdown formatting for consistency.The markdown linter suggests using a heading instead of emphasis for "type". While this is a minor formatting issue, it's good practice to follow markdown conventions.
Apply this diff:
-**type** +### Type Changes - add `selectors` property to CSSConditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/vast-apples-cry.md(1 hunks)packages/css/src/css/index.ts(4 hunks)packages/transform-to-vanilla/src/types/style-rule.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/css/src/css/index.ts (1)
packages/transform-to-vanilla/src/types/style-rule.ts (2)
CSSRule(33-33)AtRulesKeywords(193-193)
🪛 markdownlint-cli2 (0.18.1)
.changeset/vast-apples-cry.md
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (7)
packages/transform-to-vanilla/src/types/style-rule.ts (1)
33-33: LGTM! Clean type hierarchy refactor.The changes correctly introduce
SelectorPropertyas an internal interface and compose it intoCSSConditions, which ultimately makes theselectorsproperty available onCSSRulethrough the type hierarchy chain. This modular approach improves type organization and aligns with the PR's goal of makinghoistSelectorsmore type-safe.Also applies to: 158-171
packages/css/src/css/index.ts (6)
10-11: LGTM! Necessary imports for type-safe at-rule handling.The
GlobalCSSRuleandAtRulesKeywordsimports enable the new type guards and type-safe hoisting logic.
44-47: LGTM! Well-defined types for path-based hoisting.
AtRulePathandTypeSafeHoistResultclearly express the intent of the new implementation, using readonly modifiers appropriately for immutability guarantees.
54-93: Verify the type casts are necessary.The implementation uses several type casts (lines 63, 68, 72, 86) to work around TypeScript's type inference. While the logic appears correct, these casts reduce compile-time safety guarantees.
Consider if the type definitions can be improved to avoid these casts, or add runtime assertions to validate the casts are safe. The current implementation assumes the structure is correct but doesn't validate it beyond the type guards.
For example, at line 86, you cast
nestedRuletoCSSRule, but there's only a check that it's an object. You might want to add a helper that validates the structure more thoroughly:function isCSSRule(value: unknown): value is CSSRule { return typeof value === "object" && value !== null && !Array.isArray(value); }Then use this guard before the cast to make the intent clearer and add runtime safety.
49-97: LGTM! Solid implementation of type-safe hoisting.The refactored
hoistSelectorsfunction correctly implements path-based traversal to hoist selectors out of nested at-rules. The recursive approach with explicit path tracking is appropriate for this use case. The algorithm correctly:
- Identifies selectors within at-rules using type guards
- Builds the inverted structure with selectors at the top level
- Preserves the nesting path for each selector
99-123: LGTM! Type guards provide good runtime safety.The three type guard functions (
hasSelectorsProperty,isAtRuleKey,isAtRuleObject) add necessary runtime checks to validate the structure during hoisting. They follow TypeScript best practices for type predicates.
279-595: Excellent test coverage!The test suite comprehensively covers:
- Simple and complex selector hoisting scenarios
- Nested at-rule structures (up to 3 levels deep)
- Multiple selectors and at-rule combinations
- Edge cases (empty inputs, malformed data, missing selectors)
- Type guard behavior
- Complex selector strings
This thorough coverage gives high confidence in the implementation's correctness.
Also applies to: 966-1122
|
Triggered from #248 by @black7375. Checking if we can fast forward Target branch ( commit d9445d541aae580053755bfc3a5f9e07620241d0 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Jul 31 00:00:00 2025 +0900
Feat: `selector()` API implement #236Pull request ( commit f1ca15cac81f1356b267060896b2c63cd871cfad (pull_request/type-fixes)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Aug 7 00:00:00 2025 +0900
Refactor: `hoistSelectors` makes to type-safeIt is possible to fast forward |
|
/fast-forward |
|
Triggered from #248 (comment) by @black7375. Trying to fast forward Target branch ( commit d9445d541aae580053755bfc3a5f9e07620241d0 (HEAD -> main, origin/main)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Jul 31 00:00:00 2025 +0900
Feat: `selector()` API implement #236Pull request ( commit f1ca15cac81f1356b267060896b2c63cd871cfad (pull_request/type-fixes)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Thu Aug 7 00:00:00 2025 +0900
Refactor: `hoistSelectors` makes to type-safeFast forwarding $ git push origin f1ca15cac81f1356b267060896b2c63cd871cfad:main
To https://github.com/mincho-js/mincho.git
d9445d5..f1ca15c f1ca15cac81f1356b267060896b2c63cd871cfad -> main |
1 participant
Description
Make typesafe
hoistSelectors()Related Issue
Summary by CodeRabbit
Additional context
Checklist