Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Stylelint] Add invalid properties rule #4374

Merged
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Add an achievement badger to the PR ([#3721](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3721))
- Upgrade the backport workflow ([#4343](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4343))
- [Lint] Add custom stylelint rules and config to prevent unintended style overrides ([#4290](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4290))
- [Lint] Add stylelint rule to define properties that are restricted from being used ([#4374](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4374))

### 📝 Documentation

Expand Down
8 changes: 8 additions & 0 deletions packages/osd-stylelint-config/.stylelintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
17 changes: 17 additions & 0 deletions packages/osd-stylelint-config/config/restricted_properties.json
Original file line number Diff line number Diff line change
@@ -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"
]
}
}
50 changes: 50 additions & 0 deletions packages/osd-stylelint-plugin-stylelint/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
2 changes: 2 additions & 0 deletions packages/osd-stylelint-plugin-stylelint/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@
* GitHub history for details.
*/

import noRestrictedProperties from './no_restricted_properties';
import noCustomColors from './no_custom_colors';
import noModifyingGlobalSelectors from './no_modifying_global_selectors';

// eslint-disable-next-line import/no-default-export
export default {
no_custom_colors: noCustomColors,
no_modifying_global_selectors: noModifyingGlobalSelectors,
no_restricted_properties: noRestrictedProperties,
};
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
getRulesFromConfig,
isColorProperty,
isValidOptions,
ValueBasedConfig,
} from '../../utils';

const isOuiAuditEnabled = Boolean(process.env.OUI_AUDIT_ENABLED);
Expand All @@ -42,7 +43,7 @@ const ruleFunction = (
return;
}

const rules = getRulesFromConfig(primaryOption.config);
const rules: ValueBasedConfig = getRulesFromConfig(primaryOption.config);

const isAutoFixing = Boolean(context.fix);

Expand Down Expand Up @@ -84,7 +85,7 @@ const ruleFunction = (

shouldReport = !ruleObject.isComplaint;

if (shouldReport && isAutoFixing) {
if (shouldReport && isAutoFixing && ruleObject.expected) {
decl.value = ruleObject.expected;
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import {
getNotCompliantMessage,
getRulesFromConfig,
isValidOptions,
getSelectorRule,
getRuleFromConfig,
FileBasedConfig,
} from '../../utils';

const { ruleMessages, report } = stylelint.utils;
Expand All @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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<string, any>,
secondaryOptionObject: Record<string, any>,
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;
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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;
}
});
Expand All @@ -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}`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,21 @@ import path from 'path';
import { readFileSync } from 'fs';
import { matches } from './matches';

export type FileBasedConfig = Record<string, { approved?: string[] }>;
BSFishy marked this conversation as resolved.
Show resolved Hide resolved
export type ValueBasedConfig = Record<
string,
Record<string, Array<{ approved?: string; rejected?: string[] }>>
>;

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];
}
}

Expand Down