Restore ESLint warnings as errors (ignoring them for now) (#4382)

Currently, `yarn lint` produces a bunch of warnings from ESLint. These warnings were added in a previous commit that upgraded ESLint packages as a way to avoid fixing lint violations created by the upgrade. However, this causes two problems: 1. They produce a lot of noise that makes it very difficult to find true errors, especially when looking at a CI run. 2. They allow new instances of these violations to show up (because warnings don't cause `eslint` to fail). This commit removes the overrides from the ESLint config for these rules so that they go back to being errors and adds `eslint-disable` directives above the lines that cause the violations. Note that this is not intended to be a long-term solution, and in the future, we should dedicate time to either fixing the violations or explaining why ignoring them is necessary.
MetaMask · Jun 10, 2024 · e361db6 · e361db6
1 parent 1fe8a66
commit e361db6
Show file tree

Hide file tree

Showing 117 changed files with 1,560 additions and 17 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -48,19 +48,13 @@ module.exports = {
         // TODO: auto-fix breaks stuff
         '@typescript-eslint/promise-function-async': 'off',
 
-        // TODO: re-enble most of these rules
-        '@typescript-eslint/await-thenable': 'warn',
-        '@typescript-eslint/naming-convention': 'off',
-        '@typescript-eslint/no-floating-promises': 'warn',
-        '@typescript-eslint/no-misused-promises': 'warn',
+        // TODO: re-enable most of these rules
         '@typescript-eslint/no-unnecessary-type-assertion': 'off',
         '@typescript-eslint/unbound-method': 'off',
         '@typescript-eslint/prefer-enum-initializers': 'off',
         '@typescript-eslint/prefer-nullish-coalescing': 'off',
         '@typescript-eslint/prefer-optional-chain': 'off',
         '@typescript-eslint/prefer-reduce-type-parameter': 'off',
-        '@typescript-eslint/restrict-plus-operands': 'warn',
-        '@typescript-eslint/restrict-template-expressions': 'warn',
         'no-restricted-syntax': 'off',
         'no-restricted-globals': 'off',
       },

diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts
@@ -189,7 +189,11 @@ function createExpectedInternalAccount({
 }): InternalAccount {
   const accountTypeToMethods = {
     [`${EthAccountType.Eoa}`]: [...Object.values(ETH_EOA_METHODS)],
+    // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+    // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
     [`${EthAccountType.Erc4337}`]: [...Object.values(ETH_ERC_4337_METHODS)],
+    // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+    // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
     [`${BtcAccountType.P2wpkh}`]: [...Object.values(BtcMethod)],
   };
 

diff --git a/packages/accounts-controller/src/AccountsController.ts b/packages/accounts-controller/src/AccountsController.ts
@@ -948,6 +948,8 @@ export class AccountsController extends BaseController<
    * @param metadataKey - The key of the metadata to retrieve.
    * @returns The value of the specified metadata key, or undefined if the account or metadata key does not exist.
    */
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
   #populateExistingMetadata<T extends keyof InternalAccount['metadata']>(
     accountId: string,
     metadataKey: T,

diff --git a/packages/address-book-controller/src/AddressBookController.ts b/packages/address-book-controller/src/AddressBookController.ts
@@ -27,8 +27,14 @@ export interface ContactEntry {
 }
 
 export enum AddressType {
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
   externallyOwnedAccounts = 'EXTERNALLY_OWNED_ACCOUNTS',
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
   contractAccounts = 'CONTRACT_ACCOUNTS',
+  // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+  // eslint-disable-next-line @typescript-eslint/naming-convention
   nonAccounts = 'NON_ACCOUNTS',
 }
 

diff --git a/packages/approval-controller/src/ApprovalController.test.ts b/packages/approval-controller/src/ApprovalController.test.ts
@@ -740,12 +740,16 @@ describe('approval controller', () => {
     });
 
     it('returns true for existing entry by origin', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE });
 
       expect(approvalController.has({ origin: 'bar.baz' })).toBe(true);
     });
 
     it('returns true for existing entry by origin and type', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' });
 
       expect(
@@ -754,24 +758,32 @@ describe('approval controller', () => {
     });
 
     it('returns true for existing type', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' });
 
       expect(approvalController.has({ type: 'myType' })).toBe(true);
     });
 
     it('returns false for non-existing entry by id', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE });
 
       expect(approvalController.has({ id: 'fizz' })).toBe(false);
     });
 
     it('returns false for non-existing entry by origin', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE });
 
       expect(approvalController.has({ origin: 'fizz.buzz' })).toBe(false);
     });
 
     it('returns false for non-existing entry by existing origin and non-existing type', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: TYPE });
 
       expect(
@@ -780,6 +792,8 @@ describe('approval controller', () => {
     });
 
     it('returns false for non-existing entry by non-existing origin and existing type', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType' });
 
       expect(
@@ -788,6 +802,8 @@ describe('approval controller', () => {
     });
 
     it('returns false for non-existing entry by type', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'myType1' });
 
       expect(approvalController.has({ type: 'myType2' })).toBe(false);
@@ -801,6 +817,8 @@ describe('approval controller', () => {
         origin: 'bar.baz',
         type: 'myType',
       });
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo', 'success');
 
       const result = await approvalPromise;
@@ -819,11 +837,15 @@ describe('approval controller', () => {
         type: 'myType2',
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo2', 'success2');
 
       let result = await approvalPromise2;
       expect(result).toBe('success2');
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo1', 'success1');
 
       result = await approvalPromise1;
@@ -886,6 +908,8 @@ describe('approval controller', () => {
         expectsResult: true,
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept(ID_MOCK, VALUE_MOCK);
 
       expect(await approvalPromise).toStrictEqual({
@@ -895,6 +919,8 @@ describe('approval controller', () => {
     });
 
     it('throws if accept wants to wait but request does not expect result', async () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({
         id: ID_MOCK,
         origin: ORIGIN_MOCK,
@@ -909,8 +935,12 @@ describe('approval controller', () => {
     });
 
     it('deletes entry', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type' });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo');
 
       expect(
@@ -922,9 +952,15 @@ describe('approval controller', () => {
     });
 
     it('deletes one entry out of many without side-effects', () => {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'foo', origin: 'bar.baz', type: 'type1' });
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({ id: 'fizz', origin: 'bar.baz', type: 'type2' });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('fizz');
 
       expect(
@@ -1034,6 +1070,8 @@ describe('approval controller', () => {
         type: 'myType4',
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo2', 'success2');
 
       let result = await promise2;
@@ -1048,6 +1086,8 @@ describe('approval controller', () => {
       expect(approvalController.has({ origin: 'fizz.buzz' })).toBe(false);
       expect(approvalController.has({ origin: 'bar.baz' })).toBe(true);
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept('foo1', 'success1');
 
       result = await promise1;
@@ -1154,6 +1194,8 @@ describe('approval controller', () => {
         showApprovalRequest,
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       messenger.call(
         'ApprovalController:addRequest',
         { id: 'foo', origin: 'bar.baz', type: TYPE },
@@ -1178,6 +1220,8 @@ describe('approval controller', () => {
         showApprovalRequest,
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       messenger.call(
         'ApprovalController:addRequest',
         { id: 'foo', origin: 'bar.baz', type: TYPE },
@@ -1202,6 +1246,8 @@ describe('approval controller', () => {
         showApprovalRequest,
       });
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.add({
         id: 'foo',
         origin: 'bar.baz',
@@ -1375,6 +1421,8 @@ describe('approval controller', () => {
         approvalController.state[PENDING_APPROVALS_STORE_KEY],
       )[0].id;
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept(resultRequestId);
       await promise;
 
@@ -1391,9 +1439,13 @@ describe('approval controller', () => {
     async function doesNotThrowIfAddingRequestFails(
       methodCallback: () => Promise<unknown>,
     ) {
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       methodCallback();
 
       // Second call will fail as mocked nanoid will generate the same ID.
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       methodCallback();
 
       expect(console.info).toHaveBeenCalledTimes(1);
@@ -1419,6 +1471,8 @@ describe('approval controller', () => {
         approvalController.state[PENDING_APPROVALS_STORE_KEY],
       )[0].id;
 
+      // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+      // eslint-disable-next-line @typescript-eslint/no-floating-promises
       approvalController.accept(resultRequestId);
       await promise;
 
@@ -1431,11 +1485,15 @@ describe('approval controller', () => {
 
     describe('success', () => {
       it('adds request with result success approval type', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.success(SUCCESS_OPTIONS_MOCK);
         expectRequestAdded(APPROVAL_TYPE_RESULT_SUCCESS, SUCCESS_OPTIONS_MOCK);
       });
 
       it('adds request with no options', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.success();
 
         expectRequestAdded(APPROVAL_TYPE_RESULT_SUCCESS, {
@@ -1447,6 +1505,8 @@ describe('approval controller', () => {
       });
 
       it('only includes relevant options in request data', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.success({
           ...SUCCESS_OPTIONS_MOCK,
           extra: 'testValue',
@@ -1460,6 +1520,8 @@ describe('approval controller', () => {
       });
 
       it('shows approval request', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.success(SUCCESS_OPTIONS_MOCK);
         expect(showApprovalRequest).toHaveBeenCalledTimes(1);
       });
@@ -1474,6 +1536,8 @@ describe('approval controller', () => {
       });
 
       it('does not throw if adding request fails', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         doesNotThrowIfAddingRequestFails(() =>
           approvalController.success(SUCCESS_OPTIONS_MOCK),
         );
@@ -1491,11 +1555,15 @@ describe('approval controller', () => {
 
     describe('error', () => {
       it('adds request with result error approval type', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.error(ERROR_OPTIONS_MOCK);
         expectRequestAdded(APPROVAL_TYPE_RESULT_ERROR, ERROR_OPTIONS_MOCK);
       });
 
       it('adds request with no options', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.error();
 
         expectRequestAdded(APPROVAL_TYPE_RESULT_ERROR, {
@@ -1507,6 +1575,8 @@ describe('approval controller', () => {
       });
 
       it('only includes relevant options in request data', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.error({
           ...ERROR_OPTIONS_MOCK,
           extra: 'testValue',
@@ -1520,6 +1590,8 @@ describe('approval controller', () => {
       });
 
       it('shows approval request', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         approvalController.error(ERROR_OPTIONS_MOCK);
         expect(showApprovalRequest).toHaveBeenCalledTimes(1);
       });
@@ -1534,6 +1606,8 @@ describe('approval controller', () => {
       });
 
       it('does not throw if adding request fails', async () => {
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/no-floating-promises
         doesNotThrowIfAddingRequestFails(() =>
           approvalController.error(ERROR_OPTIONS_MOCK),
         );

diff --git a/packages/approval-controller/src/ApprovalController.ts b/packages/approval-controller/src/ApprovalController.ts
@@ -379,6 +379,8 @@ export class ApprovalController extends BaseController<
 
     this.#approvals = new Map();
     this.#origins = new Map();
+    // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+    // eslint-disable-next-line @typescript-eslint/no-misused-promises
     this.#showApprovalRequest = showApprovalRequest;
     this.#typesExcludedFromRateLimiting = typesExcludedFromRateLimiting;
     this.registerMessageHandlers();
@@ -590,6 +592,8 @@ export class ApprovalController extends BaseController<
     if (origin) {
       return Array.from(
         (this.#origins.get(origin) || new Map()).values(),
+        // TODO: Either fix this lint violation or explain why it's necessary to ignore.
+        // eslint-disable-next-line @typescript-eslint/restrict-plus-operands
       ).reduce((total, value) => total + value, 0);
     }