Test: css(), cssVariants(), rules() #93 #120
Conversation
|
WalkthroughThe changes introduce a new CSS module utilizing the Vanilla Extract CSS framework, providing functions for defining global and component-specific styles. Key functions include Changes
Possibly related PRs
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 #120 by @black7375. Checking if we can fast forward Target branch ( commit d5e2ba2028355f39deb4990d9312faf1d2ea3eda (HEAD -> main, origin/main, origin/HEAD)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 16:34:08 2024 +0900
Fix: CI - Hard Reset at changeset versioning #118Pull request ( commit d2301cb783cb95e046775be9a02560307674f5ed (pull_request/test/rules)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 22:02:50 2024 +0900
Test: `css()`, `cssVariants()`, `rules()` #93It 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: 4
🧹 Outside diff range and nitpick comments (2)
packages/css/src/css/index.ts (2)
80-89: Consider UsingObject.keys()for Object IterationUsing
for...inloops can iterate over inherited enumerable properties, which might lead to unexpected behavior. To safely iterate only over the object's own properties, consider usingObject.keys(items)with afor...ofloop.Apply this diff to refactor the loop:
- for (const key in items) { + for (const key of Object.keys(items)) {
182-182: Address the TODO for Testing Variant ReferencesThe TODO comment suggests that tests for
globalCss()and variant references are pending. Implementing these tests will enhance test coverage and help ensure that variant references are correctly handled.Would you like assistance in creating these test cases or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/css/src/css/index.ts (1 hunks)
- packages/css/src/index.ts (2 hunks)
- packages/css/src/rules/index.ts (4 hunks)
- packages/css/src/utils.ts (1 hunks)
🧰 Additional context used
📓 Learnings (1)
packages/css/src/rules/index.ts (1)
Learnt from: black7375 PR: mincho-js/mincho#113 File: packages/css/src/index.ts:33-33 Timestamp: 2024-09-30T08:48:48.707Z Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
🔇 Additional comments (4)
packages/css/src/utils.ts (1)
1-5: 🛠️ Refactor suggestionConsider enhancing the
classNamefunction for robustness and clarity.The
classNamefunction is a useful utility for creating regular expressions to match class names with debug IDs. However, there are a few points to consider for improvement:
The function name "className" might be misleading as it returns a RegExp, not a string. Consider renaming it to something like
classNamePatternorcreateClassNameRegexfor clarity.The function doesn't handle empty input gracefully. It might be beneficial to add a check for this case and return an appropriate result or throw an error.
The regex pattern
[a-zA-Z0-9]+is quite permissive and might match unintended strings. Depending on your specific use case, you might want to make it more restrictive to ensure it only matches valid class name hash patterns.Here's a suggested refactoring that addresses these points:
export function createClassNameRegex(...debugIds: string[]): RegExp { if (debugIds.length === 0) { throw new Error("At least one debug ID must be provided"); } const hashRegex = "[a-zA-Z0-9]{6}"; // Assuming a 6-character hash const classStr = debugIds.map((id) => `${id}__${hashRegex}`).join(" "); return new RegExp(`^${classStr}$`); }This refactored version:
- Has a more descriptive name.
- Throws an error if no debug IDs are provided.
- Uses a more specific regex pattern for the hash (adjust as needed for your use case).
To ensure this function is used correctly throughout the codebase, you can run the following verification script:
This script will help identify any potential misuse of the function in the codebase.
✅ Verification successful
Usage of the
classNamefunction is correctly implemented with appropriate arguments throughout the codebase.No instances of
classNamebeing called without arguments were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the className function and verify it's called with arguments # Search for usage of className function echo "Searching for className function usage:" rg --type typescript "className\(" -A 2 # Check if there are any calls without arguments echo "\nChecking for calls without arguments:" rg --type typescript "className\(\s*\)" || echo "No calls without arguments found."Length of output: 408
Script:
#!/bin/bash # Description: Check for usage of the className function and verify it's called with arguments # Search for usage of className function in TypeScript files echo "Searching for className function usage in .ts and .tsx files:" rg --type ts "className\(" -A 2 rg --type tsx "className\(" -A 2 # Check if there are any calls without arguments in TypeScript files echo "\nChecking for calls to className without arguments in .ts and .tsx files:" rg --type ts "className\(\s*\)" rg --type tsx "className\(\s*\)" || echo "No calls without arguments found."Length of output: 15120
Script:
#!/bin/bash # Description: Check for usage of the className function and verify it's called with arguments # Search for usage of className function in .ts and .tsx files echo "Searching for className function usage in .ts and .tsx files:" rg -g '*.ts' "className\(" -A 2 rg -g '*.tsx' "className\(" -A 2 # Check if there are any calls to className without arguments in .ts and .tsx files echo "\nChecking for calls to className without arguments in .ts and .tsx files:" rg -g '*.ts' "className\(\s*\)" rg -g '*.tsx' "className\(\s*\)" || echo "No calls without arguments found."Length of output: 15064
packages/css/src/index.ts (1)
28-35: Refactoring exports improves modularity and maintainabilityRe-exporting
globalCss,globalStyle,css,style,cssVariants, andstyleVariantsfrom./csscentralizes the CSS functionality. This enhances code organization and promotes easier maintenance.packages/css/src/css/index.ts (1)
81-81:⚠️ Potential issueVerify Support for
structuredClonein Target EnvironmentsThe use of
structuredClonemay not be supported in all environments, particularly in older Node.js versions or browsers. To ensure compatibility, verify that your target runtime environments supportstructuredClone. If not, consider using an alternative deep cloning method or a polyfill.packages/css/src/rules/index.ts (1)
115-115: Verify importingassertfromimport.meta.vitestIn line 115, you are destructuring
assertfromimport.meta.vitest. Typically,assertis imported fromnode:assertorvitest. Ensure thatassertis correctly imported to prevent any issues with your test assertions.Run the following script to check how
assertis imported and used elsewhere in the codebase:
| context.variantMap = variantMap; | ||
| replaceVariantReference(context); | ||
| for (const [key, value] of Object.entries(context.variantReference)) { | ||
| globalCss(key, value as CSSRule); |
There was a problem hiding this comment.
Avoid Unnecessary Type Casting with as CSSRule
Casting value to CSSRule using as CSSRule might mask potential type issues. Verify that value is indeed of type CSSRule, and adjust the type definitions if necessary. Eliminating unnecessary type casts enhances type safety and code reliability.
| "size", | ||
| "outlined" | ||
| ]); | ||
| expect(result.variants()).toEqual(["color", "size", "outlined"]); | ||
|
|
||
| // Each variant className check | ||
| assert.hasAllKeys(result.classNames.variants.color, ["brand", "accent"]); | ||
| expect(result.classNames.variants.color.brand).toMatch( | ||
| className(`${debugId}_color_brand`) | ||
| ); | ||
| expect(result.classNames.variants.color.accent).toMatch( | ||
| className(`${debugId}_color_accent`) | ||
| ); | ||
|
|
||
| assert.hasAllKeys(result.classNames.variants.size, [ | ||
| "small", | ||
| "medium", | ||
| "large" | ||
| ]); | ||
| expect(result.classNames.variants.size.small).toMatch( | ||
| className(`${debugId}_size_small`) | ||
| ); | ||
| expect(result.classNames.variants.size.medium).toMatch( | ||
| className(`${debugId}_size_medium`) | ||
| ); | ||
| expect(result.classNames.variants.size.large).toMatch( | ||
| className(`${debugId}_size_large`) | ||
| ); | ||
|
|
||
| assert.hasAllKeys(result.classNames.variants.outlined, ["true", "false"]); | ||
| expect(result.classNames.variants.outlined.true).toMatch( | ||
| className(`${debugId}_outlined_true`) | ||
| ); | ||
| expect(result.classNames.variants.outlined.false).toMatch( | ||
| className(`${debugId}_outlined_false`) | ||
| ); | ||
|
|
||
| // Compose variant className check | ||
| expect(result()).toMatch(className(debugId)); | ||
| expect(result({ color: "brand" })).toMatch( | ||
| className(debugId, `${debugId}_color_brand`) | ||
| ); | ||
| expect(result({ size: "small" })).toMatch( | ||
| className(debugId, `${debugId}_size_small`) | ||
| ); | ||
| expect(result({ outlined: true })).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
| expect(result(["outlined"])).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
|
|
||
| expect(result({ color: "brand", size: "small" })).toMatch( | ||
| className(debugId, `${debugId}_color_brand`, `${debugId}_size_small`) | ||
| ); | ||
| expect(result({ size: "small", color: "brand" })).toMatch( | ||
| className(debugId, `${debugId}_size_small`, `${debugId}_color_brand`) | ||
| ); | ||
| expect(result(["outlined", { color: "brand" }])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_outlined_true`, | ||
| `${debugId}_color_brand`, | ||
| // Compound | ||
| `${debugId}_compound_0` | ||
| ) | ||
| ); | ||
| expect(result([{ color: "brand" }, "outlined"])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_color_brand`, | ||
| `${debugId}_outlined_true`, | ||
| // Compound | ||
| `${debugId}_compound_0` | ||
| ) | ||
| ); | ||
| expect(result(["outlined", { color: "brand", size: "medium" }])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_outlined_true`, | ||
| `${debugId}_color_brand`, | ||
| `${debugId}_size_medium`, | ||
| // Compound | ||
| `${debugId}_compound_0`, | ||
| `${debugId}_compound_1` | ||
| ) | ||
| ); | ||
|
|
||
| expect(result([{ color: "brand" }, { color: "accent" }])).toMatch( | ||
| className(debugId, `${debugId}_color_accent`) | ||
| ); | ||
| expect(result(["outlined", { outlined: false }])).toMatch( | ||
| className(debugId, `${debugId}_outlined_false`) | ||
| ); | ||
| expect(result(["outlined", { outlined: false }, "outlined"])).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify assertion styles in tests
Throughout the test cases, both assert and expect are used for assertions. Mixing assertion styles might reduce readability and maintainability. Consider using a single assertion library consistently across all tests.
You could replace all assert statements with expect for consistency:
- assert.isFunction(result);
+ expect(typeof result).toBe('function');Or vice versa, depending on your preference.
Committable suggestion was skipped due to low confidence.
|
Triggered from #120 by @black7375. Checking if we can fast forward Target branch ( commit d5e2ba2028355f39deb4990d9312faf1d2ea3eda (HEAD -> main, origin/main, origin/HEAD)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 16:34:08 2024 +0900
Fix: CI - Hard Reset at changeset versioning #118Pull request ( commit 5e6f6310022254b94a4447a27b545a0895e2ebba (pull_request/test/rules)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 22:02:50 2024 +0900
Test: `css()`, `cssVariants()`, `rules()` #93It is possible to fast forward |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/css/src/index.ts (1)
36-42: Approve new exports and suggest documentation updates.The introduction of new exports from the "./rules" module and new type exports from "./rules/types" enhances the functionality of the package. The preservation of existing type exports maintains backward compatibility.
Consider updating the package documentation to reflect these new exports and types, especially
rules,recipe,RulesVariants,RecipeVariants, andRuntimeFn. This will help users understand and utilize the new functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/css/src/css/index.ts (1 hunks)
- packages/css/src/index.ts (2 hunks)
- packages/css/src/rules/index.ts (4 hunks)
- packages/css/src/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/css/src/css/index.ts
- packages/css/src/utils.ts
🧰 Additional context used
📓 Learnings (1)
packages/css/src/rules/index.ts (1)
Learnt from: black7375 PR: mincho-js/mincho#113 File: packages/css/src/index.ts:33-33 Timestamp: 2024-09-30T08:48:48.707Z Learning: `recipe` is properly exported from './rules' in `packages/css/src/rules/index.ts`.
🔇 Additional comments (5)
packages/css/src/index.ts (2)
Line range hint
1-26: LGTM: Import statements and type exports are maintained.The import statements for types and the export statements for types and functions from external libraries are preserved. This indicates that the core functionality and type definitions are still required and used in the project.
28-35: Approve refactoring, but verify functionality of new exports.The introduction of new exports from the local "./css" module aligns with the PR objectives of restructuring the index.ts file. This change potentially improves code organization and maintainability.
Please ensure that the new exports (globalCss, globalStyle, css, style, cssVariants, styleVariants) provide the same functionality as the removed functions. Run the following script to verify the exports:
✅ Verification successful
Verification script update required due to unsupported file type.
Please run the following scripts to verify the exports from the
./cssmodule with corrected commands:
Verified the functionality of new exports.
The new exports (
globalCss,globalStyle,css,style,cssVariants,styleVariants) are correctly defined in the./cssmodule. This ensures that the refactoring maintains existing functionality and improves code organization.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the exports from the ./css module # Test: Check if the ./css module exists and exports the expected functions ast-grep --lang typescript --pattern 'export { $_ } from "./css"' # Test: Verify that the exported functions are defined in the ./css module rg --type typescript -e 'export (const|function) (globalCss|globalStyle|css|style|cssVariants|styleVariants)' packages/css/src/css/Length of output: 236
Script:
#!/bin/bash # Description: Verify the exports from the ./css module # Test: Check if the ./css module exists and exports the expected functions ast-grep --lang typescript --pattern 'export { $_ } from "./css"' # Test: Verify that the exported functions are defined in the ./css module rg -e 'export (const|function) (globalCss|globalStyle|css|style|cssVariants|styleVariants)' packages/css/src/css/Length of output: 746
packages/css/src/rules/index.ts (3)
3-3: LGTM: Import changes and function updatesThe changes to the imports,
rulesfunction, and the newrecipeexport improve type safety and align with the PR objectives of restructuring the codebase. The explicit return type for therulesfunction enhances type checking, which is a good practice.Also applies to: 6-7, 44-44, 46-46, 95-95, 113-113
114-120: LGTM: Test suite setupThe addition of a comprehensive test suite using
vitestis a significant improvement. It aligns with the PR objectives of introducing test codes forcss(),cssVariants(), andrules(). The use ofsetFileScope("test")ensures proper isolation for the tests.
Line range hint
1-538: Overall, great improvements to the codebaseThe changes in this file significantly enhance the code quality and testability. The updated imports, function changes, and comprehensive test suite align well with the PR objectives. The suggestions for refactoring test cases and adding edge case tests are minor improvements that can further boost the robustness of the code.
Great job on this substantial update!
| assert.hasAllKeys(result.classNames.variants, [ | ||
| "color", | ||
| "size", | ||
| "outlined" | ||
| ]); | ||
| expect(result.variants()).toEqual(["color", "size", "outlined"]); | ||
|
|
||
| // Each variant className check | ||
| assert.hasAllKeys(result.classNames.variants.color, ["brand", "accent"]); | ||
| expect(result.classNames.variants.color.brand).toMatch( | ||
| className(`${debugId}_color_brand`) | ||
| ); | ||
| expect(result.classNames.variants.color.accent).toMatch( | ||
| className(`${debugId}_color_accent`) | ||
| ); | ||
|
|
||
| assert.hasAllKeys(result.classNames.variants.size, [ | ||
| "small", | ||
| "medium", | ||
| "large" | ||
| ]); | ||
| expect(result.classNames.variants.size.small).toMatch( | ||
| className(`${debugId}_size_small`) | ||
| ); | ||
| expect(result.classNames.variants.size.medium).toMatch( | ||
| className(`${debugId}_size_medium`) | ||
| ); | ||
| expect(result.classNames.variants.size.large).toMatch( | ||
| className(`${debugId}_size_large`) | ||
| ); | ||
|
|
||
| assert.hasAllKeys(result.classNames.variants.outlined, ["true", "false"]); | ||
| expect(result.classNames.variants.outlined.true).toMatch( | ||
| className(`${debugId}_outlined_true`) | ||
| ); | ||
| expect(result.classNames.variants.outlined.false).toMatch( | ||
| className(`${debugId}_outlined_false`) | ||
| ); | ||
|
|
||
| // Compose variant className check | ||
| expect(result()).toMatch(className(debugId)); | ||
| expect(result({ color: "brand" })).toMatch( | ||
| className(debugId, `${debugId}_color_brand`) | ||
| ); | ||
| expect(result({ size: "small" })).toMatch( | ||
| className(debugId, `${debugId}_size_small`) | ||
| ); | ||
| expect(result({ outlined: true })).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
| expect(result(["outlined"])).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
|
|
||
| expect(result({ color: "brand", size: "small" })).toMatch( | ||
| className(debugId, `${debugId}_color_brand`, `${debugId}_size_small`) | ||
| ); | ||
| expect(result({ size: "small", color: "brand" })).toMatch( | ||
| className(debugId, `${debugId}_size_small`, `${debugId}_color_brand`) | ||
| ); | ||
| expect(result(["outlined", { color: "brand" }])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_outlined_true`, | ||
| `${debugId}_color_brand`, | ||
| // Compound | ||
| `${debugId}_compound_0` | ||
| ) | ||
| ); | ||
| expect(result([{ color: "brand" }, "outlined"])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_color_brand`, | ||
| `${debugId}_outlined_true`, | ||
| // Compound | ||
| `${debugId}_compound_0` | ||
| ) | ||
| ); | ||
| expect(result(["outlined", { color: "brand", size: "medium" }])).toMatch( | ||
| className( | ||
| debugId, | ||
| `${debugId}_outlined_true`, | ||
| `${debugId}_color_brand`, | ||
| `${debugId}_size_medium`, | ||
| // Compound | ||
| `${debugId}_compound_0`, | ||
| `${debugId}_compound_1` | ||
| ) | ||
| ); | ||
|
|
||
| expect(result([{ color: "brand" }, { color: "accent" }])).toMatch( | ||
| className(debugId, `${debugId}_color_accent`) | ||
| ); | ||
| expect(result(["outlined", { outlined: false }])).toMatch( | ||
| className(debugId, `${debugId}_outlined_false`) | ||
| ); | ||
| expect(result(["outlined", { outlined: false }, "outlined"])).toMatch( | ||
| className(debugId, `${debugId}_outlined_true`) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring test cases to reduce duplication
The test cases are comprehensive and cover various scenarios, which is excellent. However, there's some repetition in the assertion patterns, especially in checking the structure of the result object and its properties. Consider refactoring these common assertions into helper functions to improve maintainability and readability.
For example, you could create a helper function like:
function assertBaseResultStructure(result: ReturnType<typeof rules>) {
assert.isFunction(result);
assert.hasAllKeys(result, ["variants", "classNames"]);
assert.hasAllKeys(result.classNames, ["base", "variants"]);
}Then use this helper in your tests:
it("base style", () => {
const result = rules({ base: { color: "red" } }, debugId);
assertBaseResultStructure(result);
// ... rest of the specific assertions
});This approach would make the tests more concise and easier to maintain.
🛠️ Refactor suggestion
Consider adding tests for edge cases and error scenarios
The test suite is comprehensive and well-structured, covering the main functionalities of the rules function. To further improve the robustness of the tests, consider adding test cases for edge cases and error scenarios. Some suggestions:
- Test with empty variant objects or undefined values.
- Test with invalid variant combinations.
- Test error handling when passing incorrect types to the function.
- Test with very large numbers of variants to ensure performance.
These additional tests will help catch potential issues and ensure the function behaves correctly under various conditions.
|
/fast-forward |
|
Triggered from #120 (comment) by @black7375. Trying to fast forward Target branch ( commit d5e2ba2028355f39deb4990d9312faf1d2ea3eda (HEAD -> main, origin/main, origin/HEAD)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 16:34:08 2024 +0900
Fix: CI - Hard Reset at changeset versioning #118Pull request ( commit 5e6f6310022254b94a4447a27b545a0895e2ebba (pull_request/test/rules)
Author: alstjr7375 <alstjr7375@daum.net>
Date: Tue Oct 1 22:02:50 2024 +0900
Test: `css()`, `cssVariants()`, `rules()` #93Fast forwarding $ git push origin 5e6f6310022254b94a4447a27b545a0895e2ebba:main
To https://github.com/mincho-js/mincho.git
d5e2ba2..5e6f631 5e6f6310022254b94a4447a27b545a0895e2ebba -> main |
Description
index.tsto./css/index.ts:index.tsfocuses only on export.css(),cssVariants(),rules()rules()'s base style debugIddefaultVariantsresolve at buildtimeRelated Issue
Summary by CodeRabbit
New Features
classNamefor constructing class name patterns.Bug Fixes
Tests
Additional context
Checklist