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 declaration-property-value-no-unknown #6376

Closed
caneta opened this issue Sep 29, 2022 · 5 comments · Fixed by #6511
Closed

Add declaration-property-value-no-unknown #6376

caneta opened this issue Sep 29, 2022 · 5 comments · Fixed by #6511
Labels
status: wip is being worked on by someone type: new rule an entirely new rule

Comments

@caneta
Copy link

caneta commented Sep 29, 2022

What steps are needed to reproduce the bug?

body {
  padding: auto;
}

What Stylelint configuration is needed to reproduce the bug?

{
  "extends": [
    "stylelint-config-standard",
    "stylelint-config-prettier"
  ],
  "rules": {
    "block-closing-brace-newline-after": [
      "always", {
        "ignoreAtRules": [ "if", "else" ]
      }
    ],
    "at-rule-no-unknown": [
      true,
      {
        "ignoreAtRules": [
          "mixin",
          "if",
          "else",
          "error",
          "include",
          "at-root",
          "extend"
        ]
      }
    ]
  },
  "property-no-unknown": true,
  "defaultSeverity": "warning"
}

How did you run Stylelint?

From VSCode

Which version of Stylelint are you using?

14.13.0

What did you expect to happen?

The editor should tell me that 'auto' is not a permitted value for property 'padding'.

What actually happened?

Nothing, the error is not reported.

Does the bug relate to non-standard syntax?

Nope, tryed in a plain CSS

Proposal to fix the bug

No response

@ybiquitous
Copy link
Member

@caneta Thanks for using the template.

Unfortunately, Stylelint doesn't support the validation of property-and-value pairs. So I recommend using the stylelint-csstree-validator plugin. For example:

$ cat package.json
{
  "devDependencies": {
    "stylelint": "^14.13.0",
    "stylelint-csstree-validator": "^2.0.0"
  },
  "stylelint": {
    "plugins": [
      "stylelint-csstree-validator"
    ],
    "rules": {
      "csstree/validator": true
    }
  }
}

$ cat test.css
body {
  padding: auto;
}

$ npx stylelint *.css

test.css
 2:12  ✖  Invalid value for "padding"  csstree/validator

1 problem (1 error, 0 warnings)

See also our document about validators.

@jeddy3
Copy link
Member

jeddy3 commented Sep 30, 2022

This question pops up often, and it feels like an appropriate time to revisit it.

As mentioned in our doc that @ybiquitous linked to, there are three types of tools:

  • linters
  • validators
  • formatters (aka pretty printers)

Stylelint has historically overlapped with both validators and formatters. We intend to remove our formatting rules so that we don't overlap with formatters. We need a clear vision for how we approach validation too.

We split our rules into two groups that:

The latter group is made up of 5 subgroups that:

These all apply to valid CSS. They are powerful rules, mostly unique to Stylelint and aligned with our goal of being a linter.

The group that avoids errors is made up of two subgroups that check for:

The former group overlaps with validators whereas the latter does not. Like the rules that enforce (non-stylistic) conventions, the ones in the latter subgroup are mostly unique to Stylelint.

Additionally, we catch and surfaces broad syntax errors, e.g. unclosed blocks.

The complete list of our validating rules is:

Of which, the following overlap with CSSTree:

Now that we have the lay of the land, let's decide on a clear vision for validation.

I think we have three options:

  • keep our current approach of adding validation for simple cases and suggesting people use stylelint-csstree-validator
  • remove our validation rules, i.e. the *-no-unknown and *-no-invalid ones
  • add built-in rules to validate pairings like property/value and at-rule/params ones

For the first two, we'd need to improve our documentation to better explain linters and validators, and how Stylelint can be used with a validator like CSSTree.

There are pros and cons to all three approaches, for example:

  • our current approach muddies the water, i.e. we offer some validation but not for pairings like property/value ones and this confuses users
  • removing our validation rules would be clear-cut for our uses, but current validators don't catch things like invalid named grid areas
  • to add built-in rules we may need to bundle a 2nd parser, e.g. CSSTree, although we've spoken about adopting specific parsers for tricky rules in Use SWC as parser #5586

Based on how often people open property/value pair validation issues in our tracker, I feel users are expecting this feature. Especially as some CSS-in-JS libraries like vanilla-extract now support this (via the CSSType library).

What do people think?

It's a big topic and one that may refine the direction of Stylelint. So let's leave this issue open as a discussion for a while so that we can stew on it.

@jeddy3 jeddy3 changed the title Check for a bad property value Add rules to validate property/value and at-rule/param pairs Sep 30, 2022
@jeddy3 jeddy3 added the status: needs discussion triage needs further discussion label Sep 30, 2022
@caneta
Copy link
Author

caneta commented Sep 30, 2022

@caneta Thanks for using the template.

Unfortunately, Stylelint doesn't support the validation of property-and-value pairs. So I recommend using the stylelint-csstree-validator plugin. For example:

$ cat package.json
{
  "devDependencies": {
    "stylelint": "^14.13.0",
    "stylelint-csstree-validator": "^2.0.0"
  },
  "stylelint": {
    "plugins": [
      "stylelint-csstree-validator"
    ],
    "rules": {
      "csstree/validator": true
    }
  }
}

