From b6f0d803944d4dcd08735bae9dc7abdcffd6c428 Mon Sep 17 00:00:00 2001 From: "opensearch-trigger-bot[bot]" <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Date: Thu, 29 Jun 2023 16:22:44 -0700 Subject: [PATCH] [Stylelint] Add invalid properties rule (#4374) (#4441) * Add invalid properties rule Signed-off-by: Matt Provost * Update changelog Signed-off-by: Matt Provost * Rename old variable Signed-off-by: Matt Provost * Add types for configs Signed-off-by: Matt Provost * Rename rule to no_restricted_properties Signed-off-by: Matt Provost * Refactor duplicate functions into generic one Signed-off-by: Matt Provost * Add type definitions Signed-off-by: Matt Provost * Add some documentation about supported config types Signed-off-by: Matt Provost * Update changelog Signed-off-by: Matt Provost * Optchain instead of unwrapping source file Signed-off-by: Matt Provost --------- Signed-off-by: Matt Provost (cherry picked from commit d285ecbd30927bc27332c4fbaa1be836f6702044) Signed-off-by: github-actions[bot] # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] --- packages/osd-stylelint-config/.stylelintrc.js | 8 ++ .../config/restricted_properties.json | 17 ++++ .../osd-stylelint-plugin-stylelint/README.md | 50 +++++++++++ .../src/rules/index.ts | 2 + .../src/rules/no_custom_colors/index.ts | 5 +- .../no_modifying_global_selectors/index.ts | 7 +- .../rules/no_restricted_properties/index.ts | 87 +++++++++++++++++++ .../src/utils/compliance_engine.ts | 14 +-- .../src/utils/get_rules_from_config.ts | 14 ++- 9 files changed, 189 insertions(+), 15 deletions(-) create mode 100644 packages/osd-stylelint-config/config/restricted_properties.json create mode 100644 packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts diff --git a/packages/osd-stylelint-config/.stylelintrc.js b/packages/osd-stylelint-config/.stylelintrc.js index 6a4e2099f5a6..aa06f1a72f08 100644 --- a/packages/osd-stylelint-config/.stylelintrc.js +++ b/packages/osd-stylelint-config/.stylelintrc.js @@ -15,6 +15,14 @@ module.exports = { ], rules: { + '@osd/stylelint/no_restricted_properties': [ + { + config: "./../../../osd-stylelint-config/config/restricted_properties.json" + }, + { + severity: "error" + } + ], '@osd/stylelint/no_modifying_global_selectors': [ { config: "./../../../osd-stylelint-config/config/global_selectors.json" diff --git a/packages/osd-stylelint-config/config/restricted_properties.json b/packages/osd-stylelint-config/config/restricted_properties.json new file mode 100644 index 000000000000..ff79c6fed46b --- /dev/null +++ b/packages/osd-stylelint-config/config/restricted_properties.json @@ -0,0 +1,17 @@ +{ + "font-family": { + "approved": [ + "src/plugins/discover/public/application/_discover.scss", + "src/plugins/maps_legacy/public/map/_leaflet_overrides.scss", + "src/plugins/maps_legacy/public/map/_legend.scss", + "src/plugins/opensearch_dashboards_legacy/public/font_awesome/font_awesome.scss", + "src/plugins/opensearch_dashboards_react/public/markdown/_markdown.scss", + "packages/osd-ui-framework/src/components/tool_bar/_tool_bar_search.scss", + "packages/osd-ui-framework/src/global_styling/mixins/_global_mixins.scss", + "src/plugins/data/public/ui/typeahead/_suggestion.scss", + "src/plugins/vis_type_timeseries/public/application/components/_error.scss", + "packages/osd-ui-framework/src/components/form/check_box/_check_box.scss", + "src/plugins/discover/public/application/components/doc_viewer/doc_viewer.scss" + ] + } +} diff --git a/packages/osd-stylelint-plugin-stylelint/README.md b/packages/osd-stylelint-plugin-stylelint/README.md index e6879cf1a9f5..8c9f0a1d6d4b 100644 --- a/packages/osd-stylelint-plugin-stylelint/README.md +++ b/packages/osd-stylelint-plugin-stylelint/README.md @@ -16,3 +16,53 @@ Enables the ability to capture styling that potentially can be subject to compli For example, if a style attempts to set a button to have a `background-color: $incorrectVariable()` and the JSON config passed to the compliance engine does not explicitly reject the `$incorrectVariable()` being applied to a button's background color then the linter will pass. But it will output this if the environment variable is set to `true`. The goal being that setting this environment variable to output a list that can then be added to the JSON config which we can feed back into the compliance engine. + +## Supported config formats + +Currently, this package supports 2 formats for config: file based and value based. + +### File based config + +Sample: +```json +{ + "#global-id": { + "approved": [ + "src/foo/bar.scss" + ] + }, + ".component-class": { + "approved": [ + "src/bar/baz.scss" + ] + } +} +``` + +Allows specifying a selector, such as a CSS selector (class, id, etc.), to be caught by the rule, as well as an allowlist of files where the selector is allowed. + +### Value based config + +Sample: +```json +{ + "color": { + "button": [ + { + "approved": "$primaryColor", + "rejected": [ + "blue" + ] + }, + { + "approved": "$errorColor", + "rejected": [ + "red" + ] + } + ] + } +} +``` + +Allows specifying multiple selectors for finding values. Supports complex configurations of variants with both an approved value and set of rejected values. diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts index 0f09ff62b234..6dc26fe93f26 100644 --- a/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts +++ b/packages/osd-stylelint-plugin-stylelint/src/rules/index.ts @@ -9,6 +9,7 @@ * GitHub history for details. */ +import noRestrictedProperties from './no_restricted_properties'; import noCustomColors from './no_custom_colors'; import noModifyingGlobalSelectors from './no_modifying_global_selectors'; @@ -16,4 +17,5 @@ import noModifyingGlobalSelectors from './no_modifying_global_selectors'; export default { no_custom_colors: noCustomColors, no_modifying_global_selectors: noModifyingGlobalSelectors, + no_restricted_properties: noRestrictedProperties, }; diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts index b8061f7ff9bd..249b87074949 100644 --- a/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts +++ b/packages/osd-stylelint-plugin-stylelint/src/rules/no_custom_colors/index.ts @@ -19,6 +19,7 @@ import { getRulesFromConfig, isColorProperty, isValidOptions, + ValueBasedConfig, } from '../../utils'; const isOuiAuditEnabled = Boolean(process.env.OUI_AUDIT_ENABLED); @@ -42,7 +43,7 @@ const ruleFunction = ( return; } - const rules = getRulesFromConfig(primaryOption.config); + const rules: ValueBasedConfig = getRulesFromConfig(primaryOption.config); const isAutoFixing = Boolean(context.fix); @@ -84,7 +85,7 @@ const ruleFunction = ( shouldReport = !ruleObject.isComplaint; - if (shouldReport && isAutoFixing) { + if (shouldReport && isAutoFixing && ruleObject.expected) { decl.value = ruleObject.expected; return; } diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts index 2c6ee035aec1..f2b07a7c8d8b 100644 --- a/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts +++ b/packages/osd-stylelint-plugin-stylelint/src/rules/no_modifying_global_selectors/index.ts @@ -15,7 +15,8 @@ import { getNotCompliantMessage, getRulesFromConfig, isValidOptions, - getSelectorRule, + getRuleFromConfig, + FileBasedConfig, } from '../../utils'; const { ruleMessages, report } = stylelint.utils; @@ -36,12 +37,12 @@ const ruleFunction = ( return; } - const rules = getRulesFromConfig(primaryOption.config); + const rules: FileBasedConfig = getRulesFromConfig(primaryOption.config); const isAutoFixing = Boolean(context.fix); postcssRoot.walkRules((rule: any) => { - const selectorRule = getSelectorRule(rules, rule); + const selectorRule = getRuleFromConfig(rules, rule.selector); if (!selectorRule) { return; } diff --git a/packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts b/packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts new file mode 100644 index 000000000000..9da587545b1d --- /dev/null +++ b/packages/osd-stylelint-plugin-stylelint/src/rules/no_restricted_properties/index.ts @@ -0,0 +1,87 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import stylelint from 'stylelint'; +import { NAMESPACE } from '../..'; +import { + getNotCompliantMessage, + getRuleFromConfig, + getRulesFromConfig, + isValidOptions, + FileBasedConfig, +} from '../../utils'; + +const { ruleMessages, report } = stylelint.utils; + +const ruleName = 'no_restricted_properties'; +const messages = ruleMessages(ruleName, { + expected: (message) => `${message}`, +}); + +const ruleFunction: stylelint.Rule = ( + primaryOption: Record, + secondaryOptionObject: Record, + context +) => { + return (postcssRoot, postcssResult) => { + const validOptions = isValidOptions(postcssResult, ruleName, primaryOption); + if (!validOptions) { + return; + } + + const rules: FileBasedConfig = getRulesFromConfig(primaryOption.config); + + const isAutoFixing = Boolean(context.fix); + + postcssRoot.walkDecls((decl) => { + const propertyRule = getRuleFromConfig(rules, decl.prop); + if (!propertyRule) { + return; + } + + let shouldReport = false; + + const file = postcssRoot.source?.input.file; + if (!file) { + return; + } + + const approvedFiles = propertyRule.approved; + + const reportInfo = { + ruleName: `${NAMESPACE}/${ruleName}`, + result: postcssResult, + node: decl, + message: '', + }; + + if (approvedFiles) { + shouldReport = !approvedFiles.some((inspectedFile) => { + return file.includes(inspectedFile); + }); + } + + if (shouldReport && isAutoFixing) { + decl.remove(); + return; + } + + if (!shouldReport) { + return; + } + + reportInfo.message = messages.expected( + getNotCompliantMessage(`Usage of property "${decl.prop}" is not allowed.`) + ); + report(reportInfo); + }); + }; +}; + +ruleFunction.ruleName = ruleName; +ruleFunction.messages = messages; + +// eslint-disable-next-line import/no-default-export +export default ruleFunction; diff --git a/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts b/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts index dcdbcf0cc161..7b95e94e536c 100644 --- a/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts +++ b/packages/osd-stylelint-plugin-stylelint/src/utils/compliance_engine.ts @@ -9,14 +9,16 @@ * GitHub history for details. */ +import { ValueBasedConfig } from './get_rules_from_config'; + export interface ComplianceRule { isComplaint: boolean; actual: string; - expected: string; + expected: string | undefined; message: string; } -const getRule = (rules: JSON, nodeInfo: { selector: string; prop: string }) => { +const getRule = (rules: ValueBasedConfig, nodeInfo: { selector: string; prop: string }) => { const rule = rules[nodeInfo.prop]; if (!rule) { return undefined; @@ -38,12 +40,12 @@ const getRule = (rules: JSON, nodeInfo: { selector: string; prop: string }) => { return rule[ruleKey]; }; -const isTracked = (rules: JSON, nodeInfo: { selector: string; prop: string }) => { +const isTracked = (rules: ValueBasedConfig, nodeInfo: { selector: string; prop: string }) => { return getRule(rules, nodeInfo) !== undefined; }; const getComplianceRule = ( - rules: JSON, + rules: ValueBasedConfig, nodeInfo: { selector: string; prop: string; value: string } ): ComplianceRule | undefined => { const rule = getRule(rules, nodeInfo); @@ -53,7 +55,7 @@ const getComplianceRule = ( } const ruleObject = rule.find((object) => { - if (object.approved.includes(nodeInfo.value) || object.rejected.includes(nodeInfo.value)) { + if (object.approved?.includes(nodeInfo.value) || object.rejected?.includes(nodeInfo.value)) { return object; } }); @@ -63,7 +65,7 @@ const getComplianceRule = ( } return { - isComplaint: !ruleObject.rejected.includes(nodeInfo.value), + isComplaint: !ruleObject.rejected?.includes(nodeInfo.value), actual: nodeInfo.value, expected: ruleObject.approved, message: `${nodeInfo.selector} expected: ${ruleObject.approved} - actual: ${nodeInfo.value}`, diff --git a/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts b/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts index dc13230fa6b0..cc97d13a030c 100644 --- a/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts +++ b/packages/osd-stylelint-plugin-stylelint/src/utils/get_rules_from_config.ts @@ -13,15 +13,21 @@ import path from 'path'; import { readFileSync } from 'fs'; import { matches } from './matches'; +export type FileBasedConfig = Record; +export type ValueBasedConfig = Record< + string, + Record> +>; + export const getRulesFromConfig = (configPath: string) => { const filePath = path.resolve(__dirname, configPath); return JSON.parse(readFileSync(filePath, 'utf-8')); }; -export const getSelectorRule = (rules: any, rule: any) => { - for (const configRule of Object.keys(rules)) { - if (matches(configRule, rule.selector)) { - return rules[configRule]; +export const getRuleFromConfig = (rules: FileBasedConfig, value: string) => { + for (const key of Object.keys(rules)) { + if (matches(key, value)) { + return rules[key]; } }