Skip to content

Commit

Permalink
Update ESLint and related plugins (jaegertracing#1250)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- Contributes towards jaegertracing#1199 

## Short description of the changes
Bump ESLint to v7 and update related plugins. Suppress newly introduced
or updated rules in a separate config section so that they can be
evaluated and enabled later, except for stylistic rules that do not
match the existing code style, or where the impact was limited enough to
be trivially fixable or suppressable.

`@typescript-eslint/eslint-plugin` now validates that input files are
indeed part of the tsconfig file configured in ESLint options. Since the
plugin doesn't understand project references, this requires a small
change to [specify the package-specific tsconfig files for
linting](https://typescript-eslint.io/linting/typed-linting/monorepos#one-tsconfigjson-per-package-and-an-optional-one-in-the-root)
instead.

Also remove `eslint-config-react-app` (the CRA ESLint config), as it
doesn't actually seem to bring much to the table—the Airbnb config seems
to overwrite much of the same rules that this config is trying to
provide. I ran `eslint --print-config
packages/jaeger-ui/src/components/App/index.jsx` and `eslint
--print-config packages/jaeger-ui/src/components/App/Page.tsx` to dump
the active rules for JS and TS files with and without the config loaded,
then diffed them:

```diff
--- ts-active-rules-cra.json	2023-03-10 21:24:37
+++ ts-active-rules-no-cra.json	2023-03-10 21:25:38
@@ -4,8 +4,7 @@
     "jest": true,
     "jasmine": true,
     "es6": true,
-    "node": true,
-    "commonjs": true
+    "node": true
   },
   "globals": {
     "__REACT_APP_GA_DEBUG__": false,
@@ -27,7 +26,6 @@
     "sourceType": "module"
   },
   "plugins": [
-    "flowtype",
     "import",
     "react",
     "jsx-a11y",
@@ -2850,15 +2848,6 @@
     ],
     "yoda": [
       "error"
-    ],
-    "flowtype/define-flow-type": [
-      "warn"
-    ],
-    "flowtype/require-valid-file-annotation": [
-      "warn"
-    ],
-    "flowtype/use-flow-type": [
-      "warn"
     ]
   },
   "settings": {
```

```diff
--- js-active-rules-cra.json	2023-03-10 21:24:48
+++ js-active-rules-no-cra.json	2023-03-10 21:25:30
@@ -4,15 +4,14 @@
     "jest": true,
     "jasmine": true,
     "es6": true,
-    "node": true,
-    "commonjs": true
+    "node": true
   },
   "globals": {
     "__REACT_APP_GA_DEBUG__": false,
     "__REACT_APP_VSN_STATE__": false,
     "__APP_ENVIRONMENT__": false
   },
-  "parser": "/REDACTED/jaeger-ui/node_modules/babel-eslint/lib/index.js",
+  "parser": null,
   "parserOptions": {
     "ecmaFeatures": {
       "jsx": true,
@@ -23,7 +22,6 @@
     "sourceType": "module"
   },
   "plugins": [
-    "flowtype",
     "import",
     "react",
     "jsx-a11y"
@@ -2830,15 +2828,6 @@
     ],
     "yoda": [
       "error"
-    ],
-    "flowtype/define-flow-type": [
-      "warn"
-    ],
-    "flowtype/require-valid-file-annotation": [
-      "warn"
-    ],
-    "flowtype/use-flow-type": [
-      "warn"
     ]
   },
   "settings": {
```

So the only things added by the config is Flow support (which the
project doesn't use), and extended ES6 support via `babel-eslint` (which
is abandoned in favor of `@babel/eslint-parser`). As such, remove
`eslint-plugin-flowtype` as well and replace `babel-eslint` with a
simple `@babel/eslint-parser` config.

---------

Signed-off-by: Máté Szabó <mszabo@fandom.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
  • Loading branch information
mszabo-wikia and yurishkuro authored Mar 10, 2023
1 parent 397b335 commit 9075aa4
Show file tree
Hide file tree
Showing 12 changed files with 651 additions and 431 deletions.
60 changes: 57 additions & 3 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,25 +25,59 @@ module.exports = {
},
},
},
extends: ['react-app', 'airbnb', 'prettier', 'prettier/react'],
extends: ['airbnb', 'prettier'],
overrides: [
{
files: ['*.jsx', '*.test.js'],
parser: '@babel/eslint-parser',
parserOptions: {
requireConfigFile: false,
babelOptions: {
babelrc: false,
configFile: false,
presets: ["@babel/preset-react"],
},
}
},
{
files: ['*.ts', '*.tsx'],
parser: '@typescript-eslint/parser',
parserOptions: {
project: './tsconfig.json',
project: ['./packages/*/tsconfig.json'],
tsconfigRootDir: '.',
},
plugins: ['@typescript-eslint'],
rules: {
'no-unused-vars': 0,
'@typescript-eslint/interface-name-prefix': ['error', 'always'],
'@typescript-eslint/naming-convention': [
'error',
{
selector: 'interface',
format: ['PascalCase'],
prefix: ['I'],
},
],
'@typescript-eslint/no-unused-vars': 1,

// Disable prop type checks for TSX components, as prop type validation is expected
// to be handled by TypeScript there. Stray prop types in components converted from Flow
// should eventually be removed.
'react/require-default-props': 0,
'react/default-props-match-prop-types': 0,
'react/no-unused-prop-types': 0,
},
},
{
files: ['*.test.js'],
rules: {
// Used for Jest module mocking.
'no-import-assign': 0,
},
},
],
rules: {
/* general */
'arrow-body-style': 0,
'arrow-parens': [1, 'as-needed'],
'comma-dangle': 0,
'lines-between-class-members': ['error', 'always', { exceptAfterSingleLine: true }],
Expand All @@ -68,13 +102,16 @@ module.exports = {
'react/jsx-curly-brace-presence': ['error', 'never'],
'react/jsx-filename-extension': 0,
'react/forbid-prop-types': 1,
'react/function-component-definition': 0,
'react/require-default-props': 1,
'react/no-array-index-key': 1,
'react/no-unused-class-component-methods': 0,
'react/sort-comp': [
2,
{
order: [
'type-annotations',
'defaultProps',
'statics',
'state',
'propTypes',
Expand All @@ -89,6 +126,23 @@ module.exports = {
},
],

// eslint-config-airbnb v18+ relaxations
'jsx-a11y/control-has-associated-label': 0,
'react/jsx-props-no-spreading': 0,
'react/state-in-constructor': 0,
'react/static-property-placement': 0,
'react/jsx-fragments': 0,
'react/prop-types': 0,
'max-classes-per-file': 0,
'no-restricted-exports': [
'error',
{
restrictedNamedExports: ['then'],
},
],
'prefer-arrow-callback': 0,
'prefer-object-spread': 0,

/* import */
'import/prefer-default-export': 1,
'import/no-named-default': 0,
Expand Down
4 changes: 0 additions & 4 deletions BUILD.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@

### Dependencies (dev and otherwise)

#### `eslint-plugin-flowtype`

While, this project does not use [`flow`](https://flow.org/), this ESLint plugin is required because the configuration extends [`react-app`](https://github.com/facebook/create-react-app/blob/master/packages/eslint-config-react-app/package.json#L18), which requires this plugin.

#### `@typescript-eslint/eslint-plugin`

ESLint is being used to lint the repo, as a whole. Within `./packages/plexus` (for now), [`@typescript-eslint/eslint-plugin`](https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin) is used to apply ESLint to TypeScript. This application is localized to plexus via configuring `./packages/plexus/.eslintrc.js` for TypeScript, which means the change in settings is only applied to subdirectories of `./packages/plexus`. This package works really well, but there are quite a few issues it doesn't catch. For that, we use the TypeScript compiler.
Expand Down
19 changes: 8 additions & 11 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,15 @@
"url": "https://github.com/jaegertracing/jaeger-ui.git"
},
"devDependencies": {
"@typescript-eslint/eslint-plugin": "1.13.0",
"@typescript-eslint/parser": "1.13.0",
"@typescript-eslint/typescript-estree": "5.54.1",
"babel-eslint": "10.1.0",
"eslint": "5.16.0",
"eslint-config-airbnb": "17.1.0",
"eslint-config-prettier": "6.15.0",
"eslint-config-react-app": "3.0.7",
"eslint-plugin-flowtype": "^3.4.2",
"@babel/eslint-parser": "^7.19.1",
"@typescript-eslint/eslint-plugin": "3.2.0",
"@typescript-eslint/parser": "3.2.0",
"eslint": "7.32.0",
"eslint-config-airbnb": "19.0.4",
"eslint-config-prettier": "8.7.0",
"eslint-plugin-import": "2.27.5",
"eslint-plugin-jsx-a11y": "6.6.1",
"eslint-plugin-react": "7.12.4",
"eslint-plugin-jsx-a11y": "6.7.1",
"eslint-plugin-react": "7.32.2",
"husky": "8.0.3",
"isomorphic-fetch": "3.0.0",
"jsdom": "21.1.0",
Expand Down
9 changes: 9 additions & 0 deletions packages/jaeger-ui/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,13 @@ module.exports = {
__REACT_APP_VSN_STATE__: false,
__APP_ENVIRONMENT__: false,
},
overrides: [
{
// Instruct ESLint not to lint build tooling files as part of the project.
files: ['vite.config.ts', 'src/vite-env.d.ts'],
parserOptions: {
project: null,
},
},
],
};
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ export default class DetailTableDropdown extends React.PureComponent<TProps> {
overlayClassName="DetailTableDropdown--Tooltip"
title={
<div className="DetailTableDropdown--Tooltip--Body">
<span>Apply changes to this column{"'"}s filter</span>
<span>Apply changes to this column&apos;s filter</span>
<span>Same effect as clicking outside the dropdown</span>
</div>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,7 @@ exports[`DetailTable render renders as expected 1`] = `
className="DetailTableDropdown--Tooltip--Body"
>
<span>
Apply changes to this column
'
s filter
Apply changes to this column's filter
</span>
<span>
Same effect as clicking outside the dropdown
Expand Down
2 changes: 2 additions & 0 deletions packages/jaeger-ui/src/reducers/metrics.mock.js
Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,7 @@ const serviceOpsCallsWithNull = {
},
{
gaugeValue: {
// eslint-disable-next-line no-loss-of-precision
doubleValue: 0.031052384663085615,
},
timestamp: '2021-09-13T12:01:36.235Z',
Expand Down Expand Up @@ -658,6 +659,7 @@ const serviceOpsCallsZeroDivision = {
},
{
gaugeValue: {
// eslint-disable-next-line no-loss-of-precision
doubleValue: 0.031052384663085615,
},
timestamp: '2021-09-13T12:01:36.235Z',
Expand Down
2 changes: 1 addition & 1 deletion packages/jaeger-ui/src/utils/config/process-scripts.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
import { getConfigValue } from './get-config';
import { TScript } from '../../types/config';

export default function () {
export default function processScripts() {
const scripts = getConfigValue('scripts');
if (scripts) {
scripts.forEach((script: TScript) => {
Expand Down
53 changes: 32 additions & 21 deletions packages/plexus/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,37 @@
// directory and all subdirectories.

module.exports = {
parser: '@typescript-eslint/parser',
parserOptions: {
project: './tsconfig.json',
tsconfigRootDir: '.',
},
plugins: ['@typescript-eslint'],
extends: [
'plugin:@typescript-eslint/recommended',
// placed after '@typescript-eslint/recommended' so TS formatting rules
// that conflict with prettier rules are overriden
'prettier/@typescript-eslint',
],
rules: {
// use @typescript-eslint/no-useless-constructor to avoid null error on *.d.ts files
'no-useless-constructor': 0,
overrides: [
{
files: ['*.ts', '*.tsx'],
parser: '@typescript-eslint/parser',
parserOptions: {
project: './tsconfig.json',
tsconfigRootDir: __dirname,
},
plugins: ['@typescript-eslint'],
extends: [
'plugin:@typescript-eslint/recommended',
// placed after '@typescript-eslint/recommended' so TS formatting rules
// that conflict with prettier rules are overriden
'prettier',
],
rules: {
// use @typescript-eslint/no-useless-constructor to avoid null error on *.d.ts files
'no-useless-constructor': 0,

'@typescript-eslint/explicit-function-return-type': 0,
'@typescript-eslint/explicit-member-accessibility': 0,
'@typescript-eslint/no-explicit-any': 0,
'@typescript-eslint/no-useless-constructor': 1,
'@typescript-eslint/prefer-interface': 0,

'@typescript-eslint/explicit-function-return-type': 0,
'@typescript-eslint/explicit-member-accessibility': 0,
'@typescript-eslint/no-explicit-any': 0,
'@typescript-eslint/no-useless-constructor': 1,
'@typescript-eslint/prefer-interface': 0,
},
// @typescript-eslint/eslint-plugin v2+ relaxations
'@typescript-eslint/explicit-module-boundary-types': 0,
'@typescript-eslint/ban-types': 0,
'@typescript-eslint/no-empty-function': 0,
'@typescript-eslint/no-inferrable-types': 0,
},
},
],
};
1 change: 1 addition & 0 deletions packages/plexus/src/Digraph/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export default class Digraph<T = unknown, U = unknown> extends React.PureCompone
> {
renderUtils: TRendererUtils;

// eslint-disable-next-line react/sort-comp
static propsFactories: Record<string, TFromGraphStateFn<any, any>> = {
classNameIsSmall,
scaleOpacity: scaleProperty.opacity,
Expand Down
1 change: 1 addition & 0 deletions packages/plexus/src/DirectedGraph/DirectedGraph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ export default class DirectedGraph<T> extends React.PureComponent<
rootSelection: any;
zoomManager: ZoomManager | null;

// eslint-disable-next-line react/sort-comp
static propsFactories = {
classNameIsSmall,
mergePropSetters,
Expand Down
Loading

0 comments on commit 9075aa4

Please sign in to comment.