From 2ba85880a4785408ae67ae6b8484d51d74eba8a3 Mon Sep 17 00:00:00 2001 From: Romain Menke Date: Thu, 13 Jul 2023 17:56:50 +0200 Subject: [PATCH 1/5] Fix `declaration-property-value-no-unknown` false negatives for typed custom properties --- .../__tests__/index.mjs | 99 +++++++++++++++++++ .../index.js | 62 +++++++++++- 2 files changed, 156 insertions(+), 5 deletions(-) diff --git a/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs b/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs index 83dd0d19ea..71d6c8efab 100644 --- a/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs +++ b/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs @@ -55,6 +55,75 @@ testRule({ `, description: 'at-rule descriptor and property with the same name but different syntaxes', }, + { + code: ` + a { --foo: red; } + + @property --foo { + syntax: ""; + inherits: false; + initial-value: #c0ffee; + } + `, + }, + { + code: ` + a { --foo: 10px; } + a { --foo: 10%; } + + @property --foo { + syntax: " | "; + inherits: false; + initial-value: 10px; + } + `, + }, + { + code: ` + a { --foo: bigger; } + a { --foo: BIGGER; } + + @property --foo { + syntax: "big | bigger | BIGGER"; + inherits: false; + initial-value: big; + } + `, + }, + { + code: ` + a { --foo: 10px; } + a { --foo: 10px 10vh; } + + @property --foo { + syntax: "+"; + inherits: false; + initial-value: 10px; + } + `, + }, + { + code: ` + a { --foo: red; } + a { --foo: 10px 10vh; } + + @property --foo { + syntax: "*"; + inherits: false; + initial-value: 10px; + } + `, + }, + { + code: ` + a { --foo: var(--bar); } + @property --foo { + syntax: ""; + inherits: false; + initial-value: #c0ffee; + } + `, + }, ], reject: [ @@ -138,6 +207,36 @@ testRule({ endLine: 1, endColumn: 30, }, + { + code: ` + a { --foo: 10px; } + @property --foo { + syntax: ""; + inherits: false; + initial-value: #c0ffee; + } + `, + message: messages.rejected('--foo', '10px'), + line: 2, + column: 16, + endLine: 2, + endColumn: 20, + }, + { + code: ` + a { --foo: 10px; } + @property --foo { + syntax: not-a-string; + inherits: false; + initial-value: #c0ffee; + } + `, + message: messages.rejected('syntax', 'not-a-string'), + line: 4, + column: 14, + endLine: 4, + endColumn: 26, + }, ], }); diff --git a/lib/rules/declaration-property-value-no-unknown/index.js b/lib/rules/declaration-property-value-no-unknown/index.js index 46153e6725..e02ac7a5ba 100644 --- a/lib/rules/declaration-property-value-no-unknown/index.js +++ b/lib/rules/declaration-property-value-no-unknown/index.js @@ -1,7 +1,7 @@ 'use strict'; const { isPlainObject } = require('is-plain-object'); -const { fork, parse, find } = require('css-tree'); +const { fork, parse, find, string } = require('css-tree'); const declarationValueIndex = require('../../utils/declarationValueIndex'); const matchesStringOrRegExp = require('../../utils/matchesStringOrRegExp'); @@ -13,7 +13,7 @@ const isCustomProperty = require('../../utils/isCustomProperty'); const isStandardSyntaxValue = require('../../utils/isStandardSyntaxValue'); const isStandardSyntaxProperty = require('../../utils/isStandardSyntaxProperty'); const isStandardSyntaxDeclaration = require('../../utils/isStandardSyntaxDeclaration'); -const { isAtRule } = require('../../utils/typeGuards'); +const { isAtRule, isDeclaration } = require('../../utils/typeGuards'); const { isRegExp, isString } = require('../../utils/validateTypes'); const ruleName = 'declaration-property-value-no-unknown'; @@ -28,6 +28,8 @@ const meta = { url: 'https://stylelint.io/user-guide/rules/declaration-property-value-no-unknown', }; +const SYNTAX_PROPERTY = /^syntax$/i; + /** @type {import('stylelint').Rule} */ const rule = (primary, secondaryOptions) => { return (root, result) => { @@ -62,9 +64,47 @@ const rule = (primary, secondaryOptions) => { return valuePattern && matchesStringOrRegExp(value, valuePattern); }; - const propertiesSyntax = (secondaryOptions && secondaryOptions.propertiesSyntax) || {}; + const propertiesSyntax = { + // Take a shallow clone as this object will be appended to. + ...((secondaryOptions && secondaryOptions.propertiesSyntax) || {}), + }; const typesSyntax = (secondaryOptions && secondaryOptions.typesSyntax) || {}; + const typedCustomPropertyNames = new Map(); + + root.walkAtRules(/^property$/i, (atRule) => { + const propName = atRule.params.trim(); + + if (!propName || !atRule.nodes || !isCustomProperty(propName)) return; + + for (const node of atRule.nodes) { + if (isDeclaration(node) && SYNTAX_PROPERTY.test(node.prop)) { + const value = node.value.trim(); + const unquoted = string.decode(value); + + // Only string values are valid. + // We can not check the syntax of this property. + if (unquoted === value) continue; + + // Any value is allowed in this custom property. + // We don't need to check this property. + if (unquoted === '*') continue; + + // https://github.com/csstree/csstree/pull/256 + // We can circumvent this issue by prefixing the property name, + // making it a vendor-prefixed property instead of a custom property. + // No one should be using `-stylelint--` as a property prefix. + // + // When this is resolved `typedCustomPropertyNames` can become a `Set` + // and the prefix can be removed. + const prefixedPropName = `-stylelint${propName}`; + + typedCustomPropertyNames.set(propName, prefixedPropName); + propertiesSyntax[prefixedPropName] = unquoted; + } + } + }); + const forkedLexer = fork({ properties: propertiesSyntax, types: typesSyntax, @@ -83,10 +123,22 @@ const rule = (primary, secondaryOptions) => { if (!isStandardSyntaxValue(value)) return; - if (isCustomProperty(prop)) return; + if (isCustomProperty(prop) && !typedCustomPropertyNames.has(prop)) return; if (isPropIgnored(prop, value)) return; + // https://github.com/mdn/data/pull/674 + // `initial-value` has a incorrect syntax definition. + // In reality everything is valid. + if ( + /^initial-value$/i.test(prop) && + decl.parent && + isAtRule(decl.parent) && + /^property$/i.test(decl.parent.name) + ) { + return; + } + /** @type {import('css-tree').CssNode} */ let cssTreeValueNode; @@ -113,7 +165,7 @@ const rule = (primary, secondaryOptions) => { const { error } = parent && isAtRule(parent) ? forkedLexer.matchAtruleDescriptor(parent.name, prop, cssTreeValueNode) - : forkedLexer.matchProperty(prop, cssTreeValueNode); + : forkedLexer.matchProperty(typedCustomPropertyNames.get(prop) ?? prop, cssTreeValueNode); if (!error) return; From 98520f8510c9b3f86d9ca03bb54eb13f158dd86c Mon Sep 17 00:00:00 2001 From: Romain Menke <11521496+romainmenke@users.noreply.github.com> Date: Thu, 13 Jul 2023 18:06:39 +0200 Subject: [PATCH 2/5] Create nice-toes-reflect.md --- .changeset/nice-toes-reflect.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/nice-toes-reflect.md diff --git a/.changeset/nice-toes-reflect.md b/.changeset/nice-toes-reflect.md new file mode 100644 index 0000000000..153aa6916c --- /dev/null +++ b/.changeset/nice-toes-reflect.md @@ -0,0 +1,5 @@ +--- +"stylelint": patch +--- + +Fixed: `declaration-property-value-no-unknown` false negatives for typed custom properties From 7677c966b124deb5e38b684cf4455f469268617a Mon Sep 17 00:00:00 2001 From: Romain Menke Date: Fri, 14 Jul 2023 10:52:05 +0200 Subject: [PATCH 3/5] apply suggestions from code review Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com> --- .../__tests__/index.mjs | 36 ++++++++++--------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs b/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs index 71d6c8efab..dd20e944b6 100644 --- a/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs +++ b/lib/rules/declaration-property-value-no-unknown/__tests__/index.mjs @@ -1,3 +1,5 @@ +import { stripIndent } from 'common-tags'; + import rule from '../index.js'; const { messages, ruleName } = rule; @@ -49,14 +51,14 @@ testRule({ code: 'a { font-size: max(1rem, 3rem); }', }, { - code: ` + code: stripIndent` a { font-weight: bolder; } @font-face { font-weight: 100 200; } `, description: 'at-rule descriptor and property with the same name but different syntaxes', }, { - code: ` + code: stripIndent` a { --foo: red; } @property --foo { @@ -67,7 +69,7 @@ testRule({ `, }, { - code: ` + code: stripIndent` a { --foo: 10px; } a { --foo: 10%; } @@ -79,7 +81,7 @@ testRule({ `, }, { - code: ` + code: stripIndent` a { --foo: bigger; } a { --foo: BIGGER; } @@ -91,7 +93,7 @@ testRule({ `, }, { - code: ` + code: stripIndent` a { --foo: 10px; } a { --foo: 10px 10vh; } @@ -103,7 +105,7 @@ testRule({ `, }, { - code: ` + code: stripIndent` a { --foo: red; } a { --foo: 10px 10vh; } @@ -115,7 +117,7 @@ testRule({ `, }, { - code: ` + code: stripIndent` a { --foo: var(--bar); } @property --foo { syntax: ""; @@ -208,7 +210,7 @@ testRule({ endColumn: 30, }, { - code: ` + code: stripIndent` a { --foo: 10px; } @property --foo { syntax: ""; @@ -217,13 +219,13 @@ testRule({ } `, message: messages.rejected('--foo', '10px'), - line: 2, - column: 16, - endLine: 2, - endColumn: 20, + line: 1, + column: 12, + endLine: 1, + endColumn: 16, }, { - code: ` + code: stripIndent` a { --foo: 10px; } @property --foo { syntax: not-a-string; @@ -232,10 +234,10 @@ testRule({ } `, message: messages.rejected('syntax', 'not-a-string'), - line: 4, - column: 14, - endLine: 4, - endColumn: 26, + line: 3, + column: 10, + endLine: 3, + endColumn: 22, }, ], }); From 4bb12bce1845c0c4ac6875c76f818855e2ea1291 Mon Sep 17 00:00:00 2001 From: Romain Menke <11521496+romainmenke@users.noreply.github.com> Date: Thu, 20 Jul 2023 08:11:12 +0200 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: Masafumi Koba <473530+ybiquitous@users.noreply.github.com> --- lib/rules/declaration-property-value-no-unknown/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/rules/declaration-property-value-no-unknown/index.js b/lib/rules/declaration-property-value-no-unknown/index.js index 06ad393299..3595743db9 100644 --- a/lib/rules/declaration-property-value-no-unknown/index.js +++ b/lib/rules/declaration-property-value-no-unknown/index.js @@ -67,10 +67,11 @@ const rule = (primary, secondaryOptions) => { const propertiesSyntax = { // Take a shallow clone as this object will be appended to. - ...((secondaryOptions && secondaryOptions.propertiesSyntax) || {}), + ...(secondaryOptions?.propertiesSyntax ?? {}), }; - const typesSyntax = (secondaryOptions && secondaryOptions.typesSyntax) || {}; + const typesSyntax = secondaryOptions?.typesSyntax ?? {}; + /** @type {Map} */ const typedCustomPropertyNames = new Map(); root.walkAtRules(/^property$/i, (atRule) => { From 7185f35339bacf6a9caeec154b5f8957c3e67c7f Mon Sep 17 00:00:00 2001 From: Romain Menke Date: Thu, 20 Jul 2023 08:15:42 +0200 Subject: [PATCH 5/5] apply suggestions from code review --- lib/rules/declaration-property-value-no-unknown/index.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/lib/rules/declaration-property-value-no-unknown/index.js b/lib/rules/declaration-property-value-no-unknown/index.js index 3595743db9..1856d82eb0 100644 --- a/lib/rules/declaration-property-value-no-unknown/index.js +++ b/lib/rules/declaration-property-value-no-unknown/index.js @@ -53,9 +53,7 @@ const rule = (primary, secondaryOptions) => { return; } - const ignoreProperties = Array.from( - Object.entries((secondaryOptions && secondaryOptions.ignoreProperties) || {}), - ); + const ignoreProperties = Array.from(Object.entries(secondaryOptions?.ignoreProperties ?? {})); /** @type {(name: string, propValue: string) => boolean} */ const isPropIgnored = (name, value) => { @@ -130,7 +128,7 @@ const rule = (primary, secondaryOptions) => { if (isPropIgnored(prop, value)) return; // https://github.com/mdn/data/pull/674 - // `initial-value` has a incorrect syntax definition. + // `initial-value` has an incorrect syntax definition. // In reality everything is valid. if ( /^initial-value$/i.test(prop) &&