$ cat test.css
body {
  padding: auto;
}

$ npx stylelint *.css

test.css
 2:12  ✖  Invalid value for "padding"  csstree/validator

1 problem (1 error, 0 warnings)

See also our document about validators.

Thank you, I have used CSSTree indeed and everything worked as expected.
@jeddy3 is right, I was expecting that writing something like padding: inventedValue was notified by the linter, so it could be useful to go ahead with this topic.

@ybiquitous
Copy link
Member

@jeddy3 Thanks for the understandable classification of the built-in rules. It might be wonderful if we could reflect this grouping on our documentation somehow.

In addition, thanks for providing several future policies for validation rules.

I believe many people would like to receive the benefits of validation rules out of the box. So, now I'm inclined to the third option:

add built-in rules to validate pairings like property/value and at-rule/params ones

although there may be some problems like a larger bundle size or higher maintenance costs.

@jeddy3 jeddy3 changed the title Add rules to validate property/value and at-rule/param pairs Add declaration-property-value-no-unknown Oct 15, 2022
@jeddy3 jeddy3 added status: ready to implement is ready to be worked on by someone type: new rule an entirely new rule and removed status: needs discussion triage needs further discussion labels Oct 15, 2022
@jeddy3
Copy link
Member

jeddy3 commented Oct 15, 2022

I believe many people would like to receive the benefits of validation rules out of the box. So, now I'm inclined to the third option.

SGTM.

although there may be some problems like a larger bundle size or higher maintenance costs.

I agree. It feels like the pros outweigh the cons, though. Unless anyone knows of an alternative, we can use the CSSTree within the rule for now. We can refactor the implementation if a more appropriate library comes along (or if we end up rolling our own).

I believe the CSSTree parser returns everything we need, including what we need for accurate positions (i.e. mismatchOffset and mismatchLength):

import { lexer } from "css-tree";
lexer.matchProperty("padding", "10px auto");
{
  matched: null,
  iterations: 24,
  error: SyntaxError [SyntaxMatchError]: Mismatch
      at matchSyntax (file:///Users/jeddy3/Projects/temp/node_modules/css-tree/lib/lexer/Lexer.js:82:17)
      at Lexer.matchProperty (file:///Users/jeddy3/Projects/temp/node_modules/css-tree/lib/lexer/Lexer.js:324:16)
      at file:///Users/jeddy3/Projects/temp/csstree.mjs:7:21
      at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
      at async Promise.all (index 0)
      at async ESMLoader.import (node:internal/modules/esm/loader:533:24)
      at async loadESM (node:internal/process/esm_loader:91:5)
      at async handleMainPromise (node:internal/modules/run_main:65:12) {
    message: 'Mismatch\n' +
      '  syntax: [ <length> | <percentage> ]{1,4}\n' +
      '   value: 10px auto\n' +
      '  -------------^',
    rawMessage: 'Mismatch',
    syntax: '[ <length> | <percentage> ]{1,4}',
    css: '10px auto',
    mismatchOffset: 5,
    mismatchLength: 4,
    offset: 5,
    line: 1,
    column: 6,
    loc: { source: '<unknown>', start: [Object], end: [Object] }
  },
  getTrace: [Function: getTrace],
  isKeyword: [Function: isKeyword],
  isProperty: [Function: isProperty],
  isType: [Function: isType]
}

We'll want to limit the scope of the rule compared to the plugin, e.g. the rule should only check standard CSS for unknown values within properties. We should silently catch all other errors the parser throws, including the error for unknown properties so that the rule doesn't overlap with our existing property-no-unknown one.

We may also want to limit our unit-no-unknown and function-no-unknown rules to at-rules params so that they won't overlap with this new rule.

  • Name: declaration-property-value-no-unknown
  • Primary option: true
  • Secondary options:
    • ignoreProperties: []
    • ignoreValues: []
    • propertiesSyntax: {}
    • typesSyntax: {}
  • Autofixable: No
  • Message: Unexpected unknown value "${value}" for property "${property}"
  • Description: "Disallow unknown values for property within declarations"
  • Extended description: "This rule considers values for properties defined within the CSS specifications to be known. You can use the propertiesSyntax and typesSyntax secondary options to alter the syntax."
  • Section: "Avoid errors" -> "Unknown"

I've labelled the issue as ready to implement. Please consider contributing if anyone fancies giving this ago.

There are steps on how to add a new rule in the Developer guide.

@jeddy3 jeddy3 changed the title Add declaration-property-value-no-unknown Add declaration-property-value-no-unknown Dec 1, 2022
@jeddy3 jeddy3 added status: wip is being worked on by someone and removed status: ready to implement is ready to be worked on by someone labels Dec 6, 2022
@ybiquitous ybiquitous linked a pull request Feb 2, 2023 that will close this issue
@jeddy3 jeddy3 closed this as completed Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: wip is being worked on by someone type: new rule an entirely new rule
Development

Successfully merging a pull request may close this issue.

3 participants