Chore: Move betterer eslint rules to use eslint suppressions (#106267)

Co-authored-by: joshhunt <josh.hunt@grafana.com>
This commit is contained in:
Tom Ratcliffe 2025-09-04 10:47:13 +01:00 committed by GitHub
parent df685757ff
commit 55b638ea98
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
16 changed files with 5219 additions and 4703 deletions

View file

@ -1,187 +0,0 @@
// @ts-check
const emotionPlugin = require('@emotion/eslint-plugin');
const importPlugin = require('eslint-plugin-import');
const jestPlugin = require('eslint-plugin-jest');
const jsxA11yPlugin = require('eslint-plugin-jsx-a11y');
const lodashPlugin = require('eslint-plugin-lodash');
const barrelPlugin = require('eslint-plugin-no-barrel-files');
const reactPlugin = require('eslint-plugin-react');
const testingLibraryPlugin = require('eslint-plugin-testing-library');
const grafanaConfig = require('@grafana/eslint-config/flat');
const grafanaPlugin = require('@grafana/eslint-plugin');
const grafanaI18nPlugin = require('@grafana/i18n/eslint-plugin');
// Include the base Grafana configs and remove the rules,
// as we just want to pull in all of the necessary configuration but not run the rules
// (this should only be concerned with checking rules that we want to improve,
// so there's no need to try and run the rules that will be linted properly anyway)
const mappedBaseConfigs = grafanaConfig.map((/** @type {import('eslint').Linter.Config} */ config) => {
const { rules, ...baseConfig } = config;
return baseConfig;
});
/**
* @type {Array<import('eslint').Linter.Config>}
*/
module.exports = [
{
name: 'grafana/betterer-ignores',
ignores: [
'.github',
'.yarn',
'**/.*',
'**/*.gen.ts',
'**/build/',
'**/compiled/',
'**/dist/',
'data/',
'deployment_tools_config.json',
'devenv',
'e2e-playwright/test-plugins',
'e2e/tmp',
'packages/grafana-ui/src/components/Icon/iconBundle.ts',
'pkg',
'playwright-report',
'public/lib/monaco/',
'public/locales/_build',
'public/locales/**/*.js',
'public/vendor/',
'scripts/grafana-server/tmp',
'!.betterer.eslint.config.js',
],
},
{
name: 'react/jsx-runtime-rules',
rules: reactPlugin.configs.flat['jsx-runtime'].rules,
},
...mappedBaseConfigs,
{
files: ['**/*.{ts,tsx,js}'],
plugins: {
'@emotion': emotionPlugin,
lodash: lodashPlugin,
jest: jestPlugin,
import: importPlugin,
'jsx-a11y': jsxA11yPlugin,
'no-barrel-files': barrelPlugin,
'@grafana': grafanaPlugin,
'testing-library': testingLibraryPlugin,
'@grafana/i18n': grafanaI18nPlugin,
},
linterOptions: {
// This reports unused disable directives that we can clean up but
// it also conflicts with the betterer eslint rules so disabled
reportUnusedDisableDirectives: false,
},
},
{
files: ['**/*.{js,jsx,ts,tsx}'],
rules: {
'react-hooks/rules-of-hooks': 'error',
'@typescript-eslint/no-explicit-any': 'error',
'@grafana/no-aria-label-selectors': 'error',
'no-restricted-imports': [
'error',
{
patterns: [
{
group: ['@grafana/ui/src/*', '@grafana/runtime/src/*', '@grafana/data/src/*'],
message: 'Import from the public export instead.',
},
],
},
],
},
},
{
files: ['**/*.{js,jsx,ts,tsx}'],
ignores: [
'**/*.{test,spec}.{ts,tsx}',
'**/__mocks__/**',
'**/public/test/**',
'**/mocks.{ts,tsx}',
'**/mocks/**/*.{ts,tsx}',
'**/spec/**/*.{ts,tsx}',
],
rules: {
'@typescript-eslint/consistent-type-assertions': ['error', { assertionStyle: 'never' }],
},
},
{
files: ['**/*.{js,jsx,ts,tsx}'],
ignores: [
'**/*.{test,spec}.{ts,tsx}',
'**/__mocks__/**',
'**/public/test/**',
'**/mocks.{ts,tsx}',
'**/spec/**/*.{ts,tsx}',
],
rules: {
'no-restricted-syntax': [
'error',
{
selector: 'Identifier[name=localStorage]',
message: 'Direct usage of localStorage is not allowed. import store from @grafana/data instead',
},
{
selector: 'MemberExpression[object.name=localStorage]',
message: 'Direct usage of localStorage is not allowed. import store from @grafana/data instead',
},
{
selector:
'Program:has(ImportDeclaration[source.value="@grafana/ui"] ImportSpecifier[imported.name="Card"]) JSXOpeningElement[name.name="Card"]:not(:has(JSXAttribute[name.name="noMargin"]))',
message:
'Add noMargin prop to Card components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.',
},
{
selector:
'Program:has(ImportDeclaration[source.value="@grafana/ui"] ImportSpecifier[imported.name="Field"]) JSXOpeningElement[name.name="Field"]:not(:has(JSXAttribute[name.name="noMargin"]))',
message:
'Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.',
},
{
selector: 'CallExpression[callee.type="MemberExpression"][callee.property.name="localeCompare"]',
message:
'Using localeCompare() can cause performance issues when sorting large datasets. Consider using Intl.Collator for better performance when sorting arrays, or add an eslint-disable comment if sorting a small, known dataset.',
},
{
// eslint-disable-next-line no-restricted-syntax
selector: 'Literal[value=/gf-form/], TemplateElement[value.cooked=/gf-form/]',
// eslint-disable-next-line no-restricted-syntax
message: 'gf-form usage has been deprecated. Use a component from @grafana/ui or custom CSS instead.',
},
{
selector:
"Property[key.name='a11y'][value.type='ObjectExpression'] Property[key.name='test'][value.value='off']",
message: 'Skipping a11y tests is not allowed. Please fix the component or story instead.',
},
],
},
},
{
files: ['public/app/**/*.{ts,tsx}'],
rules: {
'no-barrel-files/no-barrel-files': 'error',
},
},
{
// custom rule for Table to avoid performance regressions
files: ['packages/grafana-ui/src/components/Table/TableNG/Cells/**/*.{ts,tsx}'],
rules: {
'no-restricted-imports': [
'error',
{
patterns: [
{
group: ['**/themes/ThemeContext'],
importNames: ['useStyles2', 'useTheme2'],
message:
'Do not use "useStyles2" or "useTheme2" in a cell directly. Instead, provide styles to cells via `getDefaultCellStyles` or `getCellSpecificStyles`.',
},
],
},
],
},
},
];

File diff suppressed because it is too large Load diff

View file

@ -1,44 +1 @@
import { BettererFileTest } from '@betterer/betterer';
import { ESLint } from 'eslint';
// Why are we ignoring these?
// They're all deprecated/being removed so doesn't make sense to fix types
const eslintPathsToIgnore = [
'packages/grafana-ui/src/graveyard', // will be removed alongside angular in Grafana 12
'public/app/angular', // will be removed in Grafana 12
'public/app/plugins/panel/graph', // will be removed alongside angular in Grafana 12
'public/app/plugins/panel/table-old', // will be removed alongside angular in Grafana 12
];
// Avoid using functions that report the position of the issues, as this causes a lot of merge conflicts
export default {
'better eslint': () =>
countEslintErrors()
.include('**/*.{ts,tsx}')
.exclude(new RegExp(eslintPathsToIgnore.join('|'))),
};
function countEslintErrors() {
return new BettererFileTest(async (filePaths, fileTestResult) => {
// Just bail early if there's no files to test. Prevents trying to get the base config from failing
if (filePaths.length === 0) {
return;
}
const runner = new ESLint({
overrideConfigFile: './.betterer.eslint.config.js',
warnIgnored: false,
});
const lintResults = await runner.lintFiles(Array.from(filePaths));
lintResults.forEach(({ messages, filePath }) => {
const file = fileTestResult.addFile(filePath, '');
messages
.sort((a, b) => (a.message > b.message ? 1 : -1))
.forEach((message, index) => {
file.addIssue(0, 0, message.message, `${index}`);
});
});
});
}
export default {};

5
.github/CODEOWNERS vendored
View file

@ -730,7 +730,6 @@
/scripts/tsconfig.base.json @grafana/frontend-ops
/.editorconfig @grafana/frontend-ops
/eslint.config.js @grafana/frontend-ops
/.betterer.eslint.config.js @grafana/frontend-ops
/.gitattributes @grafana/frontend-ops
/.gitignore @grafana/frontend-ops
/.ignore @grafana/frontend-ops
@ -1012,7 +1011,7 @@ playwright.storybook.config.ts @grafana/grafana-frontend-platform
/scripts/circle-* @grafana/grafana-developer-enablement-squad
/scripts/publish-npm-packages.sh @grafana/grafana-developer-enablement-squad @grafana/plugins-platform-frontend
/scripts/validate-npm-packages.sh @grafana/grafana-developer-enablement-squad @grafana/plugins-platform-frontend
/scripts/ci-frontend-metrics.sh @grafana/grafana-frontend-platform @grafana/plugins-platform-frontend @grafana/dataviz-squad @grafana/datapro
/scripts/ci-frontend-metrics.sh @grafana/grafana-frontend-platform @grafana/frontend-ops
/scripts/cli/ @grafana/grafana-frontend-platform
/scripts/clean-git-or-error.sh @grafana/grafana-as-code
/scripts/grafana-server/ @grafana/grafana-frontend-platform
@ -1041,8 +1040,8 @@ playwright.storybook.config.ts @grafana/grafana-frontend-platform
/scripts/**/generate-transformations* @grafana/datapro
/scripts/webpack/ @grafana/frontend-ops
/scripts/generate-a11y-report.sh @grafana/grafana-frontend-platform
.betterer.results @grafanabot
.betterer.ts @grafana/grafana-frontend-platform
eslint-suppressions.json @grafanabot
# Design system
/public/img/icons/unicons/ @grafana/design-system

View file

@ -42,4 +42,6 @@ public/mockServiceWorker.js
# Playwright results
test-results
playwright-report
playwright-report
eslint-suppressions.json

9
.vscode/launch.json vendored
View file

@ -120,6 +120,15 @@
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen"
},
{
"name": "Debug ESLint with stats reporter",
"type": "node",
"request": "launch",
"runtimeExecutable": "yarn",
"runtimeArgs": ["run", "eslint", "${file}", "--format", "./scripts/cli/eslint-stats-reporter.mjs"],
"console": "integratedTerminal",
"internalConsoleOptions": "neverOpen"
},
{
"name": "Debug Go test",
"type": "go",

View file

@ -2200,9 +2200,6 @@ secret_key = SW2YcwTIb9zpOOhoPsMm
# Should UI tests fail when console log/warn/erroring?
# Does not affect the result when running on CI - only for allowing devs to choose this behaviour locally
fail_tests_on_console = true
# Whether to enable betterer eslint rules for local development
# Useful if you want to always see betterer rules that we're trying to fix so they're more prevalent
betterer_eslint_rules = false
#################################### Plugin API Restrictions ##########################################
# Configure which plugins can access specific restricted APIs.

View file

@ -60,16 +60,17 @@ Pull requests that create new UI components or modify existing ones must adhere
Before submitting pull requests that introduce accessibility (a11y) errors, refer to the [accessibility guidelines](/contribute/style-guides/accessibility.md).
### Betterer
### ESLint & suppressions
We make use of a tool called [**Betterer**](https://phenomnomnominal.github.io/betterer/) in order to drive long-running code quality improvements. Our intention is for this requirement to be as unintrusive as possible; however, there are some things to be aware of:
We use [ESLint](https://eslint.org/) to enforce code style and best practices, along with [bulk suppressions](https://eslint.org/docs/latest/use/suppressions#suppressions-file) in order to incrementally improve our code quality and fix rule violations.
- **Betterer runs as a precommit hook**:
- You may see changes to the `.betterer.results` file automatically added to your commits.
- **ESLint runs as a precommit hook**:
- You may see changes to the `eslint-suppressions.json` file automatically added to your commits.
- You may get an error when trying to commit something that decreases the overall code quality. You can either fix these errors or temporarily override the checks (for example, to commit something that's a work in progress). To do so, use `git commit --no-verify`. All errors will eventually have to be fixed before your code can be merged because...
- **Betterer also runs as part of our CI**:
- You may see the following error message in the CI: `Unexpected changes detected in these tests while running in CI mode`. To resolve the error, merge with the target branch (usually `main`).
- You may see merge conflicts for the `.betterer.results` file. To resolve, merge with the target branch (usually `main`), and then run `yarn betterer:merge` and commit.
- **ESLint also runs as part of our CI**:
- If you have fixed suppressed issues but not updated the suppressions file, you may see the following error message in the CI: `There are suppressions left that do not occur anymore.`.
To resolve the error, run the following command: `yarn lint:prune` and commit the changes.
- You may see merge conflicts for the `eslint-suppressions.json` file. To resolve, merge with the target branch (usually `main`) and resolve conflicts however you like, and then run `yarn lint:prune` to ensure the file is up to date and commit.
## Guidelines for backend development

View file

@ -61,7 +61,7 @@ To remove precommit hooks:
make lefthook-uninstall
```
> We strongly encourage contributors who work on the frontend to install the precommit hooks, even if your IDE formats on save. By doing so, the `.betterer.results` file is kept in sync.
> We strongly encourage contributors who work on the frontend to install the precommit hooks, even if your IDE formats on save. By doing so, the `eslint-suppressions.json` file is kept in sync.
## Build Grafana

5012
eslint-suppressions.json Normal file

File diff suppressed because it is too large Load diff

View file

@ -15,17 +15,23 @@ const grafanaConfig = require('@grafana/eslint-config/flat');
const grafanaPlugin = require('@grafana/eslint-plugin');
const grafanaI18nPlugin = require('@grafana/i18n/eslint-plugin');
const bettererConfig = require('./.betterer.eslint.config');
const getEnvConfig = require('./scripts/webpack/env-util');
const envConfig = getEnvConfig();
const enableBettererRules = envConfig.frontend_dev_betterer_eslint_rules;
const pluginsToTranslate = [
'public/app/plugins/panel',
'public/app/plugins/datasource/azuremonitor',
'public/app/plugins/datasource/mssql',
];
const commonTestIgnores = [
'**/*.{test,spec}.{ts,tsx}',
'**/__mocks__/**',
'**/mocks/**/*.{ts,tsx}',
'**/public/test/**',
'**/mocks.{ts,tsx}',
'**/spec/**/*.{ts,tsx}',
];
const enterpriseIgnores = ['public/app/extensions/**/*', 'e2e/extensions/**/*'];
// [FIXME] add comment about this applying everywhere
const baseImportConfig = {
patterns: [
@ -49,6 +55,10 @@ const baseImportConfig = {
message:
'Do not import test files. If you require reuse of constants/mocks across files, create a separate file with no tests',
},
{
group: ['@grafana/ui/src/*', '@grafana/runtime/src/*', '@grafana/data/src/*'],
message: 'Import from the public export instead.',
},
],
paths: [
{
@ -69,7 +79,6 @@ function withBaseRestrictedImportsConfig(config = {}) {
patterns: [...baseImportConfig.patterns, ...(config?.patterns ?? [])],
paths: [...baseImportConfig.paths, ...(config?.paths ?? [])],
};
return finalConfig;
}
@ -100,7 +109,6 @@ module.exports = [
'public/locales/**/*.js',
'public/vendor/',
'scripts/grafana-server/tmp',
'!.betterer.eslint.config.js',
'packages/grafana-ui/src/graveyard', // deprecated UI components slated for removal
'public/build-swagger', // swagger build output
],
@ -181,10 +189,6 @@ module.exports = [
message: 'No bare anchor nodes containing only text. Use `TextLink` instead.',
},
],
// FIXME: Fix these in follow up PR
'react/no-unescaped-entities': 'off',
// Turn off react-hooks/rules-of-hooks whilst present in betterer
'react-hooks/rules-of-hooks': 'off',
},
},
@ -206,7 +210,6 @@ module.exports = [
],
},
},
{
name: 'grafana/uplot-overrides',
files: ['packages/grafana-ui/src/components/uPlot/**/*.{ts,tsx}'],
@ -449,7 +452,99 @@ module.exports = [
},
},
// Conditionally run the betterer rules if enabled in dev's config
// Should be last in the config so it can override any temporary disables in here
...(enableBettererRules ? bettererConfig : []),
{
// custom rule for Table to avoid performance regressions
files: ['packages/grafana-ui/src/components/Table/TableNG/Cells/**/*.{ts,tsx}'],
rules: {
'no-restricted-imports': [
'error',
withBaseRestrictedImportsConfig({
patterns: [
{
group: ['**/themes/ThemeContext'],
importNames: ['useStyles2', 'useTheme2'],
message:
'Do not use "useStyles2" or "useTheme2" in a cell directly. Instead, provide styles to cells via `getDefaultCellStyles` or `getCellSpecificStyles`.',
},
],
}),
],
},
},
// Old betterer rules config:
{
files: ['**/*.{js,jsx,ts,tsx}'],
ignores:
// FIXME: Remove once all enterprise issues are fixed -
// we don't have a suppressions file/approach for enterprise code yet
enterpriseIgnores,
rules: {
'@typescript-eslint/no-explicit-any': 'error',
'@grafana/no-aria-label-selectors': 'error',
},
},
{
files: ['**/*.{js,jsx,ts,tsx}'],
ignores: [
...commonTestIgnores,
// FIXME: Remove once all enterprise issues are fixed -
// we don't have a suppressions file/approach for enterprise code yet
...enterpriseIgnores,
],
rules: {
'@typescript-eslint/consistent-type-assertions': ['error', { assertionStyle: 'never' }],
'no-restricted-syntax': [
'error',
{
selector: 'Identifier[name=localStorage]',
message: 'Direct usage of localStorage is not allowed. import store from @grafana/data instead',
},
{
selector: 'MemberExpression[object.name=localStorage]',
message: 'Direct usage of localStorage is not allowed. import store from @grafana/data instead',
},
{
selector:
'Program:has(ImportDeclaration[source.value="@grafana/ui"] ImportSpecifier[imported.name="Card"]) JSXOpeningElement[name.name="Card"]:not(:has(JSXAttribute[name.name="noMargin"]))',
message:
'Add noMargin prop to Card components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.',
},
{
selector:
'Program:has(ImportDeclaration[source.value="@grafana/ui"] ImportSpecifier[imported.name="Field"]) JSXOpeningElement[name.name="Field"]:not(:has(JSXAttribute[name.name="noMargin"]))',
message:
'Add noMargin prop to Field components to remove built-in margins. Use layout components like Stack or Grid with the gap prop instead for consistent spacing.',
},
{
selector: 'CallExpression[callee.type="MemberExpression"][callee.property.name="localeCompare"]',
message:
'Using localeCompare() can cause performance issues when sorting large datasets. Consider using Intl.Collator for better performance when sorting arrays, or add an eslint-disable comment if sorting a small, known dataset.',
},
{
// eslint-disable-next-line no-restricted-syntax
selector: 'Literal[value=/gf-form/], TemplateElement[value.cooked=/gf-form/]',
// eslint-disable-next-line no-restricted-syntax
message: 'gf-form usage has been deprecated. Use a component from @grafana/ui or custom CSS instead.',
},
{
selector:
"Property[key.name='a11y'][value.type='ObjectExpression'] Property[key.name='test'][value.value='off']",
message: 'Skipping a11y tests is not allowed. Please fix the component or story instead.',
},
],
},
},
{
files: ['public/app/**/*.{ts,tsx}'],
ignores: [
...commonTestIgnores,
// FIXME: Remove once all enterprise issues are fixed -
// we don't have a suppressions file/approach for enterprise code yet
...enterpriseIgnores,
],
rules: {
'no-barrel-files/no-barrel-files': 'error',
},
},
];

View file

@ -19,7 +19,8 @@ pre-commit:
frontend-lint:
glob: '*.{js,ts,tsx}'
run: |
yarn eslint --ext .js,.tsx,.ts --cache --fix {staged_files}
yarn eslint --ext .js,.tsx,.ts --cache --prune-suppressions --fix {staged_files} &&
git add eslint-suppressions.json &&
yarn prettier --write {staged_files}
stage_fixed: true

View file

@ -38,6 +38,7 @@
"lint:ts": "eslint ./ ./public/app/extensions/ --cache --no-error-on-unmatched-pattern",
"lint:sass": "yarn stylelint '{public/sass,packages}/**/*.scss' --cache",
"lint:fix": "yarn lint:ts --fix",
"lint:prune": "yarn lint:ts --prune-suppressions",
"packages:build": "nx run-many -t build --projects='tag:scope:package'",
"packages:clean": "rimraf ./npm-artifacts && nx run-many -t clean --projects='tag:scope:package' --maxParallel=100",
"packages:i18n-extract": "nx run-many -t i18n-extract --projects='tag:scope:package'",

View file

@ -32,6 +32,17 @@ do
BETTERER_STATS+="\"grafana.ci-code.betterer.${name}\": \"${value}\","
done <<< "$(yarn betterer:stats)"
ESLINT_STATS=""
yarn lint:ts --format ./scripts/cli/eslint-stats-reporter.mjs -o eslint-stats.txt
while read -r name value
do
ESLINT_STATS+=$'\n '
# We still report these as "betterer" as the dashboards/other scripts will still look for it there
ESLINT_STATS+="\"grafana.ci-code.betterer.${name}\": \"${value}\","
done <<< "$(cat eslint-stats.txt)"
rm eslint-stats.txt
I18N_STATS=""
while read -r name value
do
@ -49,6 +60,7 @@ done <<< "$(yarn themes:usage | awk '$4 == "@grafana/theme-token-usage" {print $
echo "Metrics: {
$THEME_TOKEN_USAGE
$BETTERER_STATS
$ESLINT_STATS
$I18N_STATS
\"grafana.ci-code.strictErrors\": \"${ERROR_COUNT}\",
\"grafana.ci-code.accessibilityErrors\": \"${ACCESSIBILITY_ERRORS}\",

View file

@ -0,0 +1,56 @@
//@ts-check
import lodash from 'lodash';
const { camelCase } = lodash;
/**
* Rule IDs that are overly verbose and that we want to combine so they report more cleanly
*
* i.e. so we can report `reactHooksRulesOfHooks` instead of `reactHookFooIsCalledConditionallyReactHooksMust...`
*/
const rulesToCombine = ['react-hooks/rules-of-hooks', 'react/no-unescaped-entities', 'no-barrel-files/no-barrel-files'];
const legacyChecksToTransform = [
{ messageRegex: /gfFormUsage/i, prefix: 'noGfFormUsage' },
{ messageRegex: /skippingA11Y/i, prefix: 'noSkippingA11YTestsInStories' },
];
/**
* Custom formatter that outputs suppressed rule violations in a format suitable for
* consuming on our CI code stats scripts
*
* Output in the format:
* @example
* betterEslint_reactHooksRulesOfHooks 123
* betterEslint_noBarrelFilesNoBarrelFiles 123
*
* @type {import('eslint').ESLint.FormatterFunction}
*/
export default function statsReporter(results) {
/** @type {Record<string, number>} */
const countByMessage = {};
for (const result of results) {
for (const message of result.suppressedMessages) {
// eslint disable directives count as suppressions
// we only want to report the case where everything is a file suppression
const everySuppressionIsFile = message.suppressions.every((suppression) => suppression.kind === 'file');
if (!everySuppressionIsFile) {
continue;
}
const key =
message.ruleId && rulesToCombine.includes(message.ruleId)
? camelCase(message.ruleId)
: camelCase(message.message);
countByMessage[key] = (countByMessage[key] || 0) + 1;
}
}
return Object.entries(countByMessage)
.map(([key, value]) => {
const prefix = legacyChecksToTransform.find((v) => v.messageRegex.test(key))?.prefix || 'betterEslint';
return `${prefix}_${key} ${value}`;
})
.join('\n');
}

View file

@ -10,5 +10,6 @@
"compilerOptions": {
"module": "commonjs"
}
}
},
"include": ["./**/*.ts", "./**/*.mts", "./**/*.js", "./**/*.mjs"]
}