-
-
Notifications
You must be signed in to change notification settings - Fork 26
fix: no-invalid-properties false positives for var() in functions #227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: no-invalid-properties false positives for var() in functions #227
Conversation
src/rules/no-invalid-properties.js
Outdated
return null; | ||
} | ||
} else if (nestedChild.type === "Function") { | ||
// Recursively process nested functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're doing a traversal on top of the traversal that is already happening in the rule, which is inefficient.
A more efficient approach would be to use the Function
visitor method to do the traversal automatically. Can you take a look at that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into using the Function visitor method here, but I don’t see a straightforward way to apply it. The code at this point isn’t just traversing—it’s reconstructing the property value as a string, including nested function names and arguments. The visitor pattern alone doesn’t provide the context needed for this reconstruction, so a manual traversal seems necessary. If you know of a way to leverage the visitor pattern for this case, I’d appreciate your input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can start by looking at Function > *
and Function > *:exit
. You'd need to use a stack to keep track of the depth you're at, but otherwise, I think that should work.
Can you check the test failures? |
@@ -55,6 +55,37 @@ ruleTester.run("no-invalid-properties", rule, { | |||
":root { --my-color: red; }\na { color: var(--my-color, var(--fallback-color, var(--foo, var(--bar)))) }", | |||
":root { --my-color: red; }\na { color: var(--my-color, var(--fallback-color, var(--foo, var(--bar, blue)))) }", | |||
":root { --color: red }\na { border-top: 1px var(--style, var(--fallback, solid)) var(--color, blue); }", | |||
"a { width: calc(var(--my-width, 100px)) }", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add the following test case as well when the variable is also using calc():
/* eslint css/no-invalid-properties: "error" */
:root { --dynamic-width: calc(20px + 10px); }
div { width: calc(100% - var(--dynamic-width)); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have one false positive, can you please check?
/* eslint css/no-invalid-properties: "error" */
:root {
--dynamic-width: calc(20px + 10px);
--dynamic-width-2: calc(var(--dynamic-width) + 10px);
}
.div { width: calc(100% - var(--dynamic-width)); } // ok
.div2 { width: calc(100% - var(--dynamic-width-2)); } // Error Unknown property 'width' found
Prerequisites checklist
What is the purpose of this pull request?
This PR fixes a regression in the
no-invalid-properties
rule where it incorrectly reports for valid CSS properties containingvar()
usage within functions likecalc()
.What changes did you make? (Give an overview)
Related Issues
Fixes #223
Is there anything you'd like reviewers to focus on?