From b74fbee1ada14f53b59a104b855f5186ad954ab9 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Fri, 10 Jan 2025 14:21:25 -0700 Subject: [PATCH] Fix ESLint config The upgrade to ESLint 9 which occurred in a previous commit was not done correctly. It seems that an array must be passed to `createConfig` instead of a series of arguments. As a result, a lot of lint rules were accidentally disabled. 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. --- docs/contributing.md | 4 +- eslint-warning-thresholds.json | 30 ++ eslint.config.mjs | 125 +++++--- package.json | 2 +- packages/controller-utils/package.json | 1 + packages/controller-utils/src/util.ts | 2 +- packages/json-rpc-engine/src/JsonRpcEngine.ts | 2 +- .../src/index.test.ts | 2 +- .../src/KeyringController.ts | 1 - .../src/AbstractMessageManager.ts | 2 +- .../src/create-network-client.ts | 2 - .../tests/NetworkController.test.ts | 7 +- .../services/push/push-web.ts | 2 +- .../src/PermissionController.ts | 1 - .../src/PhishingDetector.test.ts | 2 +- .../src/AbstractPollingController.ts | 1 - .../network-syncing/controller-integration.ts | 2 +- .../tests/SelectedNetworkController.test.ts | 2 +- .../src/SignatureController.ts | 4 +- .../src/TransactionController.ts | 3 +- .../src/helpers/GasFeePoller.ts | 2 +- .../src/helpers/IncomingTransactionHelper.ts | 2 +- .../src/helpers/MethodDataHelper.ts | 2 +- .../src/helpers/PendingTransactionTracker.ts | 2 +- .../src/UserOperationController.ts | 2 +- .../helpers/PendingUserOperationTracker.ts | 2 +- scripts/run-eslint.ts | 278 ++++++++++++++++++ yarn.config.cjs | 7 +- yarn.lock | 13 +- 29 files changed, 434 insertions(+), 73 deletions(-) create mode 100644 eslint-warning-thresholds.json create mode 100644 scripts/run-eslint.ts diff --git a/docs/contributing.md b/docs/contributing.md index 43b9230d95e..e9007a0ccf4 100644 --- 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 new file mode 100644 index 00000000000..3a25785ed45 --- /dev/null +++ b/eslint-warning-thresholds.json @@ -0,0 +1,30 @@ +{ + "@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-duplicates": 32, + "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 index ec01ff56f2b..5a8b36bd8e8 100644 --- 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,26 +58,36 @@ 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', @@ -89,18 +95,24 @@ const config = createConfig( }, }, { - 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: + // + projectService: false, + }, }, rules: { // Enable rules that are disabled in `@metamask/eslint-config-typescript` @@ -110,6 +122,7 @@ const config = createConfig( '@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,26 +131,52 @@ 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-duplicates': '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', }, }, { @@ -145,8 +184,20 @@ const config = createConfig( 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 index 93cc727ff10..613afb9417d 100644 --- 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 index 8f67f9cfc2b..b65d4fd9c93 100644 --- a/packages/controller-utils/package.json +++ b/packages/controller-utils/package.json @@ -64,6 +64,7 @@ "@types/jest": "^27.4.1", "deepmerge": "^4.2.2", "jest": "^27.5.1", + "jest-environment-jsdom": "^27.5.1", "nock": "^13.3.1", "ts-jest": "^27.1.4", "typedoc": "^0.24.8", diff --git a/packages/controller-utils/src/util.ts b/packages/controller-utils/src/util.ts index 14b41caedba..7f3358b64c9 100644 --- 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 index 62fb5204d0d..c589e13358d 100644 --- 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 index d770cad07b3..6395c629a73 100644 --- 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 index 4cc90868a8f..bf5b853aeeb 100644 --- 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 { - // 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 index bdd8401f54c..ab30ef705a7 100644 --- a/packages/message-manager/src/AbstractMessageManager.ts +++ b/packages/message-manager/src/AbstractMessageManager.ts @@ -3,7 +3,7 @@ import { BaseControllerV1 } from '@metamask/base-controller'; import type { ApprovalType } from '@metamask/controller-utils'; import type { Hex, 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 { 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 index dab4702a942..e6620184878 100644 --- 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; }): JsonRpcMiddleware { - // 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 index 3f224b7170f..009fdc95138 100644 --- a/packages/network-controller/tests/NetworkController.test.ts +++ b/packages/network-controller/tests/NetworkController.test.ts @@ -13069,6 +13069,7 @@ function refreshNetworkTests({ initialState?: Partial; operation: (controller: NetworkController) => Promise; }) { + // 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( { diff --git a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts index ccfab4e40f5..67b46f68d0c 100644 --- a/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts +++ b/packages/notification-services-controller/src/NotificationServicesPushController/services/push/push-web.ts @@ -20,7 +20,7 @@ import type { PushNotificationEnv } from '../../types/firebase'; declare const self: ServiceWorkerGlobalScope; // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let supportedCache: boolean | null = null; const getPushAvailability = async () => { diff --git a/packages/permission-controller/src/PermissionController.ts b/packages/permission-controller/src/PermissionController.ts index b5e33393f86..27cf026d018 100644 --- a/packages/permission-controller/src/PermissionController.ts +++ b/packages/permission-controller/src/PermissionController.ts @@ -1982,7 +1982,6 @@ export class PermissionController< target: string, ): void { if (!isPlainObject(caveat)) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal throw new InvalidCaveatError(caveat, origin, target); } diff --git a/packages/phishing-controller/src/PhishingDetector.test.ts b/packages/phishing-controller/src/PhishingDetector.test.ts index f76b8099650..38e2f2b8b67 100644 --- a/packages/phishing-controller/src/PhishingDetector.test.ts +++ b/packages/phishing-controller/src/PhishingDetector.test.ts @@ -1110,7 +1110,7 @@ describe('PhishingDetector', () => { ]; // CID should be blocked - for await (const entry of expectedToBeBlocked) { + for (const entry of expectedToBeBlocked) { await withPhishingDetector( [ { diff --git a/packages/polling-controller/src/AbstractPollingController.ts b/packages/polling-controller/src/AbstractPollingController.ts index d52aab938a0..f22455e4b13 100644 --- a/packages/polling-controller/src/AbstractPollingController.ts +++ b/packages/polling-controller/src/AbstractPollingController.ts @@ -82,7 +82,6 @@ export function AbstractPollingControllerBaseMixin< const callbacks = this.#callbacks.get(keyToDelete); if (callbacks) { for (const callback of callbacks) { - // eslint-disable-next-line n/callback-return callback(JSON.parse(keyToDelete)); } callbacks.clear(); diff --git a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts index 63df3b5721b..359520d8294 100644 --- a/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts +++ b/packages/profile-sync-controller/src/controllers/user-storage/network-syncing/controller-integration.ts @@ -29,7 +29,7 @@ type PerformMainNetworkSyncProps = { * Ensures that listeners do not fire during main sync (prevent double requests) */ // Exported to help testing -// eslint-disable-next-line import/no-mutable-exports +// eslint-disable-next-line import-x/no-mutable-exports export let isMainNetworkSyncInProgress = false; /** diff --git a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts index fd975016ca1..847f1b5898e 100644 --- a/packages/selected-network-controller/tests/SelectedNetworkController.test.ts +++ b/packages/selected-network-controller/tests/SelectedNetworkController.test.ts @@ -44,7 +44,7 @@ function buildMessenger() { * @param options.getSubjectNames - Permissions controller list of domains with permissions * @returns The network controller restricted messenger. */ -export function buildSelectedNetworkControllerMessenger({ +function buildSelectedNetworkControllerMessenger({ messenger = new ControllerMessenger< SelectedNetworkControllerActions | AllowedActions, SelectedNetworkControllerEvents | AllowedEvents diff --git a/packages/signature-controller/src/SignatureController.ts b/packages/signature-controller/src/SignatureController.ts index 54b8f1e63ec..239cd76386c 100644 --- a/packages/signature-controller/src/SignatureController.ts +++ b/packages/signature-controller/src/SignatureController.ts @@ -30,7 +30,7 @@ import { import type { NetworkControllerGetNetworkClientByIdAction } from '@metamask/network-controller'; import type { Hex, 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 { v1 as random } from 'uuid'; @@ -782,7 +782,6 @@ export class SignatureController extends BaseController< const originalStatus = metadata.status; - // eslint-disable-next-line n/callback-return callback(metadata); statusChanged = metadata.status !== originalStatus; @@ -802,7 +801,6 @@ export class SignatureController extends BaseController< #updateState(callback: (state: SignatureControllerState) => void) { return this.update((state) => { - // eslint-disable-next-line n/callback-return, n/no-callback-literal callback(state as unknown as SignatureControllerState); const unapprovedRequests = Object.values(state.signatureRequests).filter( diff --git a/packages/transaction-controller/src/TransactionController.ts b/packages/transaction-controller/src/TransactionController.ts index ff7c60f7eaf..a92967f2105 100644 --- a/packages/transaction-controller/src/TransactionController.ts +++ b/packages/transaction-controller/src/TransactionController.ts @@ -47,7 +47,7 @@ import type { Hex } from '@metamask/utils'; import { add0x, hexToNumber } from '@metamask/utils'; import { Mutex } from 'async-mutex'; // 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 { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash'; import { v1 as random } from 'uuid'; @@ -3495,7 +3495,6 @@ export class TransactionController extends BaseController< const originalTransactionMeta = cloneDeep(transactionMeta); - // eslint-disable-next-line n/callback-return transactionMeta = callback(transactionMeta) ?? transactionMeta; if (skipValidation !== true) { diff --git a/packages/transaction-controller/src/helpers/GasFeePoller.ts b/packages/transaction-controller/src/helpers/GasFeePoller.ts index b4b87a94dc0..82ad6090fef 100644 --- a/packages/transaction-controller/src/helpers/GasFeePoller.ts +++ b/packages/transaction-controller/src/helpers/GasFeePoller.ts @@ -7,7 +7,7 @@ import type { NetworkClientId, Provider } from '@metamask/network-controller'; import type { Hex } from '@metamask/utils'; import { createModuleLogger } 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 { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts index 172f0d8c003..f8d97dd5c4c 100644 --- a/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts +++ b/packages/transaction-controller/src/helpers/IncomingTransactionHelper.ts @@ -1,7 +1,7 @@ import type { AccountsController } from '@metamask/accounts-controller'; import type { Hex } 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 { incomingTransactionsLogger as log } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/MethodDataHelper.ts b/packages/transaction-controller/src/helpers/MethodDataHelper.ts index 54dbdb6ab4a..2542cb67178 100644 --- a/packages/transaction-controller/src/helpers/MethodDataHelper.ts +++ b/packages/transaction-controller/src/helpers/MethodDataHelper.ts @@ -3,7 +3,7 @@ import { createModuleLogger } from '@metamask/utils'; import { Mutex } from 'async-mutex'; import { MethodRegistry } from 'eth-method-registry'; // 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 { projectLogger } from '../logger'; diff --git a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts index 5520fe3a717..557a5d7c302 100644 --- a/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts +++ b/packages/transaction-controller/src/helpers/PendingTransactionTracker.ts @@ -5,7 +5,7 @@ import type { NetworkClientId, } from '@metamask/network-controller'; // 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 { cloneDeep, merge } from 'lodash'; diff --git a/packages/user-operation-controller/src/UserOperationController.ts b/packages/user-operation-controller/src/UserOperationController.ts index 2c70bd80c66..eede19f0893 100644 --- a/packages/user-operation-controller/src/UserOperationController.ts +++ b/packages/user-operation-controller/src/UserOperationController.ts @@ -26,7 +26,7 @@ import { } from '@metamask/transaction-controller'; import { add0x } 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 { Patch } from 'immer'; import { cloneDeep } from 'lodash'; diff --git a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts index 54fa8ce6023..551ac6736a1 100644 --- a/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts +++ b/packages/user-operation-controller/src/helpers/PendingUserOperationTracker.ts @@ -8,7 +8,7 @@ import type { import { BlockTrackerPollingControllerOnly } from '@metamask/polling-controller'; import { createModuleLogger, type Hex } 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 { projectLogger } from '../logger'; diff --git a/scripts/run-eslint.ts b/scripts/run-eslint.ts new file mode 100644 index 00000000000..7b1d7d12852 --- /dev/null +++ b/scripts/run-eslint.ts @@ -0,0 +1,278 @@ +import { ESLint } from 'eslint'; +import fs from 'fs'; +import path from 'path'; +import yargs from 'yargs'; + +const EXISTING_WARNINGS_FILE = path.resolve( + __dirname, + '../eslint-warning-thresholds.json', +); + +/** + * An object mapping rule IDs to their warning counts. + */ +type WarningCounts = Record; + +/** + * An object indicating the difference in warnings for a specific rule. + */ +type WarningComparison = { + /** The ID of the ESLint rule. */ + ruleId: string; + /** The previous count of warnings for the rule. */ + threshold: number; + /** The current count of warnings for the rule. */ + count: number; + /** The difference between the count and the threshold for the rule. */ + difference: number; +}; + +/** + * The warning severity of level of an ESLint rule. + */ +const WARNING = 1; + +// Run the script. +main().catch((error) => { + console.error(error); + process.exitCode = 1; +}); + +/** + * The entrypoint to this script. + */ +async function main() { + const { cache, fix, quiet } = parseCommandLineArguments(); + + const eslint = new ESLint({ cache, fix }); + const results = await runESLint(eslint, { fix, quiet }); + const hasErrors = results.some((result) => result.errorCount > 0); + + if (!quiet && !hasErrors) { + evaluateWarnings(results); + } +} + +/** + * Uses `yargs` to parse the arguments given to the script. + * + * @returns The parsed arguments. + */ +function parseCommandLineArguments() { + return yargs(process.argv.slice(2)) + .option('cache', { + type: 'boolean', + description: 'Cache results to speed up future runs', + default: false, + }) + .option('fix', { + type: 'boolean', + description: 'Automatically fix problems', + default: false, + }) + .option('quiet', { + type: 'boolean', + description: + 'Only report errors, disabling the warnings quality gate in the process', + default: false, + }) + .help().argv; +} + +/** + * Runs ESLint on the project files. + * + * @param eslint - The ESLint instance. + * @param options - The options for running ESLint. + * @param options.quiet - Whether to only report errors (true) or not (false). + * @param options.fix - Whether to automatically fix problems (true) or not + * (false). + * @returns A promise that resolves to the lint results. + */ +async function runESLint( + eslint: ESLint, + options: { quiet: boolean; fix: boolean }, +): Promise { + let results = await eslint.lintFiles(['.']); + + const formatter = await eslint.loadFormatter('stylish'); + const resultText = formatter.format(results); + console.log(resultText); + + if (options.quiet) { + results = ESLint.getErrorResults(results); + } + + if (options.fix) { + await ESLint.outputFixes(results); + } + + return results; +} + +/** + * This function represents the ESLint warnings quality gate, which will cause + * linting to pass or fail depending on how many new warnings have been + * produced. + * + * - If we have no record of warnings from a previous run, then we simply + * capture the new warnings in a file and continue. + * - If we have a record of warnings from a previous run and there are any + * changes to the number of warnings overall, then we list which ESLint rules + * had increases and decreases. If are were more warnings overall then we fail, + * otherwise we pass. + * + * @param results - The results of running ESLint. + */ +function evaluateWarnings(results: ESLint.LintResult[]) { + const warningThresholds = loadWarningThresholds(); + const warningCounts = getWarningCounts(results); + + if (Object.keys(warningThresholds).length === 0) { + console.log( + 'The following ESLint warnings were produced and will be captured as thresholds for future runs:\n', + ); + for (const [ruleId, count] of Object.entries(warningCounts)) { + console.log(`- ${ruleId}: ${count}`); + } + saveWarningThresholds(warningCounts); + } else { + const comparisons = compareWarnings(warningThresholds, warningCounts); + + const changes = comparisons.filter( + (comparison) => comparison.difference !== 0, + ); + const regressionDetected = comparisons.filter( + (comparison) => comparison.difference > 0, + ); + + if (changes.length > 0) { + if (regressionDetected) { + console.log( + '🛑 New ESLint warnings have been introduced and need to be resolved for linting to pass:\n', + ); + process.exitCode = 1; + } else { + console.log( + '🛑 The overall number of ESLint warnings have decreased, good work! ❤️ Here are the differences:\n', + ); + } + for (const { ruleId, threshold, count, difference } of changes) { + console.log( + `- ${ruleId}: ${threshold} -> ${count} (${difference > 0 ? '+' : '-'}${difference})`, + ); + } + saveWarningThresholds(warningCounts); + } + } +} + +/** + * Loads previous warning counts from a file. + * + * @returns An object mapping rule IDs to their previous warning counts. + */ +function loadWarningThresholds(): WarningCounts { + if (fs.existsSync(EXISTING_WARNINGS_FILE)) { + const data = fs.readFileSync(EXISTING_WARNINGS_FILE, 'utf-8'); + return JSON.parse(data); + } + return {}; +} + +/** + * Saves current warning counts to a file so they can be used for a future run. + * + * @param warningCounts - An object mapping rule IDs to their current warning + * counts. + */ +function saveWarningThresholds(warningCounts: WarningCounts): void { + fs.writeFileSync( + EXISTING_WARNINGS_FILE, + `${JSON.stringify(warningCounts, null, 2)}\n`, + 'utf-8', + ); +} + +/** + * Given a list of results from an the ESLint run, counts the number of warnings + * produced per rule. + * + * @param results - The ESLint results. + * @returns An object mapping rule IDs to their warning counts, sorted by rule + * ID. + */ +function getWarningCounts(results: ESLint.LintResult[]): WarningCounts { + const warningCounts = results.reduce((acc, result) => { + for (const message of result.messages) { + if (message.severity === WARNING && message.ruleId) { + acc[message.ruleId] = (acc[message.ruleId] ?? 0) + 1; + } + } + return acc; + }, {} as WarningCounts); + + return Object.keys(warningCounts) + .sort(sortRules) + .reduce((sortedWarningCounts, key) => { + return { ...sortedWarningCounts, [key]: warningCounts[key] }; + }, {} as WarningCounts); +} + +/** + * Compares previous and current warning counts. + * + * @param warningThresholds - An object mapping rule IDs to the warning + * thresholds established in a previous run. + * @param warningCounts - An object mapping rule IDs to the current warning + * counts. + * @returns An array of objects indicating comparisons in warnings. + */ +function compareWarnings( + warningThresholds: WarningCounts, + warningCounts: WarningCounts, +): WarningComparison[] { + return Object.entries(warningCounts) + .map(([ruleId, count]) => { + const threshold = warningThresholds[ruleId] ?? 0; + const difference = count - threshold; + return { ruleId, threshold, count, difference }; + }) + .sort((a, b) => sortRules(a.ruleId, b.ruleId)); +} + +/** + * Sorts rule IDs, ensuring that rules with namespaces appear before rules + * without. + * + * @param ruleIdA - The first rule ID. + * @param ruleIdB - The second rule ID. + * @returns A negative number if the first rule ID should come before the + * second, a positive number if the first should come _after_ the second, or 0 + * if they should stay where they are. + * @example + * ``` typescript + * sortRules( + * '@typescript-eslint/naming-convention', + * '@typescript-eslint/explicit-function-return-type' + * ) //=> 1 (sort A after B) + * sortRules( + * 'explicit-function-return-type', + * '@typescript-eslint/naming-convention' + * ) //=> 1 (sort A after B) + */ +function sortRules(ruleIdA: string, ruleIdB: string): number { + const [namespaceA, ruleA] = ruleIdA.includes('/') + ? ruleIdA.split('/') + : ['', ruleIdA]; + const [namespaceB, ruleB] = ruleIdB.includes('/') + ? ruleIdB.split('/') + : ['', ruleIdB]; + if (namespaceA && !namespaceB) { + return -1; + } + if (!namespaceA && namespaceB) { + return 1; + } + return namespaceA.localeCompare(namespaceB) || ruleA.localeCompare(ruleB); +} diff --git a/yarn.config.cjs b/yarn.config.cjs index b802565a6a0..aa8f334e172 100644 --- a/yarn.config.cjs +++ b/yarn.config.cjs @@ -1,4 +1,9 @@ -// @ts-check +/* @ts-check */ +// The ESLint JSDoc plugin usually disables this rule for TypeScript files, +// but for JavaScript files we are typechecking, we need to disable it manually. +// See: +/* eslint-disable jsdoc/no-undefined-types */ + // This file is used to define, among other configuration, rules that Yarn will // execute when you run `yarn constraints`. These rules primarily check the // manifests of each package in the monorepo to ensure they follow a standard diff --git a/yarn.lock b/yarn.lock index 62f430f3b91..5286b058daf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2594,6 +2594,7 @@ __metadata: eth-ens-namehash: "npm:^2.0.8" fast-deep-equal: "npm:^3.1.3" jest: "npm:^27.5.1" + jest-environment-jsdom: "npm:^27.5.1" nock: "npm:^13.3.1" ts-jest: "npm:^27.1.4" typedoc: "npm:^0.24.8" @@ -5822,11 +5823,11 @@ __metadata: linkType: hard "acorn-walk@npm:^8.1.1": - version: 8.3.3 - resolution: "acorn-walk@npm:8.3.3" + version: 8.3.4 + resolution: "acorn-walk@npm:8.3.4" dependencies: acorn: "npm:^8.11.0" - checksum: 10/59701dcb7070679622ba8e9c7f37577b4935565747ca0fd7c1c3ad30b3f1b1b008276282664e323b5495eb49f77fa12d3816fd06dc68e18f90fbebe759f71450 + checksum: 10/871386764e1451c637bb8ab9f76f4995d408057e9909be6fb5ad68537ae3375d85e6a6f170b98989f44ab3ff6c74ad120bc2779a3d577606e7a0cd2b4efcaf77 languageName: node linkType: hard @@ -10971,9 +10972,9 @@ __metadata: linkType: hard "nwsapi@npm:^2.2.0": - version: 2.2.12 - resolution: "nwsapi@npm:2.2.12" - checksum: 10/172119e9ef492467ebfb337f9b5fd12a94d2b519377cde3f6ec2f74a86f6d5c00ef3873539bed7142f908ffca4e35383179be2319d04a563071d146bfa3f1673 + version: 2.2.16 + resolution: "nwsapi@npm:2.2.16" + checksum: 10/1e5e086cdd4ca4a45f414d37f49bf0ca81d84ed31c6871ac68f531917d2910845db61f77c6d844430dc90fda202d43fce9603024e74038675de95229eb834dba languageName: node linkType: hard