-
-
Notifications
You must be signed in to change notification settings - Fork 32
feat: no-invalid-properties allowUnknownVariables option #178
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
Conversation
Adds an option to `no-invalid-properties` called `allowUnknownVariables`. When true, variable references that can't be traced to a custom property definition are ignored. Fixes eslint#175
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.
Pull Request Overview
Adds an allowUnknownVariables option to the no-invalid-properties rule, letting users ignore variable references that can’t be traced to custom properties and adjust which errors are reported accordingly.
- Updated rule schema and logic to default
allowUnknownVariablestofalse, skipunknownVarreports when enabled, and only reportunknownPropertyon reference errors. - Introduced a new helper
isSyntaxReferenceErrorinutil.jsand modified imports. - Expanded tests and documentation to cover the new option.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/rules/no-invalid-properties.test.js | Added valid and invalid tests exercising allowUnknownVariables |
| src/util.js | Added isSyntaxReferenceError helper and updated import doc |
| src/rules/no-invalid-properties.js | Introduced schema option, wired up allowUnknownVariables in reporting logic |
| docs/rules/no-invalid-properties.md | Documented the new allowUnknownVariables option and examples |
Comments suppressed due to low confidence (2)
src/util.js:30
- [nitpick] The JSDoc @returns type uses a TypeScript type predicate. For clarity and consistency with JSDoc conventions, consider changing this to
@returns {boolean}and describing the predicate in the description.
* @returns {error is SyntaxReferenceError} True if the error is a syntax reference error, false if not.
docs/rules/no-invalid-properties.md:47
- [nitpick] After replacing the old
Limitationssection, you may want to reintroduce it (or rename it) to explain any remaining parser limitations, or ensure that the table of contents and heading hierarchy are updated for clarity.
## Options
src/rules/no-invalid-properties.js
Outdated
| properties: { | ||
| allowUnknownVariables: { | ||
| type: "boolean", | ||
| default: false, |
Copilot
AI
Jun 20, 2025
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.
ESLint does not apply default values from the schema at runtime. Since you already fallback in code (?? false), you can remove this default property to avoid confusion.
| default: false, |
| url: "https://github.com/eslint/css/blob/main/docs/rules/no-invalid-properties.md", | ||
| }, | ||
|
|
||
| schema: [ |
Copilot
AI
Jun 20, 2025
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.
[nitpick] Consider adding additionalProperties: false inside the schema object to prevent unknown or misspelled options from silently being accepted.
nzakas
left a comment
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.
This is looking really good. Just one note to clean things up a bit.
src/rules/no-invalid-properties.js
Outdated
| const replacements = []; | ||
|
|
||
| const allowUnknownVariables = | ||
| context.options?.[0]?.allowUnknownVariables ?? false; |
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.
If you use defaultOptions (example), then you can do this:
| context.options?.[0]?.allowUnknownVariables ?? false; | |
| context.options.[0].allowUnknownVariables; |
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.
Ah, thank you. Updated, but used destructuring syntax to match the style in relative-font-units
snitin315
left a comment
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.
Thanks for the PR. I found one issue while defining fallbacks in var
/* eslint css/no-invalid-properties: ["error"] */
:root {
--my-color: red;
}
a {
color: var(--my-color, red); // This now reports error - Unknown property 'color' found css/no-invalid-propertie
}|
Another case: /* eslint css/no-invalid-properties: ["error", { allowUnknownVariables: false }] */
:root {
--my-color: red;
}
.two {
color: var(--my-color, var(--my-background, pink)); // error Unknown property 'color' found css/no-invalid-properties
} |
Good catch, but I think this issue exists in main. Maybe it should be treated as a separate issue? Edit: Both of the cases you point out are unrelated to my changes and are both issues that exist in main. |
I can also reproduce this in |
I see. Yes, let's tackle it separately. |
Co-authored-by: Tanuj Kanti <86398394+Tanujkanti4441@users.noreply.github.com>
nzakas
left a comment
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.
LGTM, thanks! Would like @snitin315 and @Tanujkanti4441 to verify their feedback has been addressed before merging.
|
@jgoz please double-check the lint error. |
|
@nzakas That was a formatting error in a file I didn't change, so I'm not sure what happened there. I merged the latest from main in case it was an artifact of GHA's pull_request behavior that operates on a synthetic merge commit. |
|
Hi everyone, The CI failure is due to the newly released Prettier v3.6.0. I’ve opened #182 to fix the issue. Once it’s merged into main, the error should be resolved. |
|
#182 has now been merged, so the CI error should be resolved once we merge the main branch. 👍 |
|
Thanks @lumirlumir 🙏 |
|
Merged from main |
snitin315
left a comment
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.
LGTM, thanks!
Prerequisites checklist
What is the purpose of this pull request?
Adds an option to
no-invalid-propertiescalledallowUnknownVariables. When true, variable references that can't be traced to a custom property definition are ignored.What changes did you make? (Give an overview)
Added an options schema for
no-invalid-propertiesdescribingallowUnknownVariables. When the option istrue, do the following:unknownVar)unknownPropertyfor errors of typeSyntaxReferenceError, which indicate that the property is unknownAlso updated documentation for the rule to reflect this option as best I could.
Related Issues
Fixes #175
Is there anything you'd like reviewers to focus on?