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 @@ -37,6 +37,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 decline configured properties ([#4374](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4374))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rephrase this to be clearer? It seems like the change is that we can now declare CSS properties that can only be specified by allowlisted files. Something along those lines would be better for someone scanning the release notes without context.


### 📝 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"
]
}
}
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,83 @@
/*
* 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 = (
primaryOption: Record<string, any>,
secondaryOptionObject: Record<string, any>,
context
) => {
return (postcssRoot: any, postcssResult: any) => {
const validOptions = isValidOptions(postcssResult, ruleName, primaryOption);
if (!validOptions) {
return;
}

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

const isAutoFixing = Boolean(context.fix);

postcssRoot.walkDecls((decl: any) => {
const propertyRule = getRuleFromConfig(rules, decl.prop);
if (!propertyRule) {
return;
}

let shouldReport = false;

const file = postcssRoot.source.input.file;
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
Loading