fix: recursively resolve custom properties in no-invalid-properties #237
Conversation
lumirlumir
added
the
accepted
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the no-invalid-properties ESLint rule where CSS custom properties containing chained var() references were not being fully resolved before validation, causing lexer errors. The fix implements recursive resolution of custom properties to ensure all variables are resolved to their final values before being passed to the CSS lexer.
Key changes:
- Added recursive resolution logic for chained CSS custom properties
- Refactored variable resolution to handle fallback chains and cycles
- Added comprehensive test coverage for various chaining scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/rules/no-invalid-properties.js |
Implemented recursive variable resolution with new helper functions and refactored the main validation logic |
tests/rules/no-invalid-properties.test.js |
Added extensive test cases covering valid and invalid chained variable scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!fallbackText) { | ||
| continue; | ||
| } | ||
| // eslint-disable-next-line no-use-before-define -- resolveFallback and resolveVariable are mutually recursive |
There was a problem hiding this comment.
The mutual recursion between resolveFallback and resolveVariable creates a complex dependency. Consider refactoring to eliminate the need for the ESLint disable comment by restructuring the code or moving one function definition before the other.
Copilot uses AI. Check for mistakes.
| while (true) { | ||
| if (seen.has(currentVarName)) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
[nitpick] The infinite loop with break statements makes the control flow harder to follow. Consider using a more explicit loop condition or refactoring to a recursive approach with proper termination conditions.
| } | |
| while (!seen.has(currentVarName) && vars.has(currentVarName)) { |
Copilot uses AI. Check for mistakes.
| } | ||
| // eslint-disable-next-line no-use-before-define -- resolveFallback and resolveVariable are mutually recursive | ||
| const resolvedFallback = resolveFallback( | ||
| fallbackText, |
There was a problem hiding this comment.
[nitpick] The fallback resolution logic using a stack and while loop adds complexity. Consider simplifying this by handling fallbacks more directly in the main resolution loop or using recursion consistently.
| fallbackText, | |
| if (seen.has(variableName)) { | |
| return null; | |
| } | |
| seen.add(variableName); | |
| if (cache.has(variableName)) { | |
| return cache.get(variableName); | |
| } | |
| const valueNode = vars.get(variableName); | |
| if (!valueNode) { | |
| return null; | |
| } | |
| const valueText = sourceCode.getText(valueNode).trim(); | |
| const parsed = parseVarFunction(valueText); | |
| if (!parsed) { | |
| cache.set(variableName, valueText); | |
| return valueText; | |
| } | |
| // Try to resolve the referenced variable | |
| const resolved = resolveVariable(parsed.name, cache, seen); | |
| if (resolved !== null) { | |
| return resolved; | |
| } | |
| // If unresolved, try the fallback if present | |
| if (parsed.fallbackText) { | |
| // eslint-disable-next-line no-use-before-define -- resolveFallback and resolveVariable are mutually recursive | |
| const resolvedFallback = resolveFallback( | |
| parsed.fallbackText, |
Copilot uses AI. Check for mistakes.
| const fallbackStack = []; | ||
| let currentVarName = variableName; | ||
|
|
||
| while (true) { |
There was a problem hiding this comment.
Can you add some comments in this function to explain the algorithm?
| * @returns {string | null} | ||
| */ | ||
| function resolveFallback(rawFallbackText, cache, seen = new Set()) { | ||
| const trimmedFallback = rawFallbackText.trim(); |
There was a problem hiding this comment.
The fallback text is already trimmed in parseVarFunction(), so this isn't needed here.
| @@ -161,6 +270,8 @@ export default { | |||
|
|
|||
| if (usingVars) { | |||
| const valueList = []; | |||
| /** @type {Map<string,string>} */ | |||
There was a problem hiding this comment.
Can we add a comment here explaining what resolvedCache is used for?
Prerequisites checklist
What is the purpose of this pull request?
This PR prevents the lexer from receiving unresolved
var()expressions by recursively resolving chained CSS custom properties. Previously the rule only performed a single-level replacement (e.g.--a: var(--b) → var(--b)), so chained variables like--a: var(--b); --b: 80px;could still leave var() in the value passed to the lexer, causingError: Matching for a tree with var() is not supported.What changes did you make? (Give an overview)
Related Issues
Fixes #200
Is there anything you'd like reviewers to focus on?