Feat: rules() props runtime #131#150
Conversation
|
|
Warning Rate limit exceeded@black7375 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 57 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughThis pull request enhances the CSS rules module by refining type definitions and function signatures. The changes introduce a new generic parameter for property definitions in the runtime function, update the handling of CSS properties, and add utility support for extracting CSS variable names. Updates span multiple files including core logic, types, and tests, thereby ensuring tighter type safety and improved property processing in CSS rule generation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RuntimeFn
participant Config
Client->>RuntimeFn: Invoke createRuntimeFn with config & props
RuntimeFn->>Config: Retrieve propVars mapping
RuntimeFn->>RuntimeFn: Process each prop using runtimeFn.props method
alt CSS variable found
RuntimeFn->>RuntimeFn: Map prop value to corresponding CSS variable
else No variable match
RuntimeFn->>RuntimeFn: Skip property
end
RuntimeFn-->>Client: Return constructed CSSRule object
Possibly related PRs
Suggested labels
Poem
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 #150 by @black7375. Checking if we can fast forward Target branch ( commit a277d8b52a1c50b847c08eefb06345e3de238509 (HEAD -> main, origin/main, origin/HEAD)
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date: Mon Feb 10 01:10:20 2025 +0000
Chore(deps-dev): Bump the dev-dependencies group with 3 updates
Bumps the dev-dependencies group with 3 updates: [prettier](https://github.com/prettier/prettier), [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react-swc) and [eslint-plugin-react-refresh](https://github.com/ArnaudBarre/eslint-plugin-react-refresh).
Updates `prettier` from 3.4.2 to 3.5.0
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](https://github.com/prettier/prettier/compare/3.4.2...3.5.0)
Updates `@vitejs/plugin-react-swc` from 3.7.2 to 3.8.0
- [Release notes](https://github.com/vitejs/vite-plugin-react-swc/releases)
- [Changelog](https://github.com/vitejs/vite-plugin-react-swc/blob/main/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite-plugin-react-swc/compare/v3.7.2...v3.8.0)
Updates `eslint-plugin-react-refresh` from 0.4.18 to 0.4.19
- [Release notes](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/releases)
- [Changelog](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/blob/main/CHANGELOG.md)
- [Commits](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/compare/v0.4.18...v0.4.19)
---
updated-dependencies:
- dependency-name: prettier
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: "@vitejs/plugin-react-swc"
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: eslint-plugin-react-refresh
dependency-type: direct:development
update-type: version-update:semver-patch
dependency-group: dev-dependencies
...
Signed-off-by: dependabot[bot] <support@github.com>Pull request ( commit afb191ffb212fa21044fa5b5b746afef2e2f4488 (pull_request/runtime-props)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Mon Feb 10 01:28:58 2025 +0900
Feat: `rules()` props runtime #131It is possible to fast forward |
black7375
force-pushed
the
runtime-props
branch
from
afb191f to
7899606
Compare
|
Triggered from #150 by @black7375. Checking if we can fast forward Target branch ( commit a277d8b52a1c50b847c08eefb06345e3de238509 (HEAD -> main, origin/main, origin/HEAD)
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date: Mon Feb 10 01:10:20 2025 +0000
Chore(deps-dev): Bump the dev-dependencies group with 3 updates
Bumps the dev-dependencies group with 3 updates: [prettier](https://github.com/prettier/prettier), [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react-swc) and [eslint-plugin-react-refresh](https://github.com/ArnaudBarre/eslint-plugin-react-refresh).
Updates `prettier` from 3.4.2 to 3.5.0
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](https://github.com/prettier/prettier/compare/3.4.2...3.5.0)
Updates `@vitejs/plugin-react-swc` from 3.7.2 to 3.8.0
- [Release notes](https://github.com/vitejs/vite-plugin-react-swc/releases)
- [Changelog](https://github.com/vitejs/vite-plugin-react-swc/blob/main/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite-plugin-react-swc/compare/v3.7.2...v3.8.0)
Updates `eslint-plugin-react-refresh` from 0.4.18 to 0.4.19
- [Release notes](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/releases)
- [Changelog](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/blob/main/CHANGELOG.md)
- [Commits](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/compare/v0.4.18...v0.4.19)
---
updated-dependencies:
- dependency-name: prettier
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: "@vitejs/plugin-react-swc"
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: eslint-plugin-react-refresh
dependency-type: direct:development
update-type: version-update:semver-patch
dependency-group: dev-dependencies
...
Signed-off-by: dependabot[bot] <support@github.com>Pull request ( commit 7899606780aa81a3e6efd5dacfe4184ba9713c7e (pull_request/runtime-props)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Mon Feb 10 01:33:02 2025 +0900
Feat: Rules - `props()` runtime #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
🧹 Nitpick comments (5)
packages/css/src/rules/createRuntimeFn.ts (1)
78-90: Confirm behavior of runtimeFn.props with various prop value types.By assigning
result[varName] = propValue as string;, values like numbers or booleans are coerced to strings without explicit checking. If numeric or boolean prop values are expected, consider converting them withString(propValue)to avoid confusion. Otherwise, ensure that only string-compatible values are passed to maintain type consistency.packages/css/src/rules/types.ts (1)
70-71: ComplexPropDefinitions usability check.Allowing an array of
PropKeys | PropDefinition<PropKeys>is flexible, but increases complexity. Consider documenting the expected shape for arrays containing mixed string and object definitions for future maintainability.packages/css/src/utils.ts (2)
11-23: Add input validation to enhance robustness.While the implementation is optimized and handles all cases correctly, it could benefit from input validation.
Consider adding input validation:
export function getVarName(variable: string): PureCSSVarKey { + if (typeof variable !== 'string') { + throw new TypeError('Variable must be a string'); + } if (variable.startsWith("var(") && variable.endsWith(")")) { const inside = variable.slice(VAR_PREFIX_LENGTH, -1); const commaIndex = inside.indexOf(","); return ( commaIndex === -1 ? inside : inside.slice(0, commaIndex) ) as PureCSSVarKey; } return variable as PureCSSVarKey; }
36-62: Add edge case tests for improved coverage.The test suite is comprehensive but could benefit from additional edge cases.
Consider adding these test cases:
describe("getVarName", () => { + it("edge cases", () => { + expect(() => getVarName(null as any)).toThrow(TypeError); + expect(() => getVarName(undefined as any)).toThrow(TypeError); + expect(getVarName("var()")).toBe("var()"); + expect(getVarName("var(--name,)")).toBe("--name"); + }); it("string", () => { // existing tests...packages/transform-to-vanilla/src/types/style-rule.ts (1)
129-140: Enhance type safety for CSS variable functions.The type definitions are well-structured, but the
PureCSSVarFunctiontype could be more precise.Consider this improvement:
-export type PureCSSVarFunction = - | `var(--${string})` - | `var(--${string}, ${CSSVarValue})`; +export type PureCSSVarKey = `--${string}`; +export type PureCSSVarFunction = + | `var(${PureCSSVarKey})` + | `var(${PureCSSVarKey}, ${CSSVarValue})`; export type CSSVarFunction = | PureCSSVarFunction | `$${string}` | `$${string}(${CSSVarValue})`;This change:
- Reuses the
PureCSSVarKeytype for better consistency- Makes the relationship between variable keys and functions more explicit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/css/src/rules/createRuntimeFn.ts(3 hunks)packages/css/src/rules/index.ts(12 hunks)packages/css/src/rules/types.ts(7 hunks)packages/css/src/utils.ts(1 hunks)packages/transform-to-vanilla/src/types/style-rule.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
packages/css/src/rules/createRuntimeFn.ts (2)
1-11: Ensure consistent type usage in imports.The new imports and type references look consistent and correct. No issues found regarding correctness or security.
29-35: Validate new generic parameter addition.The updated function signature now handles two generic parameters
<Variants, Props>, providing greater extensibility for property definitions. Ensure any external calls tocreateRuntimeFnare updated accordingly to supply the second generic parameter when necessary (or allow it to infer, if intended).packages/css/src/rules/types.ts (5)
2-9: Imports appear consistent.Bringing in
PureCSSVarKeyand other types from@mincho-js/transform-to-vanillaaligns well with the extended property definitions.
93-111: Optional property handling in HandlePropTarget and PropDefinitionOutputElement.Making target properties optional (e.g., using
?) can help with type safety but may cause partial definitions to pass inadvertently. Verify that downstream usage handles the absence of these properties gracefully.
117-128: Enhanced property variable mappings with PropVars.Exposing
propVars: PropVars<Props>inPatternResultis a robust approach for referencing CSS variable names. Ensure code outside this module enforces the correct mapping of property names to these variables to avoid runtime confusion.
168-172: RuntimeFn signature consolidation.Merging a callable function with an object that includes
.props,.variants, and.classNamesoffers a complete interface. Double-check that all usage sites handle both function invocation and property access patterns consistently.
177-189: RulesVariants & RecipeVariants expansions.The updated utility types that incorporate property definitions maintain strong alignment with the rest of the codebase. These expansions appear correct. No immediate concerns.
packages/css/src/rules/index.ts (8)
2-12: Imports and utility references.Importing
createVar,fallbackVar, and referencingclassName/getVarNameextends the library’s functionality for property-based variable generation. Implementation looks cohesive.
45-48: Return type improvement.Returning
RuntimeFn<ConditionalVariants<Variants, ToggleVariants>, Exclude<Props, undefined>>explicitly clarifies the presence of props in the final runtime function. Verify any external calls expecting the old return signature handle the new pattern properly.
54-55: Explicit grouping of base and props.Separating
propsfrom other style definitions in the destructuring clarifies the boundary between standard styling and property-based variables. This is a readable and maintainable approach.
59-77: Initialize and populate propVars & propStyles.Good approach for simultaneously defining the variable mappings (
propVars) and referencing them inpropStyles. However, ensure that properties initial setup remains consistent if new properties are added dynamically at runtime.
78-90: Base class composition with propStyles.Lines 80-89 handle merging base, baseStyles, and propStyles. This approach effectively consolidates the final base class name. No immediate issues spotted.
126-132: Inclusion of propVars in PatternResult.Storing
propVarsin the config object is consistent with other pattern aspects (variantClassNames,compoundVariants). This ensures all property-based variables remain accessible to the runtime function.
150-167: processPropObject for structured prop definitions.This helper systematically creates variables for each key in the prop definition and emits fallback references to a
basevalue if provided. UsingfallbackVarensures graceful handling of missing or undefined properties.
605-716: Test coverage for props scenario.The test cases thoroughly verify partial vs. full props usage, ensuring extraneous keys are ignored. This coverage is excellent and should be maintained for additional property scenarios.
packages/css/src/utils.ts (1)
1-3: LGTM! Clean and focused imports.The imports are well-organized and properly scoped to the required functionality.
|
/fast-forward |
|
Triggered from #150 (comment) by @black7375. Trying to fast forward Target branch ( commit a277d8b52a1c50b847c08eefb06345e3de238509 (HEAD -> main, origin/main, origin/HEAD)
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date: Mon Feb 10 01:10:20 2025 +0000
Chore(deps-dev): Bump the dev-dependencies group with 3 updates
Bumps the dev-dependencies group with 3 updates: [prettier](https://github.com/prettier/prettier), [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react-swc) and [eslint-plugin-react-refresh](https://github.com/ArnaudBarre/eslint-plugin-react-refresh).
Updates `prettier` from 3.4.2 to 3.5.0
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/main/CHANGELOG.md)
- [Commits](https://github.com/prettier/prettier/compare/3.4.2...3.5.0)
Updates `@vitejs/plugin-react-swc` from 3.7.2 to 3.8.0
- [Release notes](https://github.com/vitejs/vite-plugin-react-swc/releases)
- [Changelog](https://github.com/vitejs/vite-plugin-react-swc/blob/main/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite-plugin-react-swc/compare/v3.7.2...v3.8.0)
Updates `eslint-plugin-react-refresh` from 0.4.18 to 0.4.19
- [Release notes](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/releases)
- [Changelog](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/blob/main/CHANGELOG.md)
- [Commits](https://github.com/ArnaudBarre/eslint-plugin-react-refresh/compare/v0.4.18...v0.4.19)
---
updated-dependencies:
- dependency-name: prettier
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: "@vitejs/plugin-react-swc"
dependency-type: direct:development
update-type: version-update:semver-minor
dependency-group: dev-dependencies
- dependency-name: eslint-plugin-react-refresh
dependency-type: direct:development
update-type: version-update:semver-patch
dependency-group: dev-dependencies
...
Signed-off-by: dependabot[bot] <support@github.com>Pull request ( commit 7899606780aa81a3e6efd5dacfe4184ba9713c7e (pull_request/runtime-props)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Mon Feb 10 01:33:02 2025 +0900
Feat: Rules - `props()` runtime #131Fast forwarding $ git push origin 7899606780aa81a3e6efd5dacfe4184ba9713c7e:main
To https://github.com/mincho-js/mincho.git
a277d8b..7899606 7899606780aa81a3e6efd5dacfe4184ba9713c7e -> main |
1 participant
Description
Implement
rules()props.Related Issue
Summary by CodeRabbit
New Features
Tests
Refactor
Additional context
Checklist