-
-
Notifications
You must be signed in to change notification settings - Fork 81
feat: CSS variable tracking #136
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
Changes from 5 commits
d579076
e2810f7
b12f7f0
ddd61eb
0466e46
8da0d12
e13bf02
2324729
2969f9b
a4b7251
2b89b34
4e289e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,128 @@ | ||
| - Repo: eslint/css | ||
| - Start Date: 2025-06-24 | ||
| - RFC PR: | ||
| - Authors: Nicholas C. Zakas | ||
|
|
||
| # CSS Custom Property Tracking in SourceCode | ||
|
|
||
| ## Summary | ||
|
|
||
| This RFC proposes to add capabilities to `CSSSourceCode` to track CSS custom properties (also known as CSS variables). This includes identifying where custom properties are defined and where they are used, as well as providing a way to retrieve the value of a custom property at a specific location in the code. | ||
|
|
||
| ## Motivation | ||
|
|
||
| CSS custom properties are a foundational part of modern CSS development. They allow for more modular and maintainable stylesheets. For a CSS linter to provide accurate and helpful rules, it needs to be able to understand how custom properties are being used. Currently, any rule that needs to validate property values must implement its own logic for tracking custom properties, leading to duplicated effort and potential inconsistencies. | ||
|
|
||
| By building this functionality directly into `CSSSourceCode`, we can provide a consistent and reliable way for all rules to access information about custom properties. Uses of this information include: | ||
|
|
||
| * `no-invalid-properties` rule | ||
| * `font-family-fallbacks` rule | ||
| * Detecting references to undefined custom properties. | ||
| * Future: Detecting unused custom properties. | ||
|
|
||
| This change is based on the discussion in [eslint/css#160](https://github.com/eslint/css/issues/160). | ||
|
|
||
| **Note:** The logic described in this RFC is already implemented in the [`no-invalid-properties`](https://github.com/eslint/css/blob/main/docs/rules/no-invalid-properties.md) rule. This proposal wants to standardize the logic and make it available to all rules. | ||
|
|
||
| ## Detailed Design | ||
|
|
||
| The proposed changes are implemented in the [`CSSSourceCode`](https://github.com/eslint/css/blob/main/src/languages/css-source-code.js) class. | ||
|
|
||
| ### `customProperties` Map | ||
|
|
||
| ```ts | ||
| interface CustomPropertyUses { | ||
| declarations: Array<DeclarationPlain>; | ||
| definitions: Array<AtrulePlain>; | ||
| references: Array<FunctionNode>; | ||
| } | ||
|
|
||
| interface CSSSourceCode { | ||
| #customProperties: Map<string, CustomPropertyUses> | ||
| } | ||
| ``` | ||
|
|
||
| A new private property, `#customProperties`, will be added to `CSSSourceCode`. This will be a `Map` where the keys are the custom property names (e.g., `--my-color`) and the values are `CustomPropertyUses` objects. The `CustomPropertyUses` class will have three properties: | ||
|
|
||
| * `declarations`: An array of `DeclarationPlain` nodes where the custom property value is declared. | ||
| * `definitions`: Array of `AtrulePlain` nodes where the custom property is defined using an [`@property`](https://developer.mozilla.org/en-US/docs/Web/CSS/@property) rule. | ||
fasttime marked this conversation as resolved.
Show resolved
Hide resolved
mdjermanovic marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * `references`: An array of `FunctionNode` nodes (specifically `var()` functions) where the custom property is used. | ||
|
|
||
| ### `getDeclarationVariables()` Method | ||
|
|
||
| ```ts | ||
| interface CSSSourceCode { | ||
| getDeclarationVariables(declaration: DeclarationPlain): Array<Function>; | ||
nzakas marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
| ``` | ||
|
|
||
| A new public method, `getDeclarationVariables(declaration)`, will be added to `CSSSourceCode`. This method will take a `Declaration` node as an argument and return an array of `Function` nodes representing the `var()` functions used in that declaration's value. | ||
|
|
||
| ### `getClosestVariableValue()` Method | ||
|
|
||
| ```ts | ||
| interface CSSSourceCode { | ||
| getClosestVariableValue(func: FunctionNode): Raw; | ||
| } | ||
| ``` | ||
|
|
||
| A new public method, `getClosestVariableValue(node)`, will be added to `CSSSourceCode`. This method will take a `var()` `Function` node as an argument and return the computed value of the custom property (which is currently always a `Raw` node). It will do this by searching for the last declaration of the custom property that appears before the given `Function` node in the source code. This also leaves open the possibility that we could change how this value is calculated to be more accurate in the future. | ||
mdjermanovic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ### `getVariableValues()` Method | ||
|
|
||
| ```ts | ||
| interface CSSSourceCode { | ||
| getVariableValues(func: FunctionNode): Array<Raw>; | ||
| } | ||
| ``` | ||
|
|
||
| A new public method, `getVariableValues(func)`, will be added to `CSSSourceCode`. This method will take a `var()` `Function` node as an argument and return an array of `Raw` nodes representing the declared values of the custom property. The fallback value, if specified in the `FunctionNode`, is returned as the last element of the array. | ||
|
|
||
| ### Initialization | ||
|
|
||
| The `traverse()` method in `CSSSourceCode` will be updated to populate the `#customProperties` map and the internal data structure (`WeakMap<DeclarationPlain, Array<FunctionNode>>`) used by `getDeclarationVariables()`. During traversal, it will identify `Declaration` nodes that define custom properties and `Function` nodes that are `var()` calls. | ||
|
|
||
| ## Documentation | ||
|
|
||
| Because we don't provide documentation for `CSSSourceCode`, we will rely primarily on TypeScript types to inform rule developers as to these new class members. | ||
|
|
||
| ## Drawbacks | ||
|
|
||
| The primary drawback of this approach is that it only tracks custom properties within a single file. It cannot resolve custom properties that are defined in other files or via mechanisms like inline styles on HTML elements. For the initial implementation, this is considered an acceptable tradeoff. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm glad you called this out. This drawback may not be enough to limit this proposal, since it does have some really nice features, but I think it will severely limit the ability to achieve these two goals
I'm not sure how that will be possible without being able to span multiple files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All of this logic is already implemented in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for that context. Maybe I've worked in large corporate codebases for too long, and this is biasing my perspective, but I guess I can't really see a case in any project that I've worked on, where a rule with that limitation could be used. In my experience, anything even reasonably sized will have custom props defined in a different place than where they're used. Whether it be a large design system with design tokens, or just a common set of reusable props that an app uses across its codebase (or both in many cases). In our case, specifically, our design system team also publishes an eslint plugin that enforces use of their design tokens. e.g. if you're defining a color with a raw hex value, flag that as a violation, since that should be using a custom prop from their catalog of tokens. So yeah, I'm not sure my feedback is actionable for this proposal. Just sharing my perspective and somewhat questioning how realistic the larger goal is (in most use cases), when this limitation is in place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with @michaelfaith. Rules like Furthermore, even if the tracker factored in multiple CSS files at a time, it would still suffer from not knowing which variables are used where. It's also common for projects to define variables on something other than Quick reproductions here: https://github.com/JoshuaKGoldberg/repros/tree/eslint-css-variable-tracking Given the constraints of ESLint (in particular that cross-file information is out of scope for this discussion), my opinion is that:
I.e. I think
I think this is actually the reality today. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry, I just don't subscribe to the belief that if we can't get it perfect, we should do nothing. At a minimum it works for some cases, and in the best case, this is a solid foundation upon which we can build. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I do, as I think your perspective on real-world use cases differs from mine. When working on large-scale projects, it's easy to forget that there are a lot of small-scale projects made by hobbyists or self-taught folks who don't necessarily adhere to best practices. (Anecdote: On my personal blog I use a single stylesheet -- I doubt I'm alone in this.) For those folks, this capability will be huge. For folks who do work on large-scale projects where custom properties are located in an extra file, there's zero downside to having this functionality. They just don't get the extra checks, that's it. I don't think there's a valid argument to be made that just because we can't support all use cases means we can't support any use cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I can understand that perspective, it makes sense. The only counterpoint I'd give is:
As currently implemented in eslint/css and proposed in this RFC, I don't believe that's the case? To avoid that, I'd like to suggest:
Does that seem reasonable to you? I can file an issue for the latter if so. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The first case, the false positive, is solved with The second case is not a false negative. As I've explained elsewhere, for the purposes of validation, we only need to know the type of the property. The likelihood that
I feel like the downsides section already does this.
We did have a "Limitations" section on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, thanks. I know this thread is getting long but I have one more suggestion: would it make sense to build the concept of "this project (can/can't) rely on variable tracking" into this RFC? Thinking out loud: right now, multi-file projects need to manually turn off any rule features that rely on variable options. Right now that's just the 1-2 rules but presumably it'll grow over time. Each rule would need to add a user-facing option to turn off variable tracking, and users would need to manually configure each of those options. A couple of ways come to mind to be able to configure in one place:
I think the shared setting approach is more idiomatic. Adding many more configs gets messy, especially if other cross-rule concerns like this one pop up. I'm not sure that the variable tracker itself needs to know about the shared setting. It might make more sense just to have the rules react to the presence of the shared setting. But since we're discussing the tracker here, I wanted to bring it up now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it makes sense to design a solution for a problem we don't have yet. When people start filing issues about such things, we can take a look and see what (if anything) makes sense to address it. |
||
|
|
||
| ## Backwards Compatibility Analysis | ||
|
|
||
| This is a new feature and does not change any existing APIs. It is fully backwards compatible. | ||
|
|
||
| ## Alternatives | ||
|
|
||
| The primary alternative is to have each rule implement its own custom property tracking logic. This is the current state of affairs and is what this RFC aims to improve upon. A centralized approach is more efficient and less error-prone. | ||
|
|
||
| ## Open Questions | ||
|
|
||
| **Should we use "var" instead of "variable" in function names?** | ||
|
|
||
| The proposed API includes methods like `getDeclarationVariables()` and `getVariableValue()`. Since CSS custom properties are commonly referred to as "CSS variables" in the community, there's a question about whether the method names should use the shorter "var" terminology instead. For example: | ||
|
|
||
| - `getDeclarationVars()` instead of `getDeclarationVariables()` | ||
| - `getVarValue()` instead of `getVariableValue()` | ||
|
|
||
| Using "var" would be more concise and align with the CSS `var()` function syntax that developers are already familiar with. However, "variable" is more explicit and follows common naming conventions in programming APIs. | ||
nzakas marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| **Should `getVariableValues()` return the fallback value?** | ||
|
|
||
| The fallback value is already available in the `FunctionNode` passed into `getVariableValues()`, so rule authors can still get access to that value easily. It seems like a nice touch to always have it as the last element of the returned array, but that also means that a non-declared value is present in the array, which could potentially be confusing. | ||
nzakas marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| ## Help Needed | ||
|
|
||
| No help is needed to implement this RFC. | ||
|
|
||
| ## Frequently Asked Questions | ||
|
|
||
| **Why does `getVariableValue()` return a `Raw` instead of a string?** | ||
|
|
||
| The `getVariableValue()` method returns a `Raw` node instead of a string to preserve the original source information and maintain consistency with the AST structure. A `Raw` node contains not only the text value but also the location, which is valuable for rules that need to report issues or apply fixes at specific locations in the source code. Additionally, returning the actual AST node allows for future extensibility. If we later need to return more complex computed values or support different node types, the API won't need to change. | ||
|
|
||
| ## Related Discussions | ||
|
|
||
| - [Change Request: Track variables on SourceCode #160](https.github.com/eslint/css/issues/160) | ||
nzakas marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
Uh oh!
There was an error while loading. Please reload this page.