Fix ESLint config (#5132)

The upgrade to ESLint 9 which occurred in a previous commit was not performed correctly. It seems that an array must be passed to `createConfig` instead of a series of arguments, and that was not done. As a result, a lot of lint rules were disabled accidentally. This commit fixes the ESLint config and corrects any lint violations. Unsurprisingly, upgrading the ESLint packages created a bunch of new lint violations, so those have been turned into warnings rather than errors in this commit. To prevent new instances of these violations from occurring, however, a script has been added which acts as a quality gate, running `eslint` as before, but then capturing existing warning counts and comparing them to a previously generated threshold file, exiting with non-zero if there is an increase in warnings for any particular ESLint rule. This script is also now used for `yarn lint:eslint` so that CI will fail if the count increases.
MetaMask · Jan 14, 2025 · 7409b3f · 7409b3f
1 parent c5d49d7
commit 7409b3f
Show file tree

Hide file tree

Showing 29 changed files with 448 additions and 73 deletions.
diff --git a/docs/contributing.md b/docs/contributing.md
@@ -46,9 +46,9 @@ If you need to customize the behavior of Jest for a package, see `jest.config.js
 
 ## Linting
 
-[ESLint](https://eslint.org/docs/v8.x/) v8 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files.
+[ESLint](https://eslint.org) v9 (via [MetaMask's shared ESLint configurations](https://github.com/MetaMask/eslint-config)) is used to check for code quality issues, and [Prettier](https://prettier.io/docs/en/) is used to format files.
 
-If you need to customize the behavior of ESLint, see `.eslintrc.js` in the root.
+If you need to customize the behavior of ESLint, see `eslint.config.mjs` in the root.
 
 - Run `yarn lint` to lint all files and show possible violations across the monorepo.
 - Run `yarn lint:fix` to fix any automatically fixable violations.

diff --git a/eslint-warning-thresholds.json b/eslint-warning-thresholds.json
@@ -0,0 +1,29 @@
+{
+  "@typescript-eslint/consistent-type-exports": 19,
+  "@typescript-eslint/no-base-to-string": 3,
+  "@typescript-eslint/no-duplicate-enum-values": 2,
+  "@typescript-eslint/no-misused-promises": 3,
+  "@typescript-eslint/no-unsafe-enum-comparison": 59,
+  "@typescript-eslint/no-unused-vars": 36,
+  "@typescript-eslint/prefer-promise-reject-errors": 13,
+  "@typescript-eslint/prefer-readonly": 152,
+  "@typescript-eslint/switch-exhaustiveness-check": 10,
+  "import-x/namespace": 189,
+  "import-x/no-named-as-default": 1,
+  "import-x/no-named-as-default-member": 8,
+  "import-x/order": 209,
+  "jest/no-conditional-in-test": 104,
+  "jsdoc/check-tag-names": 372,
+  "jsdoc/require-returns": 22,
+  "jsdoc/tag-lines": 329,
+  "n/no-unsupported-features/node-builtins": 18,
+  "n/prefer-global/text-encoder": 4,
+  "n/prefer-global/text-decoder": 4,
+  "prettier/prettier": 116,
+  "promise/always-return": 3,
+  "promise/catch-or-return": 2,
+  "promise/param-names": 8,
+  "no-empty-function": 2,
+  "no-shadow": 8,
+  "no-unused-private-class-members": 6
+}
diff --git a/eslint.config.mjs b/eslint.config.mjs
@@ -1,19 +1,12 @@
 import base, { createConfig } from '@metamask/eslint-config';
-import nodejs from '@metamask/eslint-config-nodejs';
 import jest from '@metamask/eslint-config-jest';
+import nodejs from '@metamask/eslint-config-nodejs';
 import typescript from '@metamask/eslint-config-typescript';
 
-const config = createConfig(
+const config = createConfig([
+  ...base,
   {
     ignores: [
-      'yarn.lock',
-      '**/**.map',
-      '**/**.tsbuildinfo',
-      '**/*.json',
-      '**/*.md',
-      '**/LICENSE',
-      '**/*.sh',
-      '**/.DS_Store',
       '**/dist/**',
       '**/docs/**',
       '**/coverage/**',
@@ -22,7 +15,6 @@ const config = createConfig(
       'scripts/create-package/package-template/**',
     ],
   },
-  ...base,
   {
     rules: {
       // Left disabled because various properties throughough this repo are snake_case because the
@@ -32,14 +24,12 @@ const config = createConfig(
       'id-length': 'off',
 
       // TODO: re-enble most of these rules
-      '@typescript-eslint/naming-convention': 'off',
       'function-paren-newline': 'off',
       'id-denylist': 'off',
       'implicit-arrow-linebreak': 'off',
-      'import/no-anonymous-default-export': 'off',
-      'import/no-unassigned-import': 'off',
+      'import-x/no-anonymous-default-export': 'off',
+      'import-x/no-unassigned-import': 'off',
       'lines-around-comment': 'off',
-      'n/no-sync': 'off',
       'no-async-promise-executor': 'off',
       'no-case-declarations': 'off',
       'no-invalid-this': 'off',
@@ -53,6 +43,12 @@ const config = createConfig(
         'off',
         { matchDescription: '^[A-Z`\\d_][\\s\\S]*[.?!`>)}]$' },
       ],
+
+      // TODO: These rules created more errors after the upgrade to ESLint 9.
+      // Re-enable these rules and address any lint violations.
+      'import-x/no-named-as-default-member': 'warn',
+      'prettier/prettier': 'warn',
+      'no-empty-function': 'warn',
     },
     settings: {
       jsdoc: {
@@ -62,54 +58,76 @@ const config = createConfig(
   },
   {
     files: [
-      '**/jest.config.js',
-      '**/jest.environment.js',
-      '**/tests/**/*.{ts,js}',
-      '*.js',
-      '*.test.{ts,js}',
+      '**/*.{js,cjs,mjs}',
+      '**/*.test.{js,ts}',
+      '**/tests/**/*.{js,ts}',
       'scripts/*.ts',
-      'scripts/create-package/*.ts',
-      'yarn.config.cjs',
+      'scripts/create-package/**/*.ts',
     ],
     extends: [nodejs],
+    rules: {
+      // TODO: Re-enable this
+      'n/no-sync': 'off',
+      // TODO: These rules created more errors after the upgrade to ESLint 9.
+      // Re-enable these rules and address any lint violations.
+      'n/no-unsupported-features/node-builtins': 'warn',
+    },
   },
   {
-    files: ['*.test.{ts,js}', '**/tests/**/*.{ts,js}'],
+    files: ['**/*.test.{js,ts}', '**/tests/**/*.{js,ts}'],
     extends: [jest],
+    rules: {
+      // TODO: These rules created more errors after the upgrade to ESLint 9.
+      // Re-enable these rules and address any lint violations.
+      'jest/no-conditional-in-test': 'warn',
+    },
   },
   {
     // These files are test helpers, not tests. We still use the Jest ESLint
     // config here to ensure that ESLint expects a test-like environment, but
     // various rules meant just to apply to tests have been disabled.
-    files: ['**/tests/**/*.{ts,js}', '!*.test.{ts,js}'],
+    files: ['**/tests/**/*.{js,ts}'],
+    ignores: ['**/*.test.{js,ts}'],
     rules: {
       'jest/no-export': 'off',
       'jest/require-top-level-describe': 'off',
       'jest/no-if': 'off',
     },
   },
   {
-    files: ['*.js', '*.cjs'],
-    parserOptions: {
+    files: ['**/*.{js,cjs}'],
+    languageOptions: {
       sourceType: 'script',
-      ecmaVersion: '2020',
+      ecmaVersion: 2020,
     },
   },
   {
-    files: ['*.ts'],
+    files: ['**/*.ts'],
     extends: [typescript],
-    parserOptions: {
-      tsconfigRootDir: import.meta.dirname,
-      project: ['./tsconfig.packages.json'],
+    languageOptions: {
+      parserOptions: {
+        tsconfigRootDir: import.meta.dirname,
+        project: './tsconfig.packages.json',
+        // Disable `projectService` because we run into out-of-memory issues.
+        // See this ticket for inspiration out how to solve this:
+        // <https://github.com/typescript-eslint/typescript-eslint/issues/1192>
+        projectService: false,
+      },
     },
     rules: {
+      // This rule does not detect multiple imports of the same file where types
+      // are being imported in one case and runtime values are being imported in
+      // another
+      'import-x/no-duplicates': 'off',
+
       // Enable rules that are disabled in `@metamask/eslint-config-typescript`
       '@typescript-eslint/no-explicit-any': 'error',
 
       // TODO: auto-fix breaks stuff
       '@typescript-eslint/promise-function-async': 'off',
 
       // TODO: re-enable most of these rules
+      '@typescript-eslint/naming-convention': 'off',
       '@typescript-eslint/no-unnecessary-type-assertion': 'off',
       '@typescript-eslint/unbound-method': 'off',
       '@typescript-eslint/prefer-enum-initializers': 'off',
@@ -118,35 +136,72 @@ const config = createConfig(
       '@typescript-eslint/prefer-reduce-type-parameter': 'off',
       'no-restricted-syntax': 'off',
       'no-restricted-globals': 'off',
+
+      // TODO: These rules created more errors after the upgrade to ESLint 9.
+      // Re-enable these rules and address any lint violations.
+      '@typescript-eslint/consistent-type-exports': 'warn',
+      '@typescript-eslint/explicit-function-return-type': 'off',
+      '@typescript-eslint/no-base-to-string': 'warn',
+      '@typescript-eslint/no-duplicate-enum-values': 'warn',
+      '@typescript-eslint/no-misused-promises': 'warn',
+      '@typescript-eslint/no-unsafe-enum-comparison': 'warn',
+      '@typescript-eslint/no-unused-vars': 'warn',
+      '@typescript-eslint/only-throw-error': 'warn',
+      '@typescript-eslint/prefer-promise-reject-errors': 'warn',
+      '@typescript-eslint/prefer-readonly': 'warn',
+      '@typescript-eslint/switch-exhaustiveness-check': 'warn',
+      'import-x/namespace': 'warn',
+      'import-x/no-named-as-default': 'warn',
+      'import-x/order': 'warn',
+      'jsdoc/check-tag-names': 'warn',
+      'jsdoc/require-returns': 'warn',
+      'jsdoc/tag-lines': 'warn',
+      'no-unused-private-class-members': 'warn',
+      'promise/always-return': 'warn',
+      'promise/catch-or-return': 'warn',
+      'promise/param-names': 'warn',
     },
   },
   {
     files: ['tests/setupAfterEnv/matchers.ts'],
-    parserOptions: {
+    languageOptions: {
       sourceType: 'script',
     },
   },
+  // This should really be in `@metamask/eslint-config-typescript`
   {
-    files: ['*.d.ts'],
+    files: ['**/*.d.ts'],
     rules: {
       '@typescript-eslint/naming-convention': 'warn',
-      'import/unambiguous': 'off',
+      'import-x/unambiguous': 'off',
     },
   },
   {
     files: ['scripts/*.ts'],
     rules: {
-      // All scripts will have shebangs.
-      'n/shebang': 'off',
+      // Scripts may be self-executable and thus have hashbangs.
+      'n/hashbang': 'off',
     },
   },
   {
     files: ['**/jest.environment.js'],
     rules: {
       // These files run under Node, and thus `require(...)` is expected.
       'n/global-require': 'off',
+
+      // TODO: These rules created more errors after the upgrade to ESLint 9.
+      // Re-enable these rules and address any lint violations.
+      'n/prefer-global/text-encoder': 'warn',
+      'n/prefer-global/text-decoder': 'warn',
+      'no-shadow': 'warn',
+    },
+  },
+  {
+    files: ['**/*.mjs'],
+    languageOptions: {
+      sourceType: 'module',
     },
   },
-);
+]);
 
 export default config;
diff --git a/package.json b/package.json
@@ -23,7 +23,7 @@
     "lint": "yarn lint:eslint && yarn lint:misc --check && yarn constraints && yarn lint:dependencies && yarn lint:teams",
     "lint:dependencies": "depcheck && yarn dedupe --check",
     "lint:dependencies:fix": "depcheck && yarn dedupe",
-    "lint:eslint": "eslint . --cache",
+    "lint:eslint": "yarn ts-node ./scripts/run-eslint.ts --cache",
     "lint:fix": "yarn lint:eslint --fix && yarn lint:misc --write && yarn constraints --fix && yarn lint:dependencies:fix",
     "lint:misc": "prettier --no-error-on-unmatched-pattern '**/*.json' '**/*.md' '**/*.yml' '!.yarnrc.yml' '!merged-packages/**' --ignore-path .gitignore",
     "lint:teams": "ts-node scripts/lint-teams-json.ts",

diff --git a/packages/controller-utils/package.json b/packages/controller-utils/package.json
@@ -65,6 +65,7 @@
     "@types/jest": "^27.4.1",
     "deepmerge": "^4.2.2",
     "jest": "^27.5.1",
+    "jest-environment-jsdom": "^27.5.1",
     "nock": "^13.3.1",
     "sinon": "^9.2.4",
     "ts-jest": "^27.1.4",

diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts
@@ -618,7 +618,7 @@ function logOrRethrowError(error: unknown, codesToCatch: number[] = []) {
       throw error;
     }
   } else {
-    // eslint-disable-next-line @typescript-eslint/no-throw-literal
+    // eslint-disable-next-line @typescript-eslint/only-throw-error
     throw error;
   }
 }

diff --git a/packages/json-rpc-engine/src/JsonRpcEngine.ts b/packages/json-rpc-engine/src/JsonRpcEngine.ts
@@ -489,7 +489,7 @@ export class JsonRpcEngine extends SafeEventEmitter {
     // Now we re-throw the middleware processing error, if any, to catch it
     // further up the call chain.
     if (error) {
-      // eslint-disable-next-line @typescript-eslint/no-throw-literal
+      // eslint-disable-next-line @typescript-eslint/only-throw-error
       throw error;
     }
   }

diff --git a/packages/json-rpc-middleware-stream/src/index.test.ts b/packages/json-rpc-middleware-stream/src/index.test.ts
@@ -8,7 +8,7 @@ import { createStreamMiddleware, createEngineStream } from '.';
 const artificialDelay = async (time = 0) =>
   new Promise((resolve) => setTimeout(resolve, time));
 // TODO: Replace `any` with type
-// eslint-disable-next-line @typescript-eslint/no-empty-function, @typescript-eslint/no-explicit-any
+// eslint-disable-next-line @typescript-eslint/no-empty-function,@typescript-eslint/no-explicit-any
 const noop = function (_a: any) {};
 
 const jsonrpc = '2.0' as const;

diff --git a/packages/keyring-controller/src/KeyringController.ts b/packages/keyring-controller/src/KeyringController.ts
@@ -1580,7 +1580,6 @@ export class KeyringController extends BaseController<
    * @deprecated Use `withKeyring` instead.
    */
   async cancelQRSynchronization(): Promise<void> {
-    // eslint-disable-next-line n/no-sync
     (await this.getOrAddQRKeyring()).cancelSync();
   }
 

diff --git a/packages/message-manager/src/AbstractMessageManager.ts b/packages/message-manager/src/AbstractMessageManager.ts
@@ -7,7 +7,7 @@ import type {
 import type { ApprovalType } from '@metamask/controller-utils';
 import type { Json } from '@metamask/utils';
 // This package purposefully relies on Node's EventEmitter module.
-// eslint-disable-next-line import/no-nodejs-modules
+// eslint-disable-next-line import-x/no-nodejs-modules
 import { EventEmitter } from 'events';
 import type { Draft } from 'immer';
 import { v1 as random } from 'uuid';

diff --git a/packages/network-controller/src/create-network-client.ts b/packages/network-controller/src/create-network-client.ts
@@ -71,7 +71,6 @@ export function createNetworkClient(
   const rpcProvider = providerFromMiddleware(rpcApiMiddleware);
 
   const blockTrackerOpts =
-    // eslint-disable-next-line n/no-process-env
     process.env.IN_TEST && networkConfig.type === 'custom'
       ? { pollingInterval: SECOND }
       : {};
@@ -190,7 +189,6 @@ function createCustomNetworkMiddleware({
   chainId: Hex;
   rpcApiMiddleware: JsonRpcMiddleware<JsonRpcParams, Json>;
 }): JsonRpcMiddleware<JsonRpcParams, Json> {
-  // eslint-disable-next-line n/no-process-env
   const testMiddlewares = process.env.IN_TEST
     ? [createEstimateGasDelayTestMiddleware()]
     : [];

diff --git a/packages/network-controller/tests/NetworkController.test.ts b/packages/network-controller/tests/NetworkController.test.ts
@@ -13069,6 +13069,7 @@ function refreshNetworkTests({
   initialState?: Partial<NetworkState>;
   operation: (controller: NetworkController) => Promise<void>;
 }) {
+  // eslint-disable-next-line jest/require-top-level-describe
   it('emits networkWillChange with state payload', async () => {
     await withController(
       {
@@ -13097,6 +13098,7 @@ function refreshNetworkTests({
     );
   });
 
+  // eslint-disable-next-line jest/require-top-level-describe
   it('emits networkDidChange with state payload', async () => {
     await withController(
       {
@@ -13126,6 +13128,7 @@ function refreshNetworkTests({
   });
 
   if (expectedNetworkClientConfiguration.type === NetworkClientType.Custom) {
+    // eslint-disable-next-line jest/require-top-level-describe
     it('sets the provider to a custom RPC provider initialized with the RPC target and chain ID', async () => {
       await withController(
         {
@@ -13167,8 +13170,7 @@ function refreshNetworkTests({
       );
     });
   } else {
-    // TODO: Either fix this lint violation or explain why it's necessary to ignore.
-    // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
+    // eslint-disable-next-line jest/require-top-level-describe
     it(`sets the provider to an Infura provider pointed to ${expectedNetworkClientConfiguration.network}`, async () => {
       await withController(
         {
@@ -13209,6 +13211,7 @@ function refreshNetworkTests({
     });
   }
 
+  // eslint-disable-next-line jest/require-top-level-describe
   it('replaces the provider object underlying the provider proxy without creating a new instance of the proxy itself', async () => {
     await withController(
       {