feat: CSS variable tracking

[nzakas]

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.

Related Issues

refs eslint/css#160

[michaelfaith]

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

• Detecting references to undefined custom properties.
• Future: Detecting unused custom properties.

I'm not sure how that will be possible without being able to span multiple files.

[nzakas]

All of this logic is already implemented in the no-invalid-properties rule and it works pretty well. We call out the limitations. The alternative of doing nothing means any property value containing a variable can't be validated or otherwise inspected in any way, which I don't think is acceptable.

[michaelfaith]

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.

[JoshuaKGoldberg]

I agree with @michaelfaith. Rules like no-invalid-properties as written today only work on a very small scale. Most -I would wager nearly all- modern codebases split their styles across many files. The (roughly) simplest common approach is to have variables declared in one file and then used in other files. Even that approach completely breaks the current & proposed variable tracking logic.

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 :root, such as a specific page container. Any case where variables are added/changed in specific element scopes would be broken too.

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:

1. Any CSS variables tracking done by ESLint or its rules is not usable for the majority of ESLint's prospective users
2. Because of how broken variable tracking is in the common case, it would only be reasonable for depending on it to be an opt-in choice of users
3. That choice is such a rare one, it would not be worth the technical investment to build it into eslint/css and/or its rules

I.e. I think no-invalid-properties and any other rules depending on variable tracking should be changed to no longer do so. Because:

any property value containing a variable can't be validated or otherwise inspected in any way

I think this is actually the reality today.

[nzakas]

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.

[nzakas]

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.

[JoshuaKGoldberg]

👍 I can understand that perspective, it makes sense. The only counterpoint I'd give is:

zero downside ... They just don't get the extra checks, that's it.

As currently implemented in eslint/css and proposed in this RFC, I don't believe that's the case? no-invalid-properties falsely reports in many cases. That's what I'm trying to avoid: setting up rules to falsely report on a large percentage of users.

To avoid that, I'd like to suggest:

• This RFC explicitly call out the kinds of file setups that do and do not work well with its proposed tracking
• Starting a precedent of rules that rely on variable tracking (or other, similarly nuanced "works in only some setups" behavior) do something to make sure projects not set up for them don't enable them. Maybe: documentation? Different config(s) and/or shared settings indicating project architecture style?

Does that seem reasonable to you? I can file an issue for the latter if so.

[nzakas]

As currently implemented in eslint/css and proposed in this RFC, I don't believe that's the case? no-invalid-properties falsely reports in many cases. That's what I'm trying to avoid: setting up rules to falsely report on a large percentage of users.

The first case, the false positive, is solved with ignoreUnknownVariables, so I don't see that as an issue. We have the escape hatch there so people can use it.

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 --colorForeground will ever be anything other than a type compatible with <color> is vanishingly small. In this example, because --colorForeground is a color and it's used inside of a color property, it should be considered valid. It really doesn't matter if we can tell the exact value of --colorForeground in any given rule, which is functionally impossible without having all of the CSS and all of the HTML to evaluate together -- it just matters that we know the type of data that --colorForeground represents, and we do.

• This RFC explicitly call out the kinds of file setups that do and do not work well with its proposed tracking

I feel like the downsides section already does this.

• Starting a precedent of rules that rely on variable tracking (or other, similarly nuanced "works in only some setups" behavior) do something to make sure projects not set up for them don't enable them. Maybe: documentation? Different config(s) and/or shared settings indicating project architecture style?

We did have a "Limitations" section on the no-invalid-properties docs page, but it looks like that was accidentally removed with the inclusion of ignoreUnknownVariables. I think it's reasonable to add that to the docs of rules that use variable tracking.

[JoshuaKGoldberg]

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:

• Provide config(s) like css/recommended-multi-file
• Provide a shared setting like css.variableTracking.multiFile
  • For any rules with an option like ignoreUnknownVariables, they could default that option to the shared setting's value

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.

[nzakas]

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.

[fasttime]

From what I understand, a new method getVariableValue(node) that returns a single Raw node will be good enough to support the functionality currently provided by no-invalid-properties, since that rule only considers the value of the last declaration. If the intent is to make the value calculation more accurate in the future, I think we should consider the possibility that a variable could assume different values regardless of the order of declarations, and in that case returning a single node may not be sufficient. Here is a simple example that illustrates the point:

:root .dark-theme {
    --backgound: #000;
}
:root :not(.dark-theme) {
    --backgound: #fff;
}
div {
    background-color: var(--backgound);
}

In the above code, the variable --background could take either of the values specified in the declarations, regardless of the order, because the values have different selectors. There is no single effective value for the variable in the context of the CSS file. For this use case it would be better if getVariableValue returned all applicable value nodes, rather than the last one.

[mdjermanovic]

I also think it should return all applicable value nodes, or maybe both all and the last before (e.g, { all: Raw[], lastBefore: Raw }, so that rule authors can decide whether the rule should consider all possible values or just one that might be the most likely.

[nzakas]

So then maybe two methods? getClosestVariableValue() and getVariableValues()?

[mdjermanovic]

So then maybe two methods? getClosestVariableValue() and getVariableValues()?

Sounds good to me.

[nzakas]

I realized we should also take into account @property definitions. I'll work on updating the RFC to address this next week. (Our fork of CSSTree can actually already validate values where all the variables have @property definitions, so that's a bit of an edge case that needs consideration.)

[nzakas]

Is there any reason this can't be moved to final commenting?

[fasttime]

Sorry for the delay, just trying to split my time across tasks. This is looking good already, I only left a couple of comments.

[mdjermanovic]

Looks good to me overall, just one suggestion.

[mdjermanovic]

Just a few fixes, then LGTM.

[mdjermanovic]

LGTM, thanks!

[fasttime]

LGTM, thanks. Moving to final commenting.

[fasttime]

I'm going to merge this now that the final commenting time is over.