Skip to content

feat: Validate property values containing variables #148

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented May 22, 2025

Prerequisites checklist

What is the purpose of this pull request?

To enable validation of CSS property values containing var(). Previously, we just skipped validating these completely. For this to work, the custom property must be defined before it's used, such as:

:root {
  --color: red
}

a {
    color: var(--color);
}

If a custom property is not defined before it's used, a lint violation is reported.

What changes did you make? (Give an overview)

  • Updated no-invalid-properties to track custom property values and use them as replacements for the value that is validated
  • Added tests for these changes
  • Updated the docs to indicate how var() is validated

Related Issues

Is there anything you'd like reviewers to focus on?

The highlighted section of code for a violation is different when there is a var() vs. when there isn't.

  • When there isn't a var() (as in HEAD), the individual incorrect value is highlighted. So for border-top: 1px solid foo, it's foo that is indicated in the location information.
  • When there is a var():
    • If the var() is the violation, then it's location is highlighted and the replacement value is reported in the message.
    • If any other part of the value is the violation, then the entire value is highlighted. This is because I couldn't figure out an efficient way to track the calculated location after variable replacement. The message still contains the exact part of the value that is the problem.

@github-project-automation github-project-automation bot moved this to Needs Triage in Triage May 22, 2025
@nzakas nzakas requested a review from Copilot May 22, 2025 21:02
@nzakas nzakas changed the title feat: Validation property values containing variables feat: Validate property values containing variables May 22, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Enables validation of CSS property values that use var() by tracking custom property definitions and replacing variables before validation.

  • Tracks var() function calls per declaration and substitutes known custom property values for validation.
  • Reports errors for unknown variables (unknownVar) and invalid substituted values (invalidPropertyValue).
  • Adds tests and updates documentation to describe variable validation behavior.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/rules/no-invalid-properties.test.js Added valid and invalid test cases covering custom property usage
src/rules/no-invalid-properties.js Implemented var() tracking, replacement logic, and new error type
docs/rules/no-invalid-properties.md Updated limitations section to explain var() validation behavior
Comments suppressed due to low confidence (2)

src/rules/no-invalid-properties.js:17

  • JSDoc tags use '@import' which is non-standard; consider using '@typedef {import("../types.js").CSSRuleDefinition}' or the equivalent JSDoc import syntax for proper type declarations.
* @import { CSSRuleDefinition } from "../types.js"

src/rules/no-invalid-properties.js:143

  • [nitpick] Using var as a data property key can be confusing since it's a reserved term; consider renaming it to varName or customProperty for clarity.
data: { var: name },

@nzakas nzakas marked this pull request as draft May 23, 2025 18:32
@nzakas nzakas requested a review from Copilot May 23, 2025 18:41
@nzakas nzakas marked this pull request as ready for review May 23, 2025 18:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request enables validation of CSS property values that include variable references through var(), ensuring that defined custom properties are used correctly.

  • Introduces variable tracking and replacement in the no-invalid-properties rule
  • Adds comprehensive tests covering various cases with var() usages
  • Updates documentation to clarify the behavior when custom properties are defined or missing

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/rules/no-invalid-properties.test.js Adds test cases for validating both defined and undefined var() usage
src/rules/no-invalid-properties.js Implements custom property tracking and replacement logic for var() in declarations
docs/rules/no-invalid-properties.md Updates documentation to explain how var() validation is handled
Comments suppressed due to low confidence (1)

src/rules/no-invalid-properties.js:143

  • Instead of returning immediately after encountering an unknown variable, consider reporting all unknown variable errors in the block to provide comprehensive feedback.
context.report({ loc: func.children[0].loc, messageId: "unknownVar", data: { var: name }, });

@lumirlumir lumirlumir added the accepted There is consensus among the team that this change meets the criteria for inclusion label May 26, 2025
@snitin315
Copy link

We have one false positive when we have spaces inside var():

/* eslint css/no-invalid-properties: error */

:root {
    --color: red
}

a {
    color: var(  --color ); // Unknown property 'color' found  css/no-invalid-properties
}

@nzakas
Copy link
Member Author

nzakas commented May 28, 2025

@snitin315 nice find! I'll take a look.

@nzakas nzakas moved this from Needs Triage to Implementing in Triage May 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion feature
Projects
Status: Implementing
Development

Successfully merging this pull request may close these issues.

3 participants