Skip to content
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

Add custom-property-no-missing-var-function rule #5317

Merged
merged 2 commits into from May 29, 2021
Merged

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented May 19, 2021

Which issue, if any, is this issue related to?

Closes #5000
Closes #5044

Is there anything in the PR that needs further explanation?

This replaces #5044.

I've thought about this rule on and off for a while now. I thought I'd unpacked it all in #5000 (comment), but there's more.

From the <dashed-ident> spec:

Some contexts accept both author-defined identifiers and CSS-defined identifiers. If not handled carefully, this can result in difficulties adding new CSS-defined values; UAs have to study existing usage and gamble that there are sufficiently few author-defined identifiers in use matching the new CSS-defined one, so giving the new value a special CSS-defined meaning won’t break existing pages.

While there are many legacy cases in CSS that mix these two values spaces in exactly this fraught way, the <dashed-ident> type is meant to be an easy way to distinguish author-defined identifiers from CSS-defined identifiers.

I believe we'll see more unreferenced dashed-idents in the future. For example, if list-style-type property was designed today, I believe dashed-idents would be required for the <custom-idents> to avoid conflicts with CSS-defined identifiers like disc, e.g.:

@counter-style --disc {}
a { list-style-type: --disc }

The only way to confidently know if a dashed-ident is a custom property, and should therefore be referenced with the var function, is to check if it has been defined as one.

I've updated the rule to do this and renamed it accordingly. Similarly to the no-unknown-animations rule, it has the limitation of only catching unreferenced custom properties that are defined within the same source. Unlike the no-unknown-animations rule, these manifest as false negatives rather than false positives. As such, I think we're OK to add the rule with this limitation as it'll be useful to some users, i.e. those using locally scoped variables or defining their variables at the top of the same source. We can remove the limitation when we find a way to provide context for rules like these.

At the moment, it'll catch:

/* same source at-properties */
@property --foo {
  syntax: "<color>";
  initial-value: red;
}
a { color: --foo; }
/* same source custom properties */
:root { --foo: red; }
a { color: --foo; }
/* locally scoped custom properties */
a { 
  --foo: red;
  color: --foo;
}

While avoiding any false positives for unreferenced dashed-idents. Which I think is good enough.

@jeddy3 jeddy3 changed the title Add custom-property-no-missing-var-function Add custom-property-no-missing-var-function rule May 19, 2021
@alexander-akait
Copy link
Member

The name is sounds better ⭐

Copy link
Member

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed explanation. I understand well.
I've left a trivial question, but this rule addition looks like no problem to me. 👍🏼

lib/rules/custom-property-no-missing-var-function/index.js Outdated Show resolved Hide resolved
Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com>
@jeddy3 jeddy3 merged commit a3cc2c4 into v14 May 29, 2021
@jeddy3 jeddy3 deleted the add-no-missing-var branch May 29, 2021 08:25
@jeddy3
Copy link
Member Author

jeddy3 commented May 29, 2021

  • Added: custom-property-no-missing-var-function rule (#5317).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants