From 3eaeec18e331b5eebc2ab34c3cb2cffb0528c6de Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 18 Nov 2024 13:22:42 +0100 Subject: [PATCH 01/23] start initial work for design token rules --- package.json | 1 + .../README.mdx | 0 .../kbn-eslint-plugin-design-tokens/index.ts | 19 +++++++ .../jest.config.js | 14 +++++ .../kibana.jsonc | 6 +++ .../package.json | 9 ++++ .../rules/no_css_color.test.tsx | 53 +++++++++++++++++++ .../rules/no_css_color.ts | 43 +++++++++++++++ tsconfig.base.json | 2 + yarn.lock | 4 ++ 10 files changed, 151 insertions(+) create mode 100644 packages/kbn-eslint-plugin-design-tokens/README.mdx create mode 100644 packages/kbn-eslint-plugin-design-tokens/index.ts create mode 100644 packages/kbn-eslint-plugin-design-tokens/jest.config.js create mode 100644 packages/kbn-eslint-plugin-design-tokens/kibana.jsonc create mode 100644 packages/kbn-eslint-plugin-design-tokens/package.json create mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx create mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts diff --git a/package.json b/package.json index 36727d19bc200..c631a1444c821 100644 --- a/package.json +++ b/package.json @@ -1450,6 +1450,7 @@ "@kbn/es": "link:packages/kbn-es", "@kbn/es-archiver": "link:packages/kbn-es-archiver", "@kbn/eslint-config": "link:packages/kbn-eslint-config", + "@kbn/eslint-plugin-design-tokens": "link:packages/kbn-eslint-plugin-design-tokens", "@kbn/eslint-plugin-disable": "link:packages/kbn-eslint-plugin-disable", "@kbn/eslint-plugin-eslint": "link:packages/kbn-eslint-plugin-eslint", "@kbn/eslint-plugin-i18n": "link:packages/kbn-eslint-plugin-i18n", diff --git a/packages/kbn-eslint-plugin-design-tokens/README.mdx b/packages/kbn-eslint-plugin-design-tokens/README.mdx new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/packages/kbn-eslint-plugin-design-tokens/index.ts b/packages/kbn-eslint-plugin-design-tokens/index.ts new file mode 100644 index 0000000000000..b69989cf93538 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/index.ts @@ -0,0 +1,19 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { noCssColor } from './rules/no_css_color'; + +/** + * Custom ESLint rules, add `'@kbn/eslint-plugin-design-tokens'` to your eslint config to use them + * @internal + */ + +const rules = { + 'no-css-color': noCssColor, +}; diff --git a/packages/kbn-eslint-plugin-design-tokens/jest.config.js b/packages/kbn-eslint-plugin-design-tokens/jest.config.js new file mode 100644 index 0000000000000..41b5394f803b6 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/jest.config.js @@ -0,0 +1,14 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +module.exports = { + preset: '@kbn/test', + rootDir: '../..', + roots: ['/packages/kbn-eslint-plugin-design-tokens'], +}; diff --git a/packages/kbn-eslint-plugin-design-tokens/kibana.jsonc b/packages/kbn-eslint-plugin-design-tokens/kibana.jsonc new file mode 100644 index 0000000000000..2602cf919bd68 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/kibana.jsonc @@ -0,0 +1,6 @@ +{ + "type": "shared-common", + "id": "@kbn/eslint-plugin-design-tokens", + "devOnly": true, + "owner": "@elastic/shared-ux" +} diff --git a/packages/kbn-eslint-plugin-design-tokens/package.json b/packages/kbn-eslint-plugin-design-tokens/package.json new file mode 100644 index 0000000000000..789bc5698d59b --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/package.json @@ -0,0 +1,9 @@ +{ + "name": "@kbn/eslint-plugin-design-tokens", + "version": "1.0.0", + "private": true, + "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0", + "peerDependencies": { + "eslint": "^7.0.0" + } +} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx new file mode 100644 index 0000000000000..570dce461f803 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx @@ -0,0 +1,53 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { RuleTester } from 'eslint'; +import { noCssColor } from './no_css_color'; + +const tsTester = [ + '@typescript-eslint/parser', + new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + ecmaFeatures: { + jsx: true, + }, + }, + }), +] as const; + +const babelTester = [ + '@babel/eslint-parser', + new RuleTester({ + parser: require.resolve('@babel/eslint-parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + requireConfigFile: false, + babelOptions: { + presets: ['@kbn/babel-preset/node_preset'], + }, + }, + }), +] as const; + +const invalid: RuleTester.InvalidTestCase[] = []; + +const valid: RuleTester.ValidTestCase[] = []; + +for (const [name, tester] of [tsTester, babelTester]) { + describe(name, () => { + tester.run('@kbn/no_css_color', noCssColor, { + valid, + invalid, + }); + }); +} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts new file mode 100644 index 0000000000000..0fe9d31b47d77 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts @@ -0,0 +1,43 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { Rule } from 'eslint'; +import type { TSESTree } from '@typescript-eslint/typescript-estree'; + +export const noCssColor: Rule.RuleModule = { + meta: { + type: 'problem', + docs: { + description: 'Use color definitions from eui theme as opposed to CSS color values', + category: 'Best Practices', + recommended: true, + url: 'https://eui.elastic.co/#/theming/colors/values', + }, + schema: [], + }, + create(context) { + return { + JSXAttribute(node: TSESTree.JSXAttribute) { + if (node.name.name !== 'style') { + return; + } + + if (node.value && node.value.type === 'Literal' && typeof node.value.value === 'string') { + const value = node.value.value; + if (value.startsWith('#') || value.startsWith('rgb') || value.startsWith('hsl')) { + context.report({ + node, + message: 'Avoid using CSS colors', + }); + } + } + }, + }; + }, +}; diff --git a/tsconfig.base.json b/tsconfig.base.json index 005dae5b423c4..c8363814b3084 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -848,6 +848,8 @@ "@kbn/es-ui-shared-plugin/*": ["src/plugins/es_ui_shared/*"], "@kbn/eslint-config": ["packages/kbn-eslint-config"], "@kbn/eslint-config/*": ["packages/kbn-eslint-config/*"], + "@kbn/eslint-plugin-design-tokens": ["packages/kbn-eslint-plugin-design-tokens"], + "@kbn/eslint-plugin-design-tokens/*": ["packages/kbn-eslint-plugin-design-tokens/*"], "@kbn/eslint-plugin-disable": ["packages/kbn-eslint-plugin-disable"], "@kbn/eslint-plugin-disable/*": ["packages/kbn-eslint-plugin-disable/*"], "@kbn/eslint-plugin-eslint": ["packages/kbn-eslint-plugin-eslint"], diff --git a/yarn.lock b/yarn.lock index 6fb2b63c27a5f..51b6aaeea2f72 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5512,6 +5512,10 @@ version "0.0.0" uid "" +"@kbn/eslint-plugin-design-tokens@link:packages/kbn-eslint-plugin-design-tokens": + version "0.0.0" + uid "" + "@kbn/eslint-plugin-disable@link:packages/kbn-eslint-plugin-disable": version "0.0.0" uid "" From e9f084052081ab60ab332221b2bbcbf96f36e9fa Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 18 Nov 2024 22:28:39 +0100 Subject: [PATCH 02/23] add test and implementation for prefer css rule --- ..._css_attributes_for_eui_components.test.ts | 89 +++++++++++++++++++ ...refer_css_attributes_for_eui_components.ts | 62 +++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts create mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts b/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts new file mode 100644 index 0000000000000..2cbcf347ed9d0 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts @@ -0,0 +1,89 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { RuleTester } from 'eslint'; +import { preferCSSAttributeForEuiComponents } from './prefer_css_attributes_for_eui_components'; + +const tsTester = [ + '@typescript-eslint/parser', + new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + ecmaFeatures: { + jsx: true, + }, + }, + }), +] as const; + +const babelTester = [ + '@babel/eslint-parser', + new RuleTester({ + parser: require.resolve('@babel/eslint-parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + requireConfigFile: false, + babelOptions: { + presets: ['@kbn/babel-preset/node_preset'], + }, + }, + }), +] as const; + +const invalid: RuleTester.InvalidTestCase[] = [ + { + name: 'Prefer the JSX css attribute for EUI components', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [ + { + messageId: 'preferCSSAttributeForEuiComponents', + }, + ], + output: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + }, +]; + +const valid: RuleTester.ValidTestCase[] = [ + { + name: invalid[0].name, + filename: invalid[0].filename, + code: invalid[0].output as string, + }, +]; + +for (const [name, tester] of [tsTester, babelTester]) { + describe(name, () => { + tester.run( + '@kbn/prefer_css_attributes_for_eui_components', + preferCSSAttributeForEuiComponents, + { + valid, + invalid, + } + ); + }); +} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts b/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts new file mode 100644 index 0000000000000..919b74d5a4725 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts @@ -0,0 +1,62 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import type { Rule } from 'eslint'; +import type { TSESTree } from '@typescript-eslint/typescript-estree'; +import type { Identifier, Node } from 'estree'; + +export const preferCSSAttributeForEuiComponents: Rule.RuleModule = { + meta: { + type: 'problem', + docs: { + description: 'Prefer the JSX css attribute for EUI components', + category: 'Best Practices', + recommended: true, + }, + messages: { + preferCSSAttributeForEuiComponents: 'Prefer the css attribute for EUI components', + }, + fixable: 'code', + schema: [], + }, + create(context) { + const isNamedEuiComponentRegex = /^Eui[A-Z]*/; + + return { + JSXOpeningElement(node: TSESTree.JSXOpeningElement) { + if (isNamedEuiComponentRegex.test((node.name as unknown as Identifier).name)) { + let styleAttrNode: TSESTree.JSXAttribute | undefined; + + if ( + // @ts-expect-error the returned result is somehow typed as a union of JSXAttribute and JSXAttributeSpread + (styleAttrNode = node.attributes.find( + (attr) => attr.type === 'JSXAttribute' && attr.name.name === 'style' + )) + ) { + context.report({ + node: styleAttrNode?.parent! as Node, + messageId: 'preferCSSAttributeForEuiComponents', + fix(fixer) { + const cssAttr = node.attributes.find( + (attr) => attr.type === 'JSXAttribute' && attr.name.name === 'css' + ); + + if (cssAttr) { + return null; + } + + return fixer.replaceTextRange(styleAttrNode?.name?.range!, 'css'); + }, + }); + } + } + }, + }; + }, +}; From 10c7dd2715e22abe1390ea9f7462020743e480b9 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 18 Nov 2024 22:35:30 +0100 Subject: [PATCH 03/23] add implementation and test for no css colro value rule --- .../rules/no_css_color.test.ts | 106 ++++++++++ .../rules/no_css_color.test.tsx | 53 ----- .../rules/no_css_color.ts | 192 +++++++++++++++++- 3 files changed, 290 insertions(+), 61 deletions(-) create mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts delete mode 100644 packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts new file mode 100644 index 0000000000000..718cc364f4501 --- /dev/null +++ b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts @@ -0,0 +1,106 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the "Elastic License + * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side + * Public License v 1"; you may not use this file except in compliance with, at + * your election, the "Elastic License 2.0", the "GNU Affero General Public + * License v3.0 only", or the "Server Side Public License, v 1". + */ + +import { RuleTester } from 'eslint'; +import { noCssColor } from './no_css_color'; + +const tsTester = [ + '@typescript-eslint/parser', + new RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + ecmaFeatures: { + jsx: true, + }, + }, + }), +] as const; + +const babelTester = [ + '@babel/eslint-parser', + new RuleTester({ + parser: require.resolve('@babel/eslint-parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2018, + requireConfigFile: false, + babelOptions: { + presets: ['@kbn/babel-preset/node_preset'], + }, + }, + }), +] as const; + +const invalid: RuleTester.InvalidTestCase[] = [ + { + name: 'Raises an error when a CSS color is used in a JSX style attribute', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, + { + name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, + { + name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with the css template function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, + { + name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with regular function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + ({ color: '#dd4040' })}>This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, +]; + +const valid: RuleTester.ValidTestCase[] = []; + +for (const [name, tester] of [tsTester, babelTester]) { + describe(name, () => { + tester.run('@kbn/no_css_color', noCssColor, { + valid, + invalid, + }); + }); +} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx deleted file mode 100644 index 570dce461f803..0000000000000 --- a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.tsx +++ /dev/null @@ -1,53 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the "Elastic License - * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side - * Public License v 1"; you may not use this file except in compliance with, at - * your election, the "Elastic License 2.0", the "GNU Affero General Public - * License v3.0 only", or the "Server Side Public License, v 1". - */ - -import { RuleTester } from 'eslint'; -import { noCssColor } from './no_css_color'; - -const tsTester = [ - '@typescript-eslint/parser', - new RuleTester({ - parser: require.resolve('@typescript-eslint/parser'), - parserOptions: { - sourceType: 'module', - ecmaVersion: 2018, - ecmaFeatures: { - jsx: true, - }, - }, - }), -] as const; - -const babelTester = [ - '@babel/eslint-parser', - new RuleTester({ - parser: require.resolve('@babel/eslint-parser'), - parserOptions: { - sourceType: 'module', - ecmaVersion: 2018, - requireConfigFile: false, - babelOptions: { - presets: ['@kbn/babel-preset/node_preset'], - }, - }, - }), -] as const; - -const invalid: RuleTester.InvalidTestCase[] = []; - -const valid: RuleTester.ValidTestCase[] = []; - -for (const [name, tester] of [tsTester, babelTester]) { - describe(name, () => { - tester.run('@kbn/no_css_color', noCssColor, { - valid, - invalid, - }); - }); -} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts index 0fe9d31b47d77..0e93a031a5d56 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts @@ -19,22 +19,198 @@ export const noCssColor: Rule.RuleModule = { recommended: true, url: 'https://eui.elastic.co/#/theming/colors/values', }, + messages: { + noCssColor: 'Avoid using CSS colors', + }, schema: [], }, create(context) { return { JSXAttribute(node: TSESTree.JSXAttribute) { - if (node.name.name !== 'style') { + if (!(node.name.name === 'style' || node.name.name === 'css')) { return; } - if (node.value && node.value.type === 'Literal' && typeof node.value.value === 'string') { - const value = node.value.value; - if (value.startsWith('#') || value.startsWith('rgb') || value.startsWith('hsl')) { - context.report({ - node, - message: 'Avoid using CSS colors', - }); + if (node.name.name === 'style') { + /** + * @example This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'TemplateLiteral' + ) { + const declarationTemplateNode = node.value.expression.quasis[0]; + + if (/color\:\s?(\'|\")?(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + } + } + + /** + * @example This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'ObjectExpression' + ) { + const declarationTemplateNode = node.value.expression.properties; + + if (!declarationTemplateNode.length) { + return; + } + + for (let i = 0; i < declarationTemplateNode.length; i++) { + const property = declarationTemplateNode[i]; + if ( + property.type === 'Property' && + property.key.type === 'Identifier' && + property.key.name === 'color' && + property.value.type === 'Literal' && + property.value.value && + /(#|rgb|hsl).*/.test(String(property.value.value)) + ) { + context.report({ + node: property.key, + messageId: 'noCssColor', + }); + + break; + } + } + } + } + + if (node.name.name === 'css') { + /** + * @example This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'TemplateLiteral' + ) { + const declarationTemplateNode = node.value.expression.quasis[0]; + + if (/color\:\s?(\'|\")(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + } + } + + /** + * @example This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'ObjectExpression' + ) { + const declarationTemplateNode = node.value.expression.properties; + + if (!declarationTemplateNode.length) { + return; + } + + for (let i = 0; i < declarationTemplateNode.length; i++) { + const property = declarationTemplateNode[i]; + + if ( + property.type === 'Property' && + property.key.type === 'Identifier' && + property.key.name === 'color' && + property.value.type === 'Literal' && + property.value.value && + /(#|rgb|hsl).*/.test(String(property.value.value)) + ) { + context.report({ + node: property.key, + messageId: 'noCssColor', + }); + + break; + } + } + } + + /** + * @example This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'TaggedTemplateExpression' && + node.value.expression.tag.type === 'Identifier' && + node.value.expression.tag.name === 'css' + ) { + const declarationTemplateNode = node.value.expression.quasi.quasis[0]; + + if (/color\:\s?(\'|\")(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + } + } + + /** + * @example ({ color: '#dd4040' })}>This is a test + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + (node.value.expression.type === 'FunctionExpression' || + node.value.expression.type === 'ArrowFunctionExpression') + ) { + let declarationPropertiesNode: TSESTree.Property[] = []; + + if (node.value.expression.body.type === 'ObjectExpression') { + // @ts-expect-error + declarationPropertiesNode = node.value.expression.body.properties; + } + + if (node.value.expression.body.type === 'BlockStatement') { + const functionReturnStatementNode = node.value.expression.body.body?.find((_node) => { + return _node.type === 'ReturnStatement'; + }); + + if (!functionReturnStatementNode) { + return; + } + + declarationPropertiesNode = // @ts-expect-error + (functionReturnStatementNode as TSESTree.ReturnStatement).argument?.properties; + } + + if (!declarationPropertiesNode.length) { + return; + } + + for (let i = 0; i < declarationPropertiesNode.length; i++) { + const property = declarationPropertiesNode[i]; + + if ( + property.type === 'Property' && + property.key.type === 'Identifier' && + property.key.name === 'color' && + property.value.type === 'Literal' && + property.value.value && + /(#|rgb|hsl).*/.test(String(property.value.value)) + ) { + context.report({ + node: property.key, + messageId: 'noCssColor', + }); + + break; + } + } } } }, From 759409bd670cf5f041a65382c9589ce3171e41bd Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 19 Nov 2024 11:50:42 +0100 Subject: [PATCH 04/23] start on adding documentation for new eslint rules --- .../README.mdx | 20 +++++++++++++++++++ .../kbn-eslint-plugin-design-tokens/index.ts | 6 ++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/kbn-eslint-plugin-design-tokens/README.mdx b/packages/kbn-eslint-plugin-design-tokens/README.mdx index e69de29bb2d1d..051a0993dc01f 100644 --- a/packages/kbn-eslint-plugin-design-tokens/README.mdx +++ b/packages/kbn-eslint-plugin-design-tokens/README.mdx @@ -0,0 +1,20 @@ +--- +id: kibSharedUXEslintPluginDesignTokens +slug: /kibana-dev-docs/shared-ux/packages/kbn-eslint-plugin-design-tokens +title: '@kbn/eslint-plugin-design-tokens' +description: Custom ESLint rules to guardrails for using eui in the Kibana repository +date: 2024-11-19 +tags: ['kibana', 'dev', 'contributor', 'shared_ux', 'eslint', 'eui'] +--- + +# Summary + +`@kbn/eslint-plugin-design-tokens` is an ESLint plugin providing custom ESLint rules to help setup guardrails for using eui in the Kibana repo especially around styling. + +The aim of this package is to help engineers to modify EUI components in a much complaint way. + +If a rule does not behave as you expect or you have an idea of how these rules can be improved, please reach out to the Shared UX team. + +# Rules + +... \ No newline at end of file diff --git a/packages/kbn-eslint-plugin-design-tokens/index.ts b/packages/kbn-eslint-plugin-design-tokens/index.ts index b69989cf93538..2915ffd79cb8c 100644 --- a/packages/kbn-eslint-plugin-design-tokens/index.ts +++ b/packages/kbn-eslint-plugin-design-tokens/index.ts @@ -8,12 +8,14 @@ */ import { noCssColor } from './rules/no_css_color'; +import { preferCSSAttributeForEuiComponents } from './rules/prefer_css_attributes_for_eui_components'; /** - * Custom ESLint rules, add `'@kbn/eslint-plugin-design-tokens'` to your eslint config to use them + * Custom ESLint rules, add `'@kbn/eslint_plugin_design_tokens'` to your eslint config to use them * @internal */ const rules = { - 'no-css-color': noCssColor, + no_css_color: noCssColor, + prefer_css_attributes_for_eui_components: preferCSSAttributeForEuiComponents, }; From 664f991a00f5c5241b5b26e8af28bf75ea54941f Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 19 Nov 2024 12:14:37 +0100 Subject: [PATCH 05/23] update regex for matching CSS color values --- .../rules/no_css_color.ts | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts index 0e93a031a5d56..f61d4ed771dee 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts @@ -42,7 +42,11 @@ export const noCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasis[0]; - if (/color\:\s?(\'|\")?(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + if ( + /color\:\s?(\'|\")?(#|rgb|hsl|hwb|lab|lch|oklab)/.test( + declarationTemplateNode.value.raw + ) + ) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -72,7 +76,7 @@ export const noCssColor: Rule.RuleModule = { property.key.name === 'color' && property.value.type === 'Literal' && property.value.value && - /(#|rgb|hsl).*/.test(String(property.value.value)) + /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) ) { context.report({ node: property.key, @@ -96,7 +100,11 @@ export const noCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasis[0]; - if (/color\:\s?(\'|\")(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + if ( + /color\:\s?(\'|\")(#|rgb|hsl|hwb|lab|lch|oklab)/.test( + declarationTemplateNode.value.raw + ) + ) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -127,7 +135,7 @@ export const noCssColor: Rule.RuleModule = { property.key.name === 'color' && property.value.type === 'Literal' && property.value.value && - /(#|rgb|hsl).*/.test(String(property.value.value)) + /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) ) { context.report({ node: property.key, @@ -151,7 +159,11 @@ export const noCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasi.quasis[0]; - if (/color\:\s?(\'|\")(#|rgb|hsl)/.test(declarationTemplateNode.value.raw)) { + if ( + /color\:\s?(\'|\")(#|rgb|hsl|hwb|lab|lch|oklab)/.test( + declarationTemplateNode.value.raw + ) + ) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -201,7 +213,7 @@ export const noCssColor: Rule.RuleModule = { property.key.name === 'color' && property.value.type === 'Literal' && property.value.value && - /(#|rgb|hsl).*/.test(String(property.value.value)) + /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) ) { context.report({ node: property.key, From 13fe975e764a7bab48f8b6eafdf72c530c7ce372 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Tue, 19 Nov 2024 14:13:52 +0000 Subject: [PATCH 06/23] [CI] Auto-commit changed files from 'node scripts/generate codeowners' --- .github/CODEOWNERS | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 79b7d17b7b4a2..7fbead6f108cc 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -345,6 +345,7 @@ packages/kbn-es-errors @elastic/kibana-core packages/kbn-es-query @elastic/kibana-data-discovery packages/kbn-es-types @elastic/kibana-core @elastic/obs-knowledge-team packages/kbn-eslint-config @elastic/kibana-operations +packages/kbn-eslint-plugin-design-tokens @elastic/shared-ux packages/kbn-eslint-plugin-disable @elastic/kibana-operations packages/kbn-eslint-plugin-eslint @elastic/kibana-operations packages/kbn-eslint-plugin-i18n @elastic/obs-knowledge-team @elastic/kibana-operations From 855ee3e34ec71ebb499c0b44b74ef5aaefb2bc70 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 19 Nov 2024 12:14:37 +0100 Subject: [PATCH 07/23] overhaul package take, config and apply rules to codebase --- .eslintrc.js | 1 - package.json | 2 +- packages/kbn-eslint-config/.eslintrc.js | 3 +++ .../README.mdx | 6 +++--- .../index.ts | 13 ++++++------- .../jest.config.js | 2 +- .../kibana.jsonc | 2 +- packages/kbn-eslint-plugin-css/package.json | 6 ++++++ .../src}/rules/no_css_color.test.ts | 19 ++++++++++++++++--- .../src}/rules/no_css_color.ts | 8 ++++---- ..._css_attribute_for_eui_components.test.ts} | 14 +++++--------- ...refer_css_attribute_for_eui_components.ts} | 4 ++-- packages/kbn-eslint-plugin-css/tsconfig.json | 11 +++++++++++ .../package.json | 9 --------- tsconfig.base.json | 8 +++----- yarn.lock | 2 +- 16 files changed, 63 insertions(+), 47 deletions(-) rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css}/README.mdx (62%) rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css}/index.ts (57%) rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css}/jest.config.js (89%) rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css}/kibana.jsonc (65%) create mode 100644 packages/kbn-eslint-plugin-css/package.json rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css/src}/rules/no_css_color.test.ts (83%) rename packages/{kbn-eslint-plugin-design-tokens => kbn-eslint-plugin-css/src}/rules/no_css_color.ts (97%) rename packages/{kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts => kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.test.ts} (88%) rename packages/{kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts => kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.ts} (95%) create mode 100644 packages/kbn-eslint-plugin-css/tsconfig.json delete mode 100644 packages/kbn-eslint-plugin-design-tokens/package.json diff --git a/.eslintrc.js b/.eslintrc.js index 2bdcaf9901474..18ea48fdf8cbb 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -764,7 +764,6 @@ module.exports = { ], }, }, - /** * Jest specific rules */ diff --git a/package.json b/package.json index c631a1444c821..84c8a1028c8ef 100644 --- a/package.json +++ b/package.json @@ -1450,7 +1450,7 @@ "@kbn/es": "link:packages/kbn-es", "@kbn/es-archiver": "link:packages/kbn-es-archiver", "@kbn/eslint-config": "link:packages/kbn-eslint-config", - "@kbn/eslint-plugin-design-tokens": "link:packages/kbn-eslint-plugin-design-tokens", + "@kbn/eslint-plugin-css": "link:packages/kbn-eslint-plugin-css", "@kbn/eslint-plugin-disable": "link:packages/kbn-eslint-plugin-disable", "@kbn/eslint-plugin-eslint": "link:packages/kbn-eslint-plugin-eslint", "@kbn/eslint-plugin-i18n": "link:packages/kbn-eslint-plugin-i18n", diff --git a/packages/kbn-eslint-config/.eslintrc.js b/packages/kbn-eslint-config/.eslintrc.js index e38b6cc48c443..38c45d89af7fd 100644 --- a/packages/kbn-eslint-config/.eslintrc.js +++ b/packages/kbn-eslint-config/.eslintrc.js @@ -28,6 +28,7 @@ module.exports = { '@kbn/eslint-plugin-imports', '@kbn/eslint-plugin-telemetry', '@kbn/eslint-plugin-i18n', + '@kbn/eslint-plugin-css', 'eslint-plugin-depend', 'prettier', ], @@ -332,6 +333,8 @@ module.exports = { '@kbn/imports/no_boundary_crossing': 'error', '@kbn/imports/no_group_crossing_manifests': 'error', '@kbn/imports/no_group_crossing_imports': 'error', + '@kbn/css/no_css_color': 'warn', + '@kbn/css/prefer_css_attributes_for_eui_components': 'warn', 'no-new-func': 'error', 'no-implied-eval': 'error', 'no-prototype-builtins': 'error', diff --git a/packages/kbn-eslint-plugin-design-tokens/README.mdx b/packages/kbn-eslint-plugin-css/README.mdx similarity index 62% rename from packages/kbn-eslint-plugin-design-tokens/README.mdx rename to packages/kbn-eslint-plugin-css/README.mdx index 051a0993dc01f..09439d56aec00 100644 --- a/packages/kbn-eslint-plugin-design-tokens/README.mdx +++ b/packages/kbn-eslint-plugin-css/README.mdx @@ -1,6 +1,6 @@ --- -id: kibSharedUXEslintPluginDesignTokens -slug: /kibana-dev-docs/shared-ux/packages/kbn-eslint-plugin-design-tokens +id: kibSharedUXEslintPluginCSS +slug: /kibana-dev-docs/shared-ux/packages/kbn-eslint-plugin-css title: '@kbn/eslint-plugin-design-tokens' description: Custom ESLint rules to guardrails for using eui in the Kibana repository date: 2024-11-19 @@ -9,7 +9,7 @@ tags: ['kibana', 'dev', 'contributor', 'shared_ux', 'eslint', 'eui'] # Summary -`@kbn/eslint-plugin-design-tokens` is an ESLint plugin providing custom ESLint rules to help setup guardrails for using eui in the Kibana repo especially around styling. +`@kbn/eslint-plugin-css` is an ESLint plugin providing custom ESLint rules to help setup guardrails for using eui in the Kibana repo especially around styling. The aim of this package is to help engineers to modify EUI components in a much complaint way. diff --git a/packages/kbn-eslint-plugin-design-tokens/index.ts b/packages/kbn-eslint-plugin-css/index.ts similarity index 57% rename from packages/kbn-eslint-plugin-design-tokens/index.ts rename to packages/kbn-eslint-plugin-css/index.ts index 2915ffd79cb8c..9ea4bcc67619f 100644 --- a/packages/kbn-eslint-plugin-design-tokens/index.ts +++ b/packages/kbn-eslint-plugin-css/index.ts @@ -7,15 +7,14 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ -import { noCssColor } from './rules/no_css_color'; -import { preferCSSAttributeForEuiComponents } from './rules/prefer_css_attributes_for_eui_components'; +import { NoCssColor } from './src/rules/no_css_color'; +import { PreferCSSAttributeForEuiComponents } from './src/rules/prefer_css_attribute_for_eui_components'; /** - * Custom ESLint rules, add `'@kbn/eslint_plugin_design_tokens'` to your eslint config to use them + * Custom ESLint rules, included as `'@kbn/eslint-plugin-design-tokens'` in the kibana eslint config * @internal */ - -const rules = { - no_css_color: noCssColor, - prefer_css_attributes_for_eui_components: preferCSSAttributeForEuiComponents, +export const rules = { + no_css_color: NoCssColor, + prefer_css_attributes_for_eui_components: PreferCSSAttributeForEuiComponents, }; diff --git a/packages/kbn-eslint-plugin-design-tokens/jest.config.js b/packages/kbn-eslint-plugin-css/jest.config.js similarity index 89% rename from packages/kbn-eslint-plugin-design-tokens/jest.config.js rename to packages/kbn-eslint-plugin-css/jest.config.js index 41b5394f803b6..c8ae20237eae8 100644 --- a/packages/kbn-eslint-plugin-design-tokens/jest.config.js +++ b/packages/kbn-eslint-plugin-css/jest.config.js @@ -10,5 +10,5 @@ module.exports = { preset: '@kbn/test', rootDir: '../..', - roots: ['/packages/kbn-eslint-plugin-design-tokens'], + roots: ['/packages/kbn-eslint-plugin-css'], }; diff --git a/packages/kbn-eslint-plugin-design-tokens/kibana.jsonc b/packages/kbn-eslint-plugin-css/kibana.jsonc similarity index 65% rename from packages/kbn-eslint-plugin-design-tokens/kibana.jsonc rename to packages/kbn-eslint-plugin-css/kibana.jsonc index 2602cf919bd68..85a31c4825368 100644 --- a/packages/kbn-eslint-plugin-design-tokens/kibana.jsonc +++ b/packages/kbn-eslint-plugin-css/kibana.jsonc @@ -1,6 +1,6 @@ { "type": "shared-common", - "id": "@kbn/eslint-plugin-design-tokens", + "id": "@kbn/eslint-plugin-css", "devOnly": true, "owner": "@elastic/shared-ux" } diff --git a/packages/kbn-eslint-plugin-css/package.json b/packages/kbn-eslint-plugin-css/package.json new file mode 100644 index 0000000000000..222f1ec3a7025 --- /dev/null +++ b/packages/kbn-eslint-plugin-css/package.json @@ -0,0 +1,6 @@ +{ + "name": "@kbn/eslint-plugin-css", + "version": "0.0.1", + "private": true, + "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0" +} diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts similarity index 83% rename from packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts rename to packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 718cc364f4501..efdca943d814f 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -8,7 +8,7 @@ */ import { RuleTester } from 'eslint'; -import { noCssColor } from './no_css_color'; +import { NoCssColor } from './no_css_color'; const tsTester = [ '@typescript-eslint/parser', @@ -80,7 +80,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColor' }], }, { - name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with regular function', + name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with an arrow function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; @@ -92,13 +92,26 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColor' }], }, + { + name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with a regular function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, ]; const valid: RuleTester.ValidTestCase[] = []; for (const [name, tester] of [tsTester, babelTester]) { describe(name, () => { - tester.run('@kbn/no_css_color', noCssColor, { + tester.run('@kbn/no_css_color', NoCssColor, { valid, invalid, }); diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts similarity index 97% rename from packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts rename to packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index f61d4ed771dee..8e568f6eac010 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -10,9 +10,9 @@ import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; -export const noCssColor: Rule.RuleModule = { +export const NoCssColor: Rule.RuleModule = { meta: { - type: 'problem', + type: 'suggestion', docs: { description: 'Use color definitions from eui theme as opposed to CSS color values', category: 'Best Practices', @@ -55,7 +55,7 @@ export const noCssColor: Rule.RuleModule = { } /** - * @example This is a test + * @example This is a test */ if ( node.value && @@ -113,7 +113,7 @@ export const noCssColor: Rule.RuleModule = { } /** - * @example This is a test + * @example This is a test */ if ( node.value && diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts b/packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.test.ts similarity index 88% rename from packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts rename to packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.test.ts index 2cbcf347ed9d0..f1534ec5c861f 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.test.ts @@ -8,7 +8,7 @@ */ import { RuleTester } from 'eslint'; -import { preferCSSAttributeForEuiComponents } from './prefer_css_attributes_for_eui_components'; +import { PreferCSSAttributeForEuiComponents } from './prefer_css_attribute_for_eui_components'; const tsTester = [ '@typescript-eslint/parser', @@ -77,13 +77,9 @@ const valid: RuleTester.ValidTestCase[] = [ for (const [name, tester] of [tsTester, babelTester]) { describe(name, () => { - tester.run( - '@kbn/prefer_css_attributes_for_eui_components', - preferCSSAttributeForEuiComponents, - { - valid, - invalid, - } - ); + tester.run('@kbn/prefer_css_attribute_for_eui_components', PreferCSSAttributeForEuiComponents, { + valid, + invalid, + }); }); } diff --git a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts b/packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.ts similarity index 95% rename from packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts rename to packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.ts index 919b74d5a4725..f2b8bfd7b7d74 100644 --- a/packages/kbn-eslint-plugin-design-tokens/rules/prefer_css_attributes_for_eui_components.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/prefer_css_attribute_for_eui_components.ts @@ -11,9 +11,9 @@ import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; import type { Identifier, Node } from 'estree'; -export const preferCSSAttributeForEuiComponents: Rule.RuleModule = { +export const PreferCSSAttributeForEuiComponents: Rule.RuleModule = { meta: { - type: 'problem', + type: 'suggestion', docs: { description: 'Prefer the JSX css attribute for EUI components', category: 'Best Practices', diff --git a/packages/kbn-eslint-plugin-css/tsconfig.json b/packages/kbn-eslint-plugin-css/tsconfig.json new file mode 100644 index 0000000000000..a6dec1c1a62c5 --- /dev/null +++ b/packages/kbn-eslint-plugin-css/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../tsconfig.base.json", + "compilerOptions": { + "outDir": "target/types", + "types": ["jest", "node"], + "lib": ["es2021"] + }, + "include": ["**/*.ts"], + "exclude": ["target/**/*"], + "kbn_references": [] +} diff --git a/packages/kbn-eslint-plugin-design-tokens/package.json b/packages/kbn-eslint-plugin-design-tokens/package.json deleted file mode 100644 index 789bc5698d59b..0000000000000 --- a/packages/kbn-eslint-plugin-design-tokens/package.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "name": "@kbn/eslint-plugin-design-tokens", - "version": "1.0.0", - "private": true, - "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0", - "peerDependencies": { - "eslint": "^7.0.0" - } -} diff --git a/tsconfig.base.json b/tsconfig.base.json index c8363814b3084..e3f990d0d39a5 100644 --- a/tsconfig.base.json +++ b/tsconfig.base.json @@ -848,8 +848,8 @@ "@kbn/es-ui-shared-plugin/*": ["src/plugins/es_ui_shared/*"], "@kbn/eslint-config": ["packages/kbn-eslint-config"], "@kbn/eslint-config/*": ["packages/kbn-eslint-config/*"], - "@kbn/eslint-plugin-design-tokens": ["packages/kbn-eslint-plugin-design-tokens"], - "@kbn/eslint-plugin-design-tokens/*": ["packages/kbn-eslint-plugin-design-tokens/*"], + "@kbn/eslint-plugin-css": ["packages/kbn-eslint-plugin-css"], + "@kbn/eslint-plugin-css/*": ["packages/kbn-eslint-plugin-css/*"], "@kbn/eslint-plugin-disable": ["packages/kbn-eslint-plugin-disable"], "@kbn/eslint-plugin-disable/*": ["packages/kbn-eslint-plugin-disable/*"], "@kbn/eslint-plugin-eslint": ["packages/kbn-eslint-plugin-eslint"], @@ -2080,9 +2080,7 @@ "@kbn/zod-helpers/*": ["packages/kbn-zod-helpers/*"], // END AUTOMATED PACKAGE LISTING // Allows for importing from `kibana` package for the exported types. - "@emotion/core": [ - "typings/@emotion" - ] + "@emotion/core": ["typings/@emotion"] }, // Support .tsx files and transform JSX into calls to React.createElement "jsx": "react", diff --git a/yarn.lock b/yarn.lock index 51b6aaeea2f72..8d4a2b347ccef 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5512,7 +5512,7 @@ version "0.0.0" uid "" -"@kbn/eslint-plugin-design-tokens@link:packages/kbn-eslint-plugin-design-tokens": +"@kbn/eslint-plugin-css@link:packages/kbn-eslint-plugin-css": version "0.0.0" uid "" From a64f092cb7c770c49cf0bdfbaa0d13fbe1a850ff Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 19 Nov 2024 18:42:54 +0100 Subject: [PATCH 08/23] add support for more css properties that accept color values --- .../src/rules/no_css_color.test.ts | 8 +- .../src/rules/no_css_color.ts | 114 +++++++++--------- 2 files changed, 59 insertions(+), 63 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index efdca943d814f..47d6a2aa601b6 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -54,7 +54,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColor' }], }, { - name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents', + name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; @@ -67,7 +67,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColor' }], }, { - name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with the css template function', + name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with the css template function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; @@ -80,7 +80,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColor' }], }, { - name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with an arrow function', + name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with an arrow function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; @@ -93,7 +93,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColor' }], }, { - name: 'Raises an error when a CSS color is used in a JSX css attribute for EuiComponents with a regular function', + name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with a regular function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 8e568f6eac010..5fc7a9fc966d9 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -10,6 +10,49 @@ import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; +/** + * @description Regex to match css color values, + * see {@link https://developer.mozilla.org/en-US/docs/Web/CSS/color_value} for definitions of valid css color values + */ +const cssColorRegex = /(#|rgb|hsl|hwb|lab|lch|oklab).*/; + +/** + * @description List of css properties that can that can apply color to html box element and text node + */ +const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', 'border']; + +/** + * @description Builds off the existing color definition regex to match css declarations that can apply color to html elements and text nodes + */ +const htmlElementColorDeclarationRegex = RegExp( + String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` +); + +const raiseReportIfPropertyHasCssColor = ( + context: Rule.RuleContext, + node: TSESTree.ObjectLiteralElement +) => { + let didReport: boolean; + + // checks if property value is a css color value for instances where style declaration is computed from an object + if ( + (didReport = Boolean( + node.type === 'Property' && + node.key.type === 'Identifier' && + node.value.type === 'Literal' && + propertiesSupportingCssColor.indexOf(node.key.name) > -1 && + cssColorRegex.test(String(node.value.value) ?? '') + )) + ) { + context.report({ + loc: node.loc, + messageId: 'noCssColor', + }); + } + + return didReport; +}; + export const NoCssColor: Rule.RuleModule = { meta: { type: 'suggestion', @@ -33,7 +76,7 @@ export const NoCssColor: Rule.RuleModule = { if (node.name.name === 'style') { /** - * @example This is a test + * @example This is an example */ if ( node.value && @@ -42,11 +85,7 @@ export const NoCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasis[0]; - if ( - /color\:\s?(\'|\")?(#|rgb|hsl|hwb|lab|lch|oklab)/.test( - declarationTemplateNode.value.raw - ) - ) { + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -55,7 +94,7 @@ export const NoCssColor: Rule.RuleModule = { } /** - * @example This is a test + * @example This is an example */ if ( node.value && @@ -70,19 +109,8 @@ export const NoCssColor: Rule.RuleModule = { for (let i = 0; i < declarationTemplateNode.length; i++) { const property = declarationTemplateNode[i]; - if ( - property.type === 'Property' && - property.key.type === 'Identifier' && - property.key.name === 'color' && - property.value.type === 'Literal' && - property.value.value && - /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) - ) { - context.report({ - node: property.key, - messageId: 'noCssColor', - }); + if (raiseReportIfPropertyHasCssColor(context, property)) { break; } } @@ -91,7 +119,7 @@ export const NoCssColor: Rule.RuleModule = { if (node.name.name === 'css') { /** - * @example This is a test + * @example This is an example */ if ( node.value && @@ -100,11 +128,7 @@ export const NoCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasis[0]; - if ( - /color\:\s?(\'|\")(#|rgb|hsl|hwb|lab|lch|oklab)/.test( - declarationTemplateNode.value.raw - ) - ) { + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -113,7 +137,7 @@ export const NoCssColor: Rule.RuleModule = { } /** - * @example This is a test + * @example This is an example */ if ( node.value && @@ -129,26 +153,14 @@ export const NoCssColor: Rule.RuleModule = { for (let i = 0; i < declarationTemplateNode.length; i++) { const property = declarationTemplateNode[i]; - if ( - property.type === 'Property' && - property.key.type === 'Identifier' && - property.key.name === 'color' && - property.value.type === 'Literal' && - property.value.value && - /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) - ) { - context.report({ - node: property.key, - messageId: 'noCssColor', - }); - + if (raiseReportIfPropertyHasCssColor(context, property)) { break; } } } /** - * @example This is a test + * @example This is an example */ if ( node.value && @@ -159,11 +171,7 @@ export const NoCssColor: Rule.RuleModule = { ) { const declarationTemplateNode = node.value.expression.quasi.quasis[0]; - if ( - /color\:\s?(\'|\")(#|rgb|hsl|hwb|lab|lch|oklab)/.test( - declarationTemplateNode.value.raw - ) - ) { + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { context.report({ node: declarationTemplateNode, messageId: 'noCssColor', @@ -172,7 +180,7 @@ export const NoCssColor: Rule.RuleModule = { } /** - * @example ({ color: '#dd4040' })}>This is a test + * @example ({ color: '#dd4040' })}>This is an example */ if ( node.value && @@ -207,19 +215,7 @@ export const NoCssColor: Rule.RuleModule = { for (let i = 0; i < declarationPropertiesNode.length; i++) { const property = declarationPropertiesNode[i]; - if ( - property.type === 'Property' && - property.key.type === 'Identifier' && - property.key.name === 'color' && - property.value.type === 'Literal' && - property.value.value && - /(#|rgb|hsl|hwb|lab|lch|oklab).*/.test(String(property.value.value)) - ) { - context.report({ - node: property.key, - messageId: 'noCssColor', - }); - + if (raiseReportIfPropertyHasCssColor(context, property)) { break; } } From d02fbc4346c42f66fc67a2c7ee81a4f5ce60d518 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Wed, 20 Nov 2024 12:45:27 +0100 Subject: [PATCH 09/23] add improvement --- packages/kbn-eslint-config/.eslintrc.js | 1 - .../src/rules/no_css_color.test.ts | 34 ++++++++++- .../src/rules/no_css_color.ts | 61 ++++++++++++------- 3 files changed, 71 insertions(+), 25 deletions(-) diff --git a/packages/kbn-eslint-config/.eslintrc.js b/packages/kbn-eslint-config/.eslintrc.js index 38c45d89af7fd..57a116a2b9b68 100644 --- a/packages/kbn-eslint-config/.eslintrc.js +++ b/packages/kbn-eslint-config/.eslintrc.js @@ -334,7 +334,6 @@ module.exports = { '@kbn/imports/no_group_crossing_manifests': 'error', '@kbn/imports/no_group_crossing_imports': 'error', '@kbn/css/no_css_color': 'warn', - '@kbn/css/prefer_css_attributes_for_eui_components': 'warn', 'no-new-func': 'error', 'no-implied-eval': 'error', 'no-prototype-builtins': 'error', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 47d6a2aa601b6..4cc81ed15887b 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -51,8 +51,36 @@ const invalid: RuleTester.InvalidTestCase[] = [ This is a test ) }`, + errors: [{ messageId: 'noCssColorSpecific' }], + }, + { + name: 'Raises an error when a CSS color is used in a JSX style attribute with template literals', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, errors: [{ messageId: 'noCssColor' }], }, + { + name: 'Raises an error when a CSS color is used for the background property in a JSX style attribute', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColorSpecific' }], + }, { name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', @@ -64,7 +92,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ This is a test ) }`, - errors: [{ messageId: 'noCssColor' }], + errors: [{ messageId: 'noCssColorSpecific' }], }, { name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with the css template function', @@ -90,7 +118,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ ({ color: '#dd4040' })}>This is a test ) }`, - errors: [{ messageId: 'noCssColor' }], + errors: [{ messageId: 'noCssColorSpecific' }], }, { name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with a regular function', @@ -103,7 +131,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ This is a test ) }`, - errors: [{ messageId: 'noCssColor' }], + errors: [{ messageId: 'noCssColorSpecific' }], }, ]; diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 5fc7a9fc966d9..3547850dd1573 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -22,7 +22,8 @@ const cssColorRegex = /(#|rgb|hsl|hwb|lab|lch|oklab).*/; const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', 'border']; /** - * @description Builds off the existing color definition regex to match css declarations that can apply color to html elements and text nodes + * @description Builds off the existing color definition regex to match css declarations that can apply color to + * html elements and text nodes for string declarations */ const htmlElementColorDeclarationRegex = RegExp( String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` @@ -46,7 +47,11 @@ const raiseReportIfPropertyHasCssColor = ( ) { context.report({ loc: node.loc, - messageId: 'noCssColor', + messageId: 'noCssColorSpecific', + data: { + // @ts-expect-error the key name is always pretty else this code will not execute + property: node.key.name, + }, }); } @@ -63,7 +68,9 @@ export const NoCssColor: Rule.RuleModule = { url: 'https://eui.elastic.co/#/theming/colors/values', }, messages: { - noCssColor: 'Avoid using CSS colors', + noCssColorSpecific: + 'Avoid using a literal CSS color value for {{property}}, use an EUI theme color instead', + noCssColor: 'Avoid using a literal CSS color value, use an EUI theme color instead', }, schema: [], }, @@ -83,13 +90,17 @@ export const NoCssColor: Rule.RuleModule = { node.value.type === 'JSXExpressionContainer' && node.value.expression.type === 'TemplateLiteral' ) { - const declarationTemplateNode = node.value.expression.quasis[0]; + for (let i = 0; i < node.value.expression.quasis.length; i++) { + const declarationTemplateNode = node.value.expression.quasis[i]; - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - node: declarationTemplateNode, - messageId: 'noCssColor', - }); + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { + context.report({ + loc: declarationTemplateNode.loc, + messageId: 'noCssColor', + }); + + break; + } } } @@ -126,13 +137,17 @@ export const NoCssColor: Rule.RuleModule = { node.value.type === 'JSXExpressionContainer' && node.value.expression.type === 'TemplateLiteral' ) { - const declarationTemplateNode = node.value.expression.quasis[0]; + for (let i = 0; i < node.value.expression.quasis.length; i++) { + const declarationTemplateNode = node.value.expression.quasis[i]; - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - node: declarationTemplateNode, - messageId: 'noCssColor', - }); + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + + break; + } } } @@ -169,13 +184,17 @@ export const NoCssColor: Rule.RuleModule = { node.value.expression.tag.type === 'Identifier' && node.value.expression.tag.name === 'css' ) { - const declarationTemplateNode = node.value.expression.quasi.quasis[0]; + for (let i = 0; i < node.value.expression.quasi.quasis.length; i++) { + const declarationTemplateNode = node.value.expression.quasi.quasis[i]; - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - node: declarationTemplateNode, - messageId: 'noCssColor', - }); + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { + context.report({ + loc: declarationTemplateNode.loc, + messageId: 'noCssColor', + }); + + break; + } } } From ac87368b57a2aca85c33014b7e1090d38f74a85a Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Wed, 20 Nov 2024 12:49:48 +0100 Subject: [PATCH 10/23] fix codeowners annotation --- .github/CODEOWNERS | 2 +- packages/kbn-eslint-plugin-css/kibana.jsonc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 7fbead6f108cc..1f2b4139c436d 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -345,7 +345,7 @@ packages/kbn-es-errors @elastic/kibana-core packages/kbn-es-query @elastic/kibana-data-discovery packages/kbn-es-types @elastic/kibana-core @elastic/obs-knowledge-team packages/kbn-eslint-config @elastic/kibana-operations -packages/kbn-eslint-plugin-design-tokens @elastic/shared-ux +packages/kbn-eslint-plugin-design-tokens @elastic/appex-sharedux packages/kbn-eslint-plugin-disable @elastic/kibana-operations packages/kbn-eslint-plugin-eslint @elastic/kibana-operations packages/kbn-eslint-plugin-i18n @elastic/obs-knowledge-team @elastic/kibana-operations diff --git a/packages/kbn-eslint-plugin-css/kibana.jsonc b/packages/kbn-eslint-plugin-css/kibana.jsonc index 85a31c4825368..3ee8bff8736f6 100644 --- a/packages/kbn-eslint-plugin-css/kibana.jsonc +++ b/packages/kbn-eslint-plugin-css/kibana.jsonc @@ -2,5 +2,5 @@ "type": "shared-common", "id": "@kbn/eslint-plugin-css", "devOnly": true, - "owner": "@elastic/shared-ux" + "owner": "@elastic/appex-sharedux" } From 7d3f999a36e16739370f8f72c064692031cdd467 Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Wed, 20 Nov 2024 12:01:10 +0000 Subject: [PATCH 11/23] [CI] Auto-commit changed files from 'node scripts/generate codeowners' --- .github/CODEOWNERS | 2 +- packages/kbn-eslint-plugin-css/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 1f2b4139c436d..9f060280bbf9a 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -345,7 +345,7 @@ packages/kbn-es-errors @elastic/kibana-core packages/kbn-es-query @elastic/kibana-data-discovery packages/kbn-es-types @elastic/kibana-core @elastic/obs-knowledge-team packages/kbn-eslint-config @elastic/kibana-operations -packages/kbn-eslint-plugin-design-tokens @elastic/appex-sharedux +packages/kbn-eslint-plugin-css @elastic/appex-sharedux packages/kbn-eslint-plugin-disable @elastic/kibana-operations packages/kbn-eslint-plugin-eslint @elastic/kibana-operations packages/kbn-eslint-plugin-i18n @elastic/obs-knowledge-team @elastic/kibana-operations diff --git a/packages/kbn-eslint-plugin-css/package.json b/packages/kbn-eslint-plugin-css/package.json index 222f1ec3a7025..c811f06f27cb7 100644 --- a/packages/kbn-eslint-plugin-css/package.json +++ b/packages/kbn-eslint-plugin-css/package.json @@ -1,6 +1,6 @@ { "name": "@kbn/eslint-plugin-css", - "version": "0.0.1", + "version": "1.0.0", "private": true, "license": "Elastic License 2.0 OR AGPL-3.0-only OR SSPL-1.0" } From 279f60ab722d2ef5f2d3e55c0a13f2ceb8957f1b Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 26 Nov 2024 13:41:10 +0100 Subject: [PATCH 12/23] even more improvements --- .../src/rules/no_css_color.test.ts | 7 +- .../src/rules/no_css_color.ts | 139 +++++++++++------- 2 files changed, 85 insertions(+), 61 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 4cc81ed15887b..7ee54c511cec4 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -60,13 +60,12 @@ const invalid: RuleTester.InvalidTestCase[] = [ import React from 'react'; function TestComponent() { + const codeStyle = { color: '#dd4040' }; return ( - This is a test + This is a test ) }`, - errors: [{ messageId: 'noCssColor' }], + errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], }, { name: 'Raises an error when a CSS color is used for the background property in a JSX style attribute', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 3547850dd1573..9beeaf6a8e28f 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -7,6 +7,7 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ +// import Color from 'color'; import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; @@ -29,30 +30,24 @@ const htmlElementColorDeclarationRegex = RegExp( String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` ); -const raiseReportIfPropertyHasCssColor = ( +const raiseReportIfPropertyHasInvalidCssColor = ( context: Rule.RuleContext, - node: TSESTree.ObjectLiteralElement + propertyNode: TSESTree.ObjectLiteralElement, + messageToReport: Rule.ReportDescriptor ) => { let didReport: boolean; // checks if property value is a css color value for instances where style declaration is computed from an object if ( (didReport = Boolean( - node.type === 'Property' && - node.key.type === 'Identifier' && - node.value.type === 'Literal' && - propertiesSupportingCssColor.indexOf(node.key.name) > -1 && - cssColorRegex.test(String(node.value.value) ?? '') + propertyNode.type === 'Property' && + propertyNode.key.type === 'Identifier' && + propertyNode.value.type === 'Literal' && + propertiesSupportingCssColor.indexOf(propertyNode.key.name) > -1 && + cssColorRegex.test(String(propertyNode.value.value) ?? '') )) ) { - context.report({ - loc: node.loc, - messageId: 'noCssColorSpecific', - data: { - // @ts-expect-error the key name is always pretty else this code will not execute - property: node.key.name, - }, - }); + context.report(messageToReport); } return didReport; @@ -68,8 +63,10 @@ export const NoCssColor: Rule.RuleModule = { url: 'https://eui.elastic.co/#/theming/colors/values', }, messages: { + noCSSColorSpecificDeclaredVariable: + 'Avoid using a literal CSS color value for "{{property}}", use an EUI theme color instead in declared variable {{variableName}} on line {{line}}', noCssColorSpecific: - 'Avoid using a literal CSS color value for {{property}}, use an EUI theme color instead', + 'Avoid using a literal CSS color value for "{{property}}", use an EUI theme color instead', noCssColor: 'Avoid using a literal CSS color value, use an EUI theme color instead', }, schema: [], @@ -83,48 +80,67 @@ export const NoCssColor: Rule.RuleModule = { if (node.name.name === 'style') { /** - * @example This is an example + * @example This is an example */ if ( node.value && node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'TemplateLiteral' + node.value.expression.type === 'ObjectExpression' ) { - for (let i = 0; i < node.value.expression.quasis.length; i++) { - const declarationTemplateNode = node.value.expression.quasis[i]; - - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - loc: declarationTemplateNode.loc, - messageId: 'noCssColor', - }); + const declarationPropertiesNode = node.value.expression.properties; + + declarationPropertiesNode?.forEach((property) => { + raiseReportIfPropertyHasInvalidCssColor(context, property, { + loc: property.loc, + messageId: 'noCssColorSpecific', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: property.key.name, + }, + }); + }); - break; - } - } + return; } /** - * @example This is an example + * @example + * + * const codeStyle = { color: '#dd4040' }; + * + * This is an example */ if ( node.value && node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'ObjectExpression' + node.value.expression.type === 'Identifier' ) { - const declarationTemplateNode = node.value.expression.properties; + const styleVariableName = node.value.expression.name; - if (!declarationTemplateNode.length) { + const styleVariableDeclaration = context.sourceCode + .getScope(node.value.expression) + .variables.find((variable) => variable.name === styleVariableName); + + if (!styleVariableDeclaration) { return; } - for (let i = 0; i < declarationTemplateNode.length; i++) { - const property = declarationTemplateNode[i]; + // assuming there's only one definition of the variable + ( + styleVariableDeclaration.defs[0].node as TSESTree.VariableDeclarator + ).init?.properties.forEach((property) => { + raiseReportIfPropertyHasInvalidCssColor(context, property, { + loc: node.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + property: property.key.name, + variableName: styleVariableName, + line: property.loc.start.line, + }, + }); + }); - if (raiseReportIfPropertyHasCssColor(context, property)) { - break; - } - } + return; } } @@ -159,22 +175,24 @@ export const NoCssColor: Rule.RuleModule = { node.value.type === 'JSXExpressionContainer' && node.value.expression.type === 'ObjectExpression' ) { - const declarationTemplateNode = node.value.expression.properties; - - if (!declarationTemplateNode.length) { - return; - } - - for (let i = 0; i < declarationTemplateNode.length; i++) { - const property = declarationTemplateNode[i]; + const declarationPropertiesNode = node.value.expression.properties; + + declarationPropertiesNode?.forEach((property) => { + raiseReportIfPropertyHasInvalidCssColor(context, property, { + loc: property.loc, + messageId: 'noCssColorSpecific', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: property.key.name, + }, + }); + }); - if (raiseReportIfPropertyHasCssColor(context, property)) { - break; - } - } + return; } /** + * @description check if css prop is a tagged template literal from emotion * @example This is an example */ if ( @@ -196,6 +214,8 @@ export const NoCssColor: Rule.RuleModule = { break; } } + + return; } /** @@ -231,13 +251,18 @@ export const NoCssColor: Rule.RuleModule = { return; } - for (let i = 0; i < declarationPropertiesNode.length; i++) { - const property = declarationPropertiesNode[i]; + declarationPropertiesNode.forEach((property) => { + raiseReportIfPropertyHasInvalidCssColor(context, property, { + loc: property.loc, + messageId: 'noCssColorSpecific', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: property.key.name, + }, + }); + }); - if (raiseReportIfPropertyHasCssColor(context, property)) { - break; - } - } + return; } } }, From 02a896685e66ef4590b77cdf941e4f73a135cc8f Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 26 Nov 2024 18:04:43 +0100 Subject: [PATCH 13/23] modify implementation to account for spread properties, and mark any string literal as a valid report --- .../src/rules/no_css_color.test.ts | 17 ++++- .../src/rules/no_css_color.ts | 76 +++++++++++++++---- 2 files changed, 77 insertions(+), 16 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 7ee54c511cec4..c7c0232500bc2 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -54,7 +54,7 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColorSpecific' }], }, { - name: 'Raises an error when a CSS color is used in a JSX style attribute with template literals', + name: 'Raises an error when a CSS color is used in a variable that is passed to style prop of a JSX element', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; @@ -67,6 +67,21 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], }, + { + name: 'Raises an error when a CSS color is used in a variable that is spread into another variable that is passed to style prop of a JSX element', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + const baseStyle = { background: 'rgb(255, 255, 255)' }; + const codeStyle = { margin: '5px', ...baseStyle }; + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], + }, { name: 'Raises an error when a CSS color is used for the background property in a JSX style attribute', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 9beeaf6a8e28f..a6f014a2a0fe2 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -7,12 +7,11 @@ * License v3.0 only", or the "Server Side Public License, v 1". */ -// import Color from 'color'; import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; /** - * @description Regex to match css color values, + * @description Regex to match css color values when used in a template string declarations, * see {@link https://developer.mozilla.org/en-US/docs/Web/CSS/color_value} for definitions of valid css color values */ const cssColorRegex = /(#|rgb|hsl|hwb|lab|lch|oklab).*/; @@ -20,7 +19,7 @@ const cssColorRegex = /(#|rgb|hsl|hwb|lab|lch|oklab).*/; /** * @description List of css properties that can that can apply color to html box element and text node */ -const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', 'border']; +const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', 'borderColor']; /** * @description Builds off the existing color definition regex to match css declarations that can apply color to @@ -30,6 +29,7 @@ const htmlElementColorDeclarationRegex = RegExp( String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` ); +// in trying to keep this rule simple, if a string is used to define a color, we mark it as invalid for this particular use case const raiseReportIfPropertyHasInvalidCssColor = ( context: Rule.RuleContext, propertyNode: TSESTree.ObjectLiteralElement, @@ -37,14 +37,12 @@ const raiseReportIfPropertyHasInvalidCssColor = ( ) => { let didReport: boolean; - // checks if property value is a css color value for instances where style declaration is computed from an object if ( (didReport = Boolean( propertyNode.type === 'Property' && propertyNode.key.type === 'Identifier' && propertyNode.value.type === 'Literal' && - propertiesSupportingCssColor.indexOf(propertyNode.key.name) > -1 && - cssColorRegex.test(String(propertyNode.value.value) ?? '') + propertiesSupportingCssColor.indexOf(propertyNode.key.name) > -1 )) ) { context.report(messageToReport); @@ -53,6 +51,49 @@ const raiseReportIfPropertyHasInvalidCssColor = ( return didReport; }; +/** + * + * @description style object declaration have a depth of 1, this function handles the properties of the object + */ +const handleObjectProperties = ( + context: Rule.RuleContext, + propertyParentNode: TSESTree.JSXAttribute, + property: TSESTree.ObjectLiteralElement, + reportMessage: Rule.ReportDescriptor +) => { + if (property.type === 'Property') { + raiseReportIfPropertyHasInvalidCssColor(context, property, reportMessage); + } else if (property.type === 'SpreadElement') { + const spreadElementIdentifierName = (property.argument as TSESTree.Identifier).name; + + const spreadElementDeclaration = context.sourceCode + // @ts-expect-error + .getScope(propertyParentNode!.value.expression!) + .variables.find((variable) => variable.name === spreadElementIdentifierName); + + if (!spreadElementDeclaration) { + return; + } + + reportMessage = { + loc: propertyParentNode.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: String(property.argument.name), + variableName: spreadElementIdentifierName, + line: String(property.loc.start.line), + }, + }; + + (spreadElementDeclaration.defs[0].node.init as TSESTree.ObjectExpression).properties.forEach( + (spreadProperty) => { + handleObjectProperties(context, propertyParentNode, spreadProperty, reportMessage); + } + ); + } +}; + export const NoCssColor: Rule.RuleModule = { meta: { type: 'suggestion', @@ -90,7 +131,7 @@ export const NoCssColor: Rule.RuleModule = { const declarationPropertiesNode = node.value.expression.properties; declarationPropertiesNode?.forEach((property) => { - raiseReportIfPropertyHasInvalidCssColor(context, property, { + handleObjectProperties(context, node, property, { loc: property.loc, messageId: 'noCssColorSpecific', data: { @@ -125,17 +166,22 @@ export const NoCssColor: Rule.RuleModule = { return; } - // assuming there's only one definition of the variable + // assuming there's only one definition of the variable, eslint would catch other occurrences of this ( - styleVariableDeclaration.defs[0].node as TSESTree.VariableDeclarator - ).init?.properties.forEach((property) => { - raiseReportIfPropertyHasInvalidCssColor(context, property, { + styleVariableDeclaration.defs[0].node.init as TSESTree.ObjectExpression + )?.properties.forEach((property) => { + handleObjectProperties(context, node, property, { loc: node.loc, messageId: 'noCSSColorSpecificDeclaredVariable', data: { - property: property.key.name, + property: + property.type === 'SpreadElement' + ? // @ts-expect-error the key name is always present else this code will not execute + String(property.argument.name) + : // @ts-expect-error the key name is always present else this code will not execute + String(property.key.name), variableName: styleVariableName, - line: property.loc.start.line, + line: String(property.loc.start.line), }, }); }); @@ -178,7 +224,7 @@ export const NoCssColor: Rule.RuleModule = { const declarationPropertiesNode = node.value.expression.properties; declarationPropertiesNode?.forEach((property) => { - raiseReportIfPropertyHasInvalidCssColor(context, property, { + handleObjectProperties(context, node, property, { loc: property.loc, messageId: 'noCssColorSpecific', data: { @@ -252,7 +298,7 @@ export const NoCssColor: Rule.RuleModule = { } declarationPropertiesNode.forEach((property) => { - raiseReportIfPropertyHasInvalidCssColor(context, property, { + handleObjectProperties(context, node, property, { loc: property.loc, messageId: 'noCssColorSpecific', data: { From 94205ea9d7d289114af676b526b3bfa070d0ec94 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Thu, 28 Nov 2024 18:09:50 +0100 Subject: [PATCH 14/23] start on consideration for evaluating variables defined referenced as value for style properties of interest --- .../src/rules/no_css_color.test.ts | 16 ++++++- .../src/rules/no_css_color.ts | 42 +++++++++++++++---- 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index c7c0232500bc2..26b7f64c59e87 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -54,7 +54,21 @@ const invalid: RuleTester.InvalidTestCase[] = [ errors: [{ messageId: 'noCssColorSpecific' }], }, { - name: 'Raises an error when a CSS color is used in a variable that is passed to style prop of a JSX element', + name: 'Raises an error when a CSS color references a string variable that is passed to style prop of a JSX element', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + const codeColor = '#dd4040'; + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], + }, + { + name: 'Raises an error when a CSS color is used in an object variable that is passed to style prop of a JSX element', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index a6f014a2a0fe2..88b02a4815971 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -29,23 +29,49 @@ const htmlElementColorDeclarationRegex = RegExp( String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` ); -// in trying to keep this rule simple, if a string is used to define a color, we mark it as invalid for this particular use case const raiseReportIfPropertyHasInvalidCssColor = ( context: Rule.RuleContext, - propertyNode: TSESTree.ObjectLiteralElement, + propertyNode: TSESTree.Property, messageToReport: Rule.ReportDescriptor ) => { let didReport: boolean; if ( - (didReport = Boolean( - propertyNode.type === 'Property' && - propertyNode.key.type === 'Identifier' && - propertyNode.value.type === 'Literal' && - propertiesSupportingCssColor.indexOf(propertyNode.key.name) > -1 - )) + propertyNode.key.type === 'Identifier' && + propertiesSupportingCssColor.indexOf(propertyNode.key.name) < 0 ) { + return; + } + + if ((didReport = Boolean(propertyNode.value.type === 'Literal'))) { + // in trying to keep this rule simple, if a string is used to define a color we simply mark it as invalid context.report(messageToReport); + } else if (propertyNode.value.type === 'Identifier') { + const identifierDeclaration = context.sourceCode + // @ts-expect-error + .getScope(propertyNode) + .variables.find( + (variable) => variable.name === (propertyNode.value as TSESTree.Identifier).name! + ); + + if (identifierDeclaration?.defs[0].node.init.type === 'Literal') { + context.report({ + loc: propertyNode.value.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: String(propertyNode.key.name), + line: String(propertyNode.value.loc.start.line), + variableName: propertyNode.value.name, + }, + }); + + didReport = true; + } + + return; + } else if ((didReport = Boolean(propertyNode.value.type === 'MemberExpression'))) { + // TODO: handle member expression ie. style.color.red } return didReport; From ede7c4f470f4903311cd2bbe7c6bf709dbd6c530 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Fri, 29 Nov 2024 11:57:11 +0100 Subject: [PATCH 15/23] handle cases for member expression values --- .../src/rules/no_css_color.test.ts | 15 ++++++ .../src/rules/no_css_color.ts | 51 +++++++++++++++++-- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 26b7f64c59e87..7f10839bdbab0 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -81,6 +81,21 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], }, + { + name: 'Raises an error when an object property that is a literal CSS color is used for the background property in a JSX style attribute', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + const baseStyle = { background: 'rgb(255, 255, 255)' }; + + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], + }, { name: 'Raises an error when a CSS color is used in a variable that is spread into another variable that is passed to style prop of a JSX element', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 88b02a4815971..b15068456bbcd 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -29,6 +29,14 @@ const htmlElementColorDeclarationRegex = RegExp( String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` ); +const resolveMemberExpressionRoot = (node: TSESTree.MemberExpression): TSESTree.Identifier => { + if (node.object.type === 'MemberExpression') { + return resolveMemberExpressionRoot(node.object); + } + + return node.object as TSESTree.Identifier; +}; + const raiseReportIfPropertyHasInvalidCssColor = ( context: Rule.RuleContext, propertyNode: TSESTree.Property, @@ -70,11 +78,46 @@ const raiseReportIfPropertyHasInvalidCssColor = ( } return; - } else if ((didReport = Boolean(propertyNode.value.type === 'MemberExpression'))) { - // TODO: handle member expression ie. style.color.red - } + } else if (propertyNode.value.type === 'MemberExpression') { + // @ts-expect-error we ignore the case where this node could be a private identifier + const MemberExpressionLeafName = propertyNode.value.property.name; + const memberExpressionRootName = resolveMemberExpressionRoot(propertyNode.value).name; + + const expressionRootDeclaration = context.sourceCode + // @ts-expect-error + .getScope(propertyNode) + .variables.find((variable) => variable.name === memberExpressionRootName); + + const expressionRootDeclarationInit = expressionRootDeclaration?.defs[0].node.init; - return didReport; + if (expressionRootDeclarationInit?.type === 'ObjectExpression') { + (expressionRootDeclarationInit as TSESTree.ObjectExpression).properties.forEach( + (property) => { + // This is a naive approach expecting the value to be at depth 1, we should actually be traversing the object to the same depth as the expression + if ( + property.type === 'Property' && + property.key.type === 'Identifier' && + property.key?.name === MemberExpressionLeafName + ) { + raiseReportIfPropertyHasInvalidCssColor(context, property, { + loc: propertyNode.value.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: String(propertyNode.key.name), + line: String(propertyNode.value.loc.start.line), + variableName: memberExpressionRootName, + }, + }); + } + } + ); + } else if (expressionRootDeclarationInit?.type === 'CallExpression') { + // TODO: if this object was returned from invoking a function the best we can do ids probably validate that the method invoked is one that returns an euitheme object + } + + return didReport; + } }; /** From 4728e0343fc1aac75acce0095fe51b7bdbdce9b1 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 2 Dec 2024 10:08:22 +0100 Subject: [PATCH 16/23] account for css tagged template literal usage to specify component className --- .../src/rules/no_css_color.test.ts | 13 +++++++ .../src/rules/no_css_color.ts | 37 ++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 7f10839bdbab0..ee9d8e8a8abe6 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -176,6 +176,19 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColorSpecific' }], }, + { + name: 'Raises an error when a CSS color for the color property is used in a JSX className attribute for EuiComponents with the css template function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, ]; const valid: RuleTester.ValidTestCase[] = []; diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index b15068456bbcd..402b4ebd7010a 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -184,7 +184,13 @@ export const NoCssColor: Rule.RuleModule = { create(context) { return { JSXAttribute(node: TSESTree.JSXAttribute) { - if (!(node.name.name === 'style' || node.name.name === 'css')) { + if ( + !( + node.name.name === 'style' || + node.name.name === 'className' || + node.name.name === 'css' + ) + ) { return; } @@ -380,6 +386,35 @@ export const NoCssColor: Rule.RuleModule = { return; } } + + if (node.name.name === 'className') { + /** + * @description check if css prop is a tagged template literal from emotion + * @example This is an example + */ + if ( + node.value && + node.value.type === 'JSXExpressionContainer' && + node.value.expression.type === 'TaggedTemplateExpression' && + node.value.expression.tag.type === 'Identifier' && + node.value.expression.tag.name === 'css' + ) { + for (let i = 0; i < node.value.expression.quasi.quasis.length; i++) { + const declarationTemplateNode = node.value.expression.quasi.quasis[i]; + + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { + context.report({ + loc: declarationTemplateNode.loc, + messageId: 'noCssColor', + }); + + break; + } + } + + return; + } + } }, }; }, From c7357c88e8e19cef2b88752c97882e9f713f55a8 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 2 Dec 2024 10:46:53 +0100 Subject: [PATCH 17/23] improve README --- packages/kbn-eslint-plugin-css/README.mdx | 111 +++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/packages/kbn-eslint-plugin-css/README.mdx b/packages/kbn-eslint-plugin-css/README.mdx index 09439d56aec00..1e121657bc571 100644 --- a/packages/kbn-eslint-plugin-css/README.mdx +++ b/packages/kbn-eslint-plugin-css/README.mdx @@ -17,4 +17,113 @@ If a rule does not behave as you expect or you have an idea of how these rules c # Rules -... \ No newline at end of file +## `@kbn/css/no_css_color` + +This rule warns engineers to not use literal css color in the codebase, particularly for CSS properties that apply color to +either the html element or text nodes, but rather urge users to defer to using the color tokens provided by EUI. + +This rule kicks in on the following JSXAttributes; `style`, `className` and `css` and supports various approaches to providing styling declarations. + +### Example + +The following code: + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + return ( + You know, for search + ) +} +``` + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + + const style = { + color: 'red' + } + + return ( + You know, for search + ) +} +``` + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + const colorValue = '#dd4040'; + + return ( + You know, for search + ) +} +``` + +will all raise an eslint report with an appropriate message of severity that matches the configuration of the rule, further more all the examples above +will also match for when the attribute in question is `css`. The `css` attribute will also raise a report the following cases below; + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { css } from '@emotion/css'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + return ( + You know, for search + ) +} +``` + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + return ( + ({ color: '#dd4040' })}>You know, for search + ) +} +``` + +A special case is also covered for the `className` attribute, where the rule will also raise a report for the following case below; + + +``` +// Filename: /x-pack/plugins/observability_solution/observability/public/my_component.tsx + +import React from 'react'; +import { css } from '@emotion/css'; +import { EuiText } from '@elastic/eui'; + +function MyComponent() { + return ( + You know, for search + ) +} +``` + +it's worth pointing out that although the examples provided are specific to EUI components, this rule applies to all JSX elements. + +## `@kbn/css/prefer_css_attributes_for_eui_components` + +This rule warns engineers to use the `css` attribute for EUI components instead of the `style` attribute. + From ce7f27dc91dc62d381ff46a2723bc841d2c30ba2 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Mon, 2 Dec 2024 11:21:11 +0100 Subject: [PATCH 18/23] consolidate logic where possible --- .../src/rules/no_css_color.ts | 189 ++++++++---------- 1 file changed, 83 insertions(+), 106 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 402b4ebd7010a..b0dddfc38eae0 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -194,86 +194,90 @@ export const NoCssColor: Rule.RuleModule = { return; } - if (node.name.name === 'style') { - /** - * @example This is an example - */ - if ( - node.value && - node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'ObjectExpression' - ) { - const declarationPropertiesNode = node.value.expression.properties; - - declarationPropertiesNode?.forEach((property) => { - handleObjectProperties(context, node, property, { - loc: property.loc, - messageId: 'noCssColorSpecific', - data: { - // @ts-expect-error the key name is always present else this code will not execute - property: property.key.name, - }, - }); + /** + * @example + * const codeStyle = { color: '#dd4040' }; + * This is an example + * + * @example + * const codeStyle = { color: '#dd4040' }; + * This is an example + */ + if ( + node.name.name !== 'className' && + node.value?.type === 'JSXExpressionContainer' && + node.value.expression.type === 'Identifier' + ) { + const styleVariableName = node.value.expression.name; + + const styleVariableDeclaration = context.sourceCode + .getScope(node.value.expression) + .variables.find((variable) => variable.name === styleVariableName); + + // we assume there's only one definition of the variable + ( + styleVariableDeclaration?.defs[0].node.init as TSESTree.ObjectExpression + )?.properties.forEach((property) => { + handleObjectProperties(context, node, property, { + loc: node.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + property: + property.type === 'SpreadElement' + ? // @ts-expect-error the key name is always present else this code will not execute + String(property.argument.name) + : // @ts-expect-error the key name is always present else this code will not execute + String(property.key.name), + variableName: styleVariableName, + line: String(property.loc.start.line), + }, }); + }); - return; - } - - /** - * @example - * - * const codeStyle = { color: '#dd4040' }; - * - * This is an example - */ - if ( - node.value && - node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'Identifier' - ) { - const styleVariableName = node.value.expression.name; - - const styleVariableDeclaration = context.sourceCode - .getScope(node.value.expression) - .variables.find((variable) => variable.name === styleVariableName); + return; + } - if (!styleVariableDeclaration) { - return; - } + /** + * @example + * This is an example + * + * @example + * This is an example + * + * @example + * const styleRules = { color: '#dd4040' }; + * This is an example + * + * @example + * const styleRules = { color: '#dd4040' }; + * This is an example + */ + if ( + node.value?.type === 'JSXExpressionContainer' && + node.value.expression.type === 'ObjectExpression' + ) { + const declarationPropertiesNode = node.value.expression.properties; - // assuming there's only one definition of the variable, eslint would catch other occurrences of this - ( - styleVariableDeclaration.defs[0].node.init as TSESTree.ObjectExpression - )?.properties.forEach((property) => { - handleObjectProperties(context, node, property, { - loc: node.loc, - messageId: 'noCSSColorSpecificDeclaredVariable', - data: { - property: - property.type === 'SpreadElement' - ? // @ts-expect-error the key name is always present else this code will not execute - String(property.argument.name) - : // @ts-expect-error the key name is always present else this code will not execute - String(property.key.name), - variableName: styleVariableName, - line: String(property.loc.start.line), - }, - }); + declarationPropertiesNode?.forEach((property) => { + handleObjectProperties(context, node, property, { + loc: property.loc, + messageId: 'noCssColorSpecific', + data: { + // @ts-expect-error the key name is always present else this code will not execute + property: property.key.name, + }, }); + }); - return; - } + return; } - if (node.name.name === 'css') { + if (node.name.name === 'css' && node.value?.type === 'JSXExpressionContainer') { /** - * @example This is an example + * @example + * This is an example */ - if ( - node.value && - node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'TemplateLiteral' - ) { + if (node.value.expression.type === 'TemplateLiteral') { for (let i = 0; i < node.value.expression.quasis.length; i++) { const declarationTemplateNode = node.value.expression.quasis[i]; @@ -288,37 +292,12 @@ export const NoCssColor: Rule.RuleModule = { } } - /** - * @example This is an example - */ - if ( - node.value && - node.value.type === 'JSXExpressionContainer' && - node.value.expression.type === 'ObjectExpression' - ) { - const declarationPropertiesNode = node.value.expression.properties; - - declarationPropertiesNode?.forEach((property) => { - handleObjectProperties(context, node, property, { - loc: property.loc, - messageId: 'noCssColorSpecific', - data: { - // @ts-expect-error the key name is always present else this code will not execute - property: property.key.name, - }, - }); - }); - - return; - } - /** * @description check if css prop is a tagged template literal from emotion - * @example This is an example + * @example + * This is an example */ if ( - node.value && - node.value.type === 'JSXExpressionContainer' && node.value.expression.type === 'TaggedTemplateExpression' && node.value.expression.tag.type === 'Identifier' && node.value.expression.tag.name === 'css' @@ -340,13 +319,12 @@ export const NoCssColor: Rule.RuleModule = { } /** - * @example ({ color: '#dd4040' })}>This is an example + * @example + * ({ color: '#dd4040' })}>This is an example */ if ( - node.value && - node.value.type === 'JSXExpressionContainer' && - (node.value.expression.type === 'FunctionExpression' || - node.value.expression.type === 'ArrowFunctionExpression') + node.value.expression.type === 'FunctionExpression' || + node.value.expression.type === 'ArrowFunctionExpression' ) { let declarationPropertiesNode: TSESTree.Property[] = []; @@ -387,14 +365,13 @@ export const NoCssColor: Rule.RuleModule = { } } - if (node.name.name === 'className') { + if (node.name.name === 'className' && node.value?.type === 'JSXExpressionContainer') { /** * @description check if css prop is a tagged template literal from emotion - * @example This is an example + * @example + * This is an example */ if ( - node.value && - node.value.type === 'JSXExpressionContainer' && node.value.expression.type === 'TaggedTemplateExpression' && node.value.expression.tag.type === 'Identifier' && node.value.expression.tag.name === 'css' From fb27e6d067488c717b14097e34f6cb07c2611066 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Wed, 4 Dec 2024 23:37:26 +0100 Subject: [PATCH 19/23] simplify logic for detecting literal values for tagged template functions --- .../src/rules/no_css_color.test.ts | 16 ++- .../src/rules/no_css_color.ts | 134 ++++++------------ 2 files changed, 60 insertions(+), 90 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index ee9d8e8a8abe6..59d4e26acbc09 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -142,10 +142,11 @@ const invalid: RuleTester.InvalidTestCase[] = [ filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; + import { css } from '@emotion/css'; function TestComponent() { return ( - This is a test + This is a test ) }`, errors: [{ messageId: 'noCssColor' }], @@ -176,15 +177,26 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColorSpecific' }], }, + { + name: 'Raises an error when a CSS color for the color property is used in with the tagged template css function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import { css } from '@emotion/css'; + + const codeColor = css\` color: #dd4040; \`; + `, + errors: [{ messageId: 'noCssColor' }], + }, { name: 'Raises an error when a CSS color for the color property is used in a JSX className attribute for EuiComponents with the css template function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', code: ` import React from 'react'; + import { css } from '@emotion/css'; function TestComponent() { return ( - This is a test + This is a test ) }`, errors: [{ messageId: 'noCssColor' }], diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index b0dddfc38eae0..92f146cb2e820 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -10,12 +10,6 @@ import type { Rule } from 'eslint'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; -/** - * @description Regex to match css color values when used in a template string declarations, - * see {@link https://developer.mozilla.org/en-US/docs/Web/CSS/color_value} for definitions of valid css color values - */ -const cssColorRegex = /(#|rgb|hsl|hwb|lab|lch|oklab).*/; - /** * @description List of css properties that can that can apply color to html box element and text node */ @@ -26,7 +20,7 @@ const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', * html elements and text nodes for string declarations */ const htmlElementColorDeclarationRegex = RegExp( - String.raw`(${propertiesSupportingCssColor.join('|')})\:\s?(\'|\")?${cssColorRegex.source}` + String.raw`(${propertiesSupportingCssColor.join('|')}):\s?.*;` ); const resolveMemberExpressionRoot = (node: TSESTree.MemberExpression): TSESTree.Identifier => { @@ -113,7 +107,7 @@ const raiseReportIfPropertyHasInvalidCssColor = ( } ); } else if (expressionRootDeclarationInit?.type === 'CallExpression') { - // TODO: if this object was returned from invoking a function the best we can do ids probably validate that the method invoked is one that returns an euitheme object + // TODO: if this object was returned from invoking a function the best we can do is probably validate that the method invoked is one that returns an euitheme object } return didReport; @@ -183,18 +177,33 @@ export const NoCssColor: Rule.RuleModule = { }, create(context) { return { + // accounts for instances where declarations are created using the template tagged css function + TaggedTemplateExpression(node) { + if (node.tag.type === 'Identifier' && node.tag.name !== 'css') { + return; + } + + for (let i = 0; i < node.quasi.quasis.length; i++) { + const declarationTemplateNode = node.quasi.quasis[i]; + + if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + + break; + } + } + }, JSXAttribute(node: TSESTree.JSXAttribute) { - if ( - !( - node.name.name === 'style' || - node.name.name === 'className' || - node.name.name === 'css' - ) - ) { + if (!(node.name.name === 'style' || node.name.name === 'css')) { return; } /** + * @description Accounts for instances where a variable is used to define a style object + * * @example * const codeStyle = { color: '#dd4040' }; * This is an example @@ -204,40 +213,43 @@ export const NoCssColor: Rule.RuleModule = { * This is an example */ if ( - node.name.name !== 'className' && node.value?.type === 'JSXExpressionContainer' && node.value.expression.type === 'Identifier' ) { const styleVariableName = node.value.expression.name; - const styleVariableDeclaration = context.sourceCode + const variableDeclarationMatches = context.sourceCode .getScope(node.value.expression) .variables.find((variable) => variable.name === styleVariableName); // we assume there's only one definition of the variable - ( - styleVariableDeclaration?.defs[0].node.init as TSESTree.ObjectExpression - )?.properties.forEach((property) => { - handleObjectProperties(context, node, property, { - loc: node.loc, - messageId: 'noCSSColorSpecificDeclaredVariable', - data: { - property: - property.type === 'SpreadElement' - ? // @ts-expect-error the key name is always present else this code will not execute - String(property.argument.name) - : // @ts-expect-error the key name is always present else this code will not execute - String(property.key.name), - variableName: styleVariableName, - line: String(property.loc.start.line), - }, + const variableInitializationNode = variableDeclarationMatches?.defs?.[0]?.node?.init; + + if (variableInitializationNode.type === 'ObjectExpression') { + // @ts-ignore + variableInitializationNode.properties.forEach((property) => { + handleObjectProperties(context, node, property, { + loc: node.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + property: + property.type === 'SpreadElement' + ? String(property.argument.name) + : String(property.key.name), + variableName: styleVariableName, + line: String(property.loc.start.line), + }, + }); }); - }); + } return; } /** + * + * @description Accounts for instances where a style object is inlined in the JSX attribute + * * @example * This is an example * @@ -292,32 +304,6 @@ export const NoCssColor: Rule.RuleModule = { } } - /** - * @description check if css prop is a tagged template literal from emotion - * @example - * This is an example - */ - if ( - node.value.expression.type === 'TaggedTemplateExpression' && - node.value.expression.tag.type === 'Identifier' && - node.value.expression.tag.name === 'css' - ) { - for (let i = 0; i < node.value.expression.quasi.quasis.length; i++) { - const declarationTemplateNode = node.value.expression.quasi.quasis[i]; - - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - loc: declarationTemplateNode.loc, - messageId: 'noCssColor', - }); - - break; - } - } - - return; - } - /** * @example * ({ color: '#dd4040' })}>This is an example @@ -364,34 +350,6 @@ export const NoCssColor: Rule.RuleModule = { return; } } - - if (node.name.name === 'className' && node.value?.type === 'JSXExpressionContainer') { - /** - * @description check if css prop is a tagged template literal from emotion - * @example - * This is an example - */ - if ( - node.value.expression.type === 'TaggedTemplateExpression' && - node.value.expression.tag.type === 'Identifier' && - node.value.expression.tag.name === 'css' - ) { - for (let i = 0; i < node.value.expression.quasi.quasis.length; i++) { - const declarationTemplateNode = node.value.expression.quasi.quasis[i]; - - if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - loc: declarationTemplateNode.loc, - messageId: 'noCssColor', - }); - - break; - } - } - - return; - } - } }, }; }, From f94333050baa90af9dc36d667e1273a3b95f3490 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Thu, 5 Dec 2024 13:10:54 +0100 Subject: [PATCH 20/23] account for variables defined outside the scope of the component that uses the declaration --- .../src/rules/no_css_color.test.ts | 16 ++++++++++++++++ .../src/rules/no_css_color.ts | 15 ++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index 59d4e26acbc09..d95160083c117 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -151,6 +151,22 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColor' }], }, + { + name: 'Raises an error when a CSS color for the color property is used in a JSX className attribute for EuiComponents with the css template function defined outside the scope of the component', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + import { css } from '@emotion/css'; + + const codeCss = css\` color: #dd4040; \` + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCssColor' }], + }, { name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with an arrow function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 92f146cb2e820..5462114dad9c2 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -218,9 +218,18 @@ export const NoCssColor: Rule.RuleModule = { ) { const styleVariableName = node.value.expression.name; - const variableDeclarationMatches = context.sourceCode - .getScope(node.value.expression) - .variables.find((variable) => variable.name === styleVariableName); + const nodeScope = context.sourceCode.getScope(node.value.expression); + + let variableDeclarationMatches = nodeScope.variables.find( + (variable) => variable.name === styleVariableName + ); + + if (!variableDeclarationMatches) { + // identifier was probably not declared in the current scope, hence we'll give it another try to find it in the parent scope + variableDeclarationMatches = nodeScope.upper?.variables.find( + (variable) => variable.name === styleVariableName + ); + } // we assume there's only one definition of the variable const variableInitializationNode = variableDeclarationMatches?.defs?.[0]?.node?.init; From 0e4d9db0529fa126b61219e56dfa044823cb2686 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Thu, 5 Dec 2024 19:02:13 +0100 Subject: [PATCH 21/23] change implementation for discovering identifiers to handle even more corner cases --- .../src/rules/no_css_color.test.ts | 38 ++++++--- .../src/rules/no_css_color.ts | 79 ++++++++++++------- 2 files changed, 78 insertions(+), 39 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts index d95160083c117..e1f683b09814f 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.test.ts @@ -137,6 +137,16 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColorSpecific' }], }, + { + name: 'Raises an error when a CSS color for the color property is used in with the tagged template css function', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import { css } from '@emotion/css'; + + const codeColor = css\` color: #dd4040; \`; + `, + errors: [{ messageId: 'noCssColor' }], + }, { name: 'Raises an error when a CSS color for the color property is used in a JSX css attribute for EuiComponents with the css template function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', @@ -158,6 +168,24 @@ const invalid: RuleTester.InvalidTestCase[] = [ import React from 'react'; import { css } from '@emotion/css'; + const codeCss = css({ + color: '#dd4040', + }) + + function TestComponent() { + return ( + This is a test + ) + }`, + errors: [{ messageId: 'noCSSColorSpecificDeclaredVariable' }], + }, + { + name: 'Raises an error when a CSS color for the color property is used in a JSX className attribute for EuiComponents with the css template function defined outside the scope of the component', + filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', + code: ` + import React from 'react'; + import { css } from '@emotion/css'; + const codeCss = css\` color: #dd4040; \` function TestComponent() { @@ -193,16 +221,6 @@ const invalid: RuleTester.InvalidTestCase[] = [ }`, errors: [{ messageId: 'noCssColorSpecific' }], }, - { - name: 'Raises an error when a CSS color for the color property is used in with the tagged template css function', - filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', - code: ` - import { css } from '@emotion/css'; - - const codeColor = css\` color: #dd4040; \`; - `, - errors: [{ messageId: 'noCssColor' }], - }, { name: 'Raises an error when a CSS color for the color property is used in a JSX className attribute for EuiComponents with the css template function', filename: '/x-pack/plugins/observability_solution/observability/public/test_component.tsx', diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 5462114dad9c2..d77908d5b28d1 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -211,6 +211,10 @@ export const NoCssColor: Rule.RuleModule = { * @example * const codeStyle = { color: '#dd4040' }; * This is an example + * + * @example + * const codeStyle = css({ color: '#dd4040' }); + * This is an example */ if ( node.value?.type === 'JSXExpressionContainer' && @@ -220,36 +224,53 @@ export const NoCssColor: Rule.RuleModule = { const nodeScope = context.sourceCode.getScope(node.value.expression); - let variableDeclarationMatches = nodeScope.variables.find( - (variable) => variable.name === styleVariableName - ); - - if (!variableDeclarationMatches) { - // identifier was probably not declared in the current scope, hence we'll give it another try to find it in the parent scope - variableDeclarationMatches = nodeScope.upper?.variables.find( - (variable) => variable.name === styleVariableName - ); - } - - // we assume there's only one definition of the variable - const variableInitializationNode = variableDeclarationMatches?.defs?.[0]?.node?.init; - - if (variableInitializationNode.type === 'ObjectExpression') { - // @ts-ignore - variableInitializationNode.properties.forEach((property) => { - handleObjectProperties(context, node, property, { - loc: node.loc, - messageId: 'noCSSColorSpecificDeclaredVariable', - data: { - property: - property.type === 'SpreadElement' - ? String(property.argument.name) - : String(property.key.name), - variableName: styleVariableName, - line: String(property.loc.start.line), - }, + const variableDeclarationMatches = nodeScope.references.find( + (ref) => ref.identifier.name === styleVariableName + )?.resolved; + + let variableInitializationNode; + + if ((variableInitializationNode = variableDeclarationMatches?.defs?.[0]?.node?.init)) { + if (variableInitializationNode.type === 'ObjectExpression') { + // @ts-ignore + variableInitializationNode.properties.forEach((property) => { + handleObjectProperties(context, node, property, { + loc: property.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + property: + property.type === 'SpreadElement' + ? String(property.argument.name) + : String(property.key.name), + variableName: styleVariableName, + line: String(property.loc.start.line), + }, + }); }); - }); + } else if ( + variableInitializationNode.type === 'CallExpression' && + variableInitializationNode.callee.name === 'css' + ) { + const cssFunctionArgument = variableInitializationNode.arguments[0]; + + if (cssFunctionArgument.type === 'ObjectExpression') { + // @ts-ignore + cssFunctionArgument.properties.forEach((property) => { + handleObjectProperties(context, node, property, { + loc: node.loc, + messageId: 'noCSSColorSpecificDeclaredVariable', + data: { + property: + property.type === 'SpreadElement' + ? String(property.argument.name) + : String(property.key.name), + variableName: styleVariableName, + line: String(property.loc.start.line), + }, + }); + }); + } + } } return; From 6438572dea008e1efea8e2de1d9ee11ffd0415c4 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Tue, 10 Dec 2024 21:25:19 +0100 Subject: [PATCH 22/23] improve implementation to prevent false positives --- package.json | 2 + .../src/rules/no_css_color.ts | 102 ++++++++++++++---- yarn.lock | 17 +++ 3 files changed, 100 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index 84c8a1028c8ef..8b0685adb804e 100644 --- a/package.json +++ b/package.json @@ -1566,6 +1566,7 @@ "@types/classnames": "^2.2.9", "@types/cli-progress": "^3.11.5", "@types/color": "^3.0.3", + "@types/cssstyle": "^2.2.4", "@types/cytoscape": "^3.14.0", "@types/d3": "^3.5.43", "@types/d3-array": "^2.12.1", @@ -1716,6 +1717,7 @@ "css-loader": "^3.4.2", "cssnano": "^5.1.12", "cssnano-preset-default": "^5.2.12", + "cssstyle": "^4.1.0", "csstype": "^3.0.2", "cypress": "13.15.2", "cypress-axe": "^1.5.0", diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index d77908d5b28d1..6e7ad791bfcaa 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -8,21 +8,45 @@ */ import type { Rule } from 'eslint'; +import { CSSStyleDeclaration } from 'cssstyle'; import type { TSESTree } from '@typescript-eslint/typescript-estree'; /** - * @description List of css properties that can that can apply color to html box element and text node + * @description List of superset css properties that can apply color to html box element elements and text nodes, leveraging the + * css style package allows us to directly singly check for these properties even if the actual declaration was written using the shorthand form */ -const propertiesSupportingCssColor = ['color', 'background', 'backgroundColor', 'borderColor']; +const propertiesSupportingCssColor = ['color', 'background', 'border']; /** - * @description Builds off the existing color definition regex to match css declarations that can apply color to + * @description Builds off the existing color definition to match css declarations that can apply color to * html elements and text nodes for string declarations */ const htmlElementColorDeclarationRegex = RegExp( - String.raw`(${propertiesSupportingCssColor.join('|')}):\s?.*;` + String.raw`(${propertiesSupportingCssColor.join('|')})` ); +const checkPropertySpecifiesInvalidCSSColor = ([property, value]: string[]) => { + if (!property || !value) return false; + + const style = new CSSStyleDeclaration(); + + // @ts-ignore the types for this packages specifics an index signature of number, alongside other valid CSS properties + style[property] = value; + + const anchor = propertiesSupportingCssColor.find((resolvedProperty) => + property.includes(resolvedProperty) + ); + + if (!anchor) return false; + + // build the resolved color property to check if the value is a string after parsing the style declaration + const resolvedColorProperty = anchor === 'color' ? 'color' : anchor + 'Color'; + + // in trying to keep this rule simple, it's enough if a string is used to define a color to mark it as invalid + // @ts-ignore the types for this packages specifics an index signature of number, alongside other valid CSS properties + return typeof style[resolvedColorProperty] === 'string'; +}; + const resolveMemberExpressionRoot = (node: TSESTree.MemberExpression): TSESTree.Identifier => { if (node.object.type === 'MemberExpression') { return resolveMemberExpressionRoot(node.object); @@ -36,18 +60,28 @@ const raiseReportIfPropertyHasInvalidCssColor = ( propertyNode: TSESTree.Property, messageToReport: Rule.ReportDescriptor ) => { - let didReport: boolean; + let didReport = false; if ( propertyNode.key.type === 'Identifier' && - propertiesSupportingCssColor.indexOf(propertyNode.key.name) < 0 + !htmlElementColorDeclarationRegex.test(propertyNode.key.name) ) { return; } - if ((didReport = Boolean(propertyNode.value.type === 'Literal'))) { - // in trying to keep this rule simple, if a string is used to define a color we simply mark it as invalid - context.report(messageToReport); + if (propertyNode.value.type === 'Literal') { + if ( + (didReport = checkPropertySpecifiesInvalidCSSColor( + // @ts-expect-error the key name is present in this scenario + propertyNode.key.name, + // @ts-expect-error we already ascertained that the value here is a literal + propertyNode.value.value + )) + ) { + context.report(messageToReport); + } + + return; } else if (propertyNode.value.type === 'Identifier') { const identifierDeclaration = context.sourceCode // @ts-expect-error @@ -56,7 +90,14 @@ const raiseReportIfPropertyHasInvalidCssColor = ( (variable) => variable.name === (propertyNode.value as TSESTree.Identifier).name! ); - if (identifierDeclaration?.defs[0].node.init.type === 'Literal') { + if ( + identifierDeclaration?.defs[0].node.init.type === 'Literal' && + checkPropertySpecifiesInvalidCSSColor([ + // @ts-expect-error the key name is present in this scenario + propertyNode.key.name, + (identifierDeclaration.defs[0].node.init as TSESTree.Literal).value as string, + ]) + ) { context.report({ loc: propertyNode.value.loc, messageId: 'noCSSColorSpecificDeclaredVariable', @@ -179,7 +220,10 @@ export const NoCssColor: Rule.RuleModule = { return { // accounts for instances where declarations are created using the template tagged css function TaggedTemplateExpression(node) { - if (node.tag.type === 'Identifier' && node.tag.name !== 'css') { + if ( + node.tag.type !== 'Identifier' || + (node.tag.type === 'Identifier' && node.tag.name !== 'css') + ) { return; } @@ -187,12 +231,19 @@ export const NoCssColor: Rule.RuleModule = { const declarationTemplateNode = node.quasi.quasis[i]; if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - node: declarationTemplateNode, - messageId: 'noCssColor', - }); + const cssText = declarationTemplateNode.value.raw.replace(/(\{|\}|\\n)/g, '').trim(); - break; + cssText.split(';').forEach((declaration) => { + if ( + declaration.length > 0 && + checkPropertySpecifiesInvalidCSSColor(declaration.split(':')) + ) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + } + }); } } }, @@ -324,12 +375,21 @@ export const NoCssColor: Rule.RuleModule = { const declarationTemplateNode = node.value.expression.quasis[i]; if (htmlElementColorDeclarationRegex.test(declarationTemplateNode.value.raw)) { - context.report({ - node: declarationTemplateNode, - messageId: 'noCssColor', + const cssText = declarationTemplateNode.value.raw + .replace(/(\{|\}|\\n)/g, '') + .trim(); + + cssText.split(';').forEach((declaration) => { + if ( + declaration.length > 0 && + checkPropertySpecifiesInvalidCSSColor(declaration.split(':')) + ) { + context.report({ + node: declarationTemplateNode, + messageId: 'noCssColor', + }); + } }); - - break; } } } diff --git a/yarn.lock b/yarn.lock index 8d4a2b347ccef..2942405cf1943 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11327,6 +11327,11 @@ resolved "https://registry.yarnpkg.com/@types/cookiejar/-/cookiejar-2.1.5.tgz#14a3e83fa641beb169a2dd8422d91c3c345a9a78" integrity sha512-he+DHOWReW0nghN24E1WUqM0efK4kI9oTqDm6XmK8ZPe2djZ90BSNdGnIyCLzCPw7/pogPlGbzI2wHGGmi4O/Q== +"@types/cssstyle@^2.2.4": + version "2.2.4" + resolved "https://registry.yarnpkg.com/@types/cssstyle/-/cssstyle-2.2.4.tgz#3d333ab9f8e6c40183ad1d6ebeebfcb8da2bfe4b" + integrity sha512-FTGMeuHZtLB7hRm+NGvOLZElslR1UkKvZmEmFevOZe/e7Av0nFleka1s8ZwoX+QvbJ2y7r9NDZXIzyqpRWDJXQ== + "@types/cytoscape@^3.14.0": version "3.14.0" resolved "https://registry.yarnpkg.com/@types/cytoscape/-/cytoscape-3.14.0.tgz#346b5430a7a1533784bcf44fcbe6c5255b948d36" @@ -16280,6 +16285,13 @@ cssstyle@^2.3.0: dependencies: cssom "~0.3.6" +cssstyle@^4.1.0: + version "4.1.0" + resolved "https://registry.yarnpkg.com/cssstyle/-/cssstyle-4.1.0.tgz#161faee382af1bafadb6d3867a92a19bcb4aea70" + integrity sha512-h66W1URKpBS5YMI/V8PyXvTMFT8SupJ1IzoIV8IeBC/ji8WVmrO8dGlTi+2dh6whmdk6BiKJLD/ZBkhWbcg6nA== + dependencies: + rrweb-cssom "^0.7.1" + csstype@3.1.2: version "3.1.2" resolved "https://registry.yarnpkg.com/csstype/-/csstype-3.1.2.tgz#1d4bf9d572f11c14031f0436e1c10bc1f571f50b" @@ -28834,6 +28846,11 @@ robust-predicates@^3.0.0: resolved "https://registry.yarnpkg.com/robust-predicates/-/robust-predicates-3.0.1.tgz#ecde075044f7f30118682bd9fb3f123109577f9a" integrity sha512-ndEIpszUHiG4HtDsQLeIuMvRsDnn8c8rYStabochtUeCvfuvNptb5TUbVD68LRAILPX7p9nqQGh4xJgn3EHS/g== +rrweb-cssom@^0.7.1: + version "0.7.1" + resolved "https://registry.yarnpkg.com/rrweb-cssom/-/rrweb-cssom-0.7.1.tgz#c73451a484b86dd7cfb1e0b2898df4b703183e4b" + integrity sha512-TrEMa7JGdVm0UThDJSx7ddw5nVm3UJS9o9CCIZ72B1vSyEZoziDqBYP3XIoi/12lKrJR8rE3jeFHMok2F/Mnsg== + rst-selector-parser@^2.2.3: version "2.2.3" resolved "https://registry.yarnpkg.com/rst-selector-parser/-/rst-selector-parser-2.2.3.tgz#81b230ea2fcc6066c89e3472de794285d9b03d91" From dfea8c25770b2c37b408ca7385aa8b8852c141d2 Mon Sep 17 00:00:00 2001 From: Eyo Okon Eyo Date: Thu, 12 Dec 2024 12:39:18 +0000 Subject: [PATCH 23/23] fix issue with resolving variable declaration for spread elements, alongside slight access issues as well --- .../src/rules/no_css_color.ts | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts index 6e7ad791bfcaa..c453e5edfcd74 100644 --- a/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts +++ b/packages/kbn-eslint-plugin-css/src/rules/no_css_color.ts @@ -55,6 +55,9 @@ const resolveMemberExpressionRoot = (node: TSESTree.MemberExpression): TSESTree. return node.object as TSESTree.Identifier; }; +/** + * @description method to inspect values of interest found on an object + */ const raiseReportIfPropertyHasInvalidCssColor = ( context: Rule.RuleContext, propertyNode: TSESTree.Property, @@ -66,22 +69,19 @@ const raiseReportIfPropertyHasInvalidCssColor = ( propertyNode.key.type === 'Identifier' && !htmlElementColorDeclarationRegex.test(propertyNode.key.name) ) { - return; + return didReport; } if (propertyNode.value.type === 'Literal') { if ( - (didReport = checkPropertySpecifiesInvalidCSSColor( + (didReport = checkPropertySpecifiesInvalidCSSColor([ // @ts-expect-error the key name is present in this scenario propertyNode.key.name, - // @ts-expect-error we already ascertained that the value here is a literal - propertyNode.value.value - )) + propertyNode.value.value, + ])) ) { context.report(messageToReport); } - - return; } else if (propertyNode.value.type === 'Identifier') { const identifierDeclaration = context.sourceCode // @ts-expect-error @@ -91,7 +91,7 @@ const raiseReportIfPropertyHasInvalidCssColor = ( ); if ( - identifierDeclaration?.defs[0].node.init.type === 'Literal' && + identifierDeclaration?.defs[0].node.init?.type === 'Literal' && checkPropertySpecifiesInvalidCSSColor([ // @ts-expect-error the key name is present in this scenario propertyNode.key.name, @@ -111,8 +111,6 @@ const raiseReportIfPropertyHasInvalidCssColor = ( didReport = true; } - - return; } else if (propertyNode.value.type === 'MemberExpression') { // @ts-expect-error we ignore the case where this node could be a private identifier const MemberExpressionLeafName = propertyNode.value.property.name; @@ -150,9 +148,9 @@ const raiseReportIfPropertyHasInvalidCssColor = ( } else if (expressionRootDeclarationInit?.type === 'CallExpression') { // TODO: if this object was returned from invoking a function the best we can do is probably validate that the method invoked is one that returns an euitheme object } - - return didReport; } + + return didReport; }; /** @@ -173,7 +171,7 @@ const handleObjectProperties = ( const spreadElementDeclaration = context.sourceCode // @ts-expect-error .getScope(propertyParentNode!.value.expression!) - .variables.find((variable) => variable.name === spreadElementIdentifierName); + .references.find((ref) => ref.identifier.name === spreadElementIdentifierName)?.resolved; if (!spreadElementDeclaration) { return; @@ -190,11 +188,16 @@ const handleObjectProperties = ( }, }; - (spreadElementDeclaration.defs[0].node.init as TSESTree.ObjectExpression).properties.forEach( - (spreadProperty) => { - handleObjectProperties(context, propertyParentNode, spreadProperty, reportMessage); - } - ); + const spreadElementDeclarationNode = spreadElementDeclaration.defs[0].node.init; + + // evaluate only statically defined declarations, other possibilities like callExpressions in this context complicate things + if (spreadElementDeclarationNode?.type === 'ObjectExpression') { + (spreadElementDeclarationNode as TSESTree.ObjectExpression).properties.forEach( + (spreadProperty) => { + handleObjectProperties(context, propertyParentNode, spreadProperty, reportMessage); + } + ); + } } }; @@ -356,8 +359,12 @@ export const NoCssColor: Rule.RuleModule = { loc: property.loc, messageId: 'noCssColorSpecific', data: { - // @ts-expect-error the key name is always present else this code will not execute - property: property.key.name, + property: + property.type === 'SpreadElement' + ? // @ts-expect-error the key name is always present else this code will not execute + String(property.argument.name) + : // @ts-expect-error the key name is always present else this code will not execute + String(property.key.name), }, }); });