Feat: Rules - Props type #131 #132
Conversation
|
WalkthroughThe changes in this pull request focus on enhancing type definitions within the CSS rules handling system. Key modifications include the introduction of a new generic type parameter Changes
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
error Couldn't find any versions for "eslint-config-custom" that matches "workspace:^" Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Triggered from #132 by @black7375. Checking if we can fast forward Target branch ( commit c1a1e2d6a1af293560bda8961dbbe05b3f30189b (HEAD -> main, origin/main, origin/HEAD)
Author: JeongJun <[email protected]>
Date: Mon Oct 7 19:59:25 2024 +0900
Test: RuntimeFn Type Test
Co-authored-by: alstjr7375 <[email protected]>Pull request ( commit d0991f539e651abf074aea91ca2dcd3078a0cd08 (pull_request/props-type)
Author: alstjr7375 <[email protected]>
Date: Sat Nov 23 22:51:35 2024 +0900
Fix: Rules - Prop expression type limitation #131It 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: 0
🧹 Outside diff range and nitpick comments (4)
packages/transform-to-vanilla/src/types/style-rule.ts (1)
247-248: LGTM! Consider adding JSDoc documentation.The export modification improves type accessibility and aligns with the PR objectives. The interface properly extends
Propertieswith appropriate type parameters.Consider adding JSDoc documentation to describe the purpose and usage of this interface:
+/** + * Resolves CSS properties to either numeric values or non-nullable strings. + * This interface is used to provide type safety for CSS property values in style rules. + */ export interface ResolvedProperties extends Properties<number | NonNullableString> {}packages/css/src/rules/index.ts (1)
Line range hint
109-411: Consider adding test cases for Props type parameterWhile the existing tests thoroughly cover the core functionality, consider adding test cases that specifically verify the new
Propstype parameter behavior.Would you like me to help generate test cases for the Props type parameter?
packages/css/src/rules/types.ts (2)
65-75: Consider documenting complex type definitions for maintainabilityThe introduction of
PropTarget,ComplexPropDefinitions, andPropDefinitionadds significant flexibility in defining complex property configurations. However, their complexity may pose challenges for future maintainers or new contributors. Consider adding comprehensive comments or documentation to explain the purpose and usage of these types.
603-630: Extend test coverage for edge cases inPropsWhile the added test cases cover various usages of the
propsfield, consider adding tests for edge cases, such as invalid inputs or unexpected property combinations, to ensure the robustness of the new functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/css/src/rules/index.ts(2 hunks)packages/css/src/rules/types.ts(6 hunks)packages/transform-to-vanilla/src/index.ts(1 hunks)packages/transform-to-vanilla/src/types/style-rule.ts(1 hunks)
🔇 Additional comments (8)
packages/transform-to-vanilla/src/index.ts (1)
8-8: LGTM! Verify the imported type implementation.
The new type export follows TypeScript best practices and aligns with the PR's objective of improving type definitions.
Let's verify the implementation of the imported type:
✅ Verification successful
Type implementation and usage verified successfully
The NonNullableString type is properly implemented in src/types/string.ts as string & NonNullable<unknown> and is being used consistently across the codebase, particularly in style rule types and font-face rule definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the NonNullableString type implementation and its usage
# Check the implementation of NonNullableString
ast-grep --pattern 'type NonNullableString = $_'
# Check where NonNullableString is being used
rg -t ts "NonNullableString"
Length of output: 3675
packages/css/src/rules/index.ts (3)
17-18: LGTM: Type imports are well-organized
The addition of ComplexPropDefinitions and PropTarget types enhances the type safety of the rules function.
31-35: LGTM: Type parameter addition improves type safety
The addition of the Props generic type parameter enhances type safety while maintaining backward compatibility.
Line range hint 71-72: Verify if @ts-expect-error can be removed
The comment indicates a temporary type issue. Let's verify if this can be resolved with the new type definitions.
✅ Verification successful
@ts-expect-error can be removed
The type system is now properly defined with:
PatternResulttype having a well-definedvariantClassNamesproperty of typeVariantsClassNames<Variants>ConditionalVariantstype correctly handling both regular variants and toggle variants- The type errors in
types.tsare intentional test cases and not related to thePatternResulttype definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if PatternResult type is now properly defined
# and if @ts-expect-error can be removed
# Search for PatternResult type definition
ast-grep --pattern 'type PatternResult<$_> = $_'
# Search for related type definitions that might affect PatternResult
rg -l "PatternResult|variantClassNames" --type ts
Length of output: 692
Script:
#!/bin/bash
# Let's check the actual usage context of PatternResult in index.ts
rg -A 5 -B 5 "@ts-expect-error" packages/css/src/rules/index.ts
# Also check how PatternResult is used in createRuntimeFn.ts for comparison
ast-grep --pattern 'PatternResult<$_>'
Length of output: 613
Script:
#!/bin/bash
# Let's check the type definition of CombinedVariants and related types
rg -A 3 "type CombinedVariants" packages/css/src/rules/index.ts
# And check the definition of ConditionalVariants
ast-grep --pattern 'type ConditionalVariants<$_> = $_'
# Also check if there are any type errors in the file
rg "type.*error" packages/css/src/rules/index.ts
Length of output: 343
Script:
#!/bin/bash
# Let's check the definition of ConditionalVariants in types.ts
rg -A 5 "ConditionalVariants" packages/css/src/rules/types.ts
# And check how variantClassNames is used in the PatternResult type
rg -A 5 "variantClassNames" packages/css/src/rules/types.ts
# Check for any type errors or related comments in types.ts
rg "error|TODO|FIXME" packages/css/src/rules/types.ts
Length of output: 1669
packages/css/src/rules/types.ts (4)
1-6: Imports are appropriate and necessary
The imported types (ComplexCSSRule, CSSRule, ResolvedProperties, NonNullableString) are used effectively in the code and are essential for the new type definitions and enhancements.
18-23: Utility type UnionToIntersection is correctly implemented
The UnionToIntersection utility type is accurately defined to convert a union type into an intersection type, which is beneficial for complex type manipulations later in the code.
77-113: Type definitions are well-structured and handle various cases effectively
The complex type definitions for PropDefinitionOutput and related utility types are carefully constructed to handle multiple scenarios in property definitions, ensuring robust type checking and flexibility.
139-143:
Verify that adding Props generic parameter does not introduce breaking changes
Adding the new generic parameter Props to PatternOptions might affect existing code that uses PatternOptions without specifying Props. It is crucial to ensure that default values are provided or existing usages are updated to prevent potential breaking changes in the codebase.
Run the following script to find all usages of PatternOptions and check for potential issues:
✅ Verification successful
No breaking changes introduced by the Props generic parameter
The verification shows that:
- The
Propsparameter is defined as optional withundefinedas a valid type:Props extends ComplexPropDefinitions<PropTarget> | undefined - In the implementation, it has a default value:
Props extends ComplexPropDefinitions<PropTarget> | undefined = undefined - The
propsfield in thePatternOptionstype is also optional:props?: Props - All usages found in the codebase are within test cases and the core implementation, where the
Propsparameter is properly handled
Since the parameter is optional with proper default values, and the implementation maintains backward compatibility, this change does not introduce breaking changes to existing code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of `PatternOptions` that may be affected by the new `Props` parameter.
# Find all TypeScript files using `PatternOptions` without the `Props` parameter
rg 'PatternOptions<[^,]*,[^,]*>' -t ts --word-regexp
Length of output: 54
Script:
#!/bin/bash
# Let's try a broader search to find all usages of PatternOptions
rg "PatternOptions" -t ts -A 2 -B 2
Length of output: 3166
Script:
#!/bin/bash
# Let's also check the type definition to understand if Props is optional
ast-grep --pattern 'type PatternOptions<$_,$_,$_> = $_'
Length of output: 1133
Script:
#!/bin/bash
# Search for any pattern function calls that might use PatternOptions
rg "pattern\(" -t ts -A 2 -B 2
Length of output: 32
|
/fast-forward |
|
Triggered from #132 (comment) by @black7375. Trying to fast forward Target branch ( commit c1a1e2d6a1af293560bda8961dbbe05b3f30189b (HEAD -> main, origin/main, origin/HEAD)
Author: JeongJun <[email protected]>
Date: Mon Oct 7 19:59:25 2024 +0900
Test: RuntimeFn Type Test
Co-authored-by: alstjr7375 <[email protected]>Pull request ( commit d0991f539e651abf074aea91ca2dcd3078a0cd08 (pull_request/props-type)
Author: alstjr7375 <[email protected]>
Date: Sat Nov 23 22:51:35 2024 +0900
Fix: Rules - Prop expression type limitation #131Fast forwarding $ git push origin d0991f539e651abf074aea91ca2dcd3078a0cd08:main
To https://github.com/mincho-js/mincho.git
c1a1e2d..d0991f5 d0991f539e651abf074aea91ca2dcd3078a0cd08 -> main |
Description
This is the type for
rules'sprop.Related Issue
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Additional context
The API types handled by Prop have been reduced due to TypeScript inference and autocompletion.
rules's prop expression working-group#14Checklist