Bump ESLint and related dependencies, and fix lint errors by Mrtenz · Pull Request #1014 · MetaMask/core

Mrtenz · 2022-12-09T12:36:52Z

This bumps ESLint and related dependencies to the latest version. I've also removed some custom rules from the ESLint config, as they seem to be unnecessary.

Packages to fix:

Breaking changes

While I tried to keep breaking changes to a minimum, there are a few:

Certain enum keys have been renamed. The values are still the same, however.
All methods with the private modifier have been changed to use a hash name instead, making them truly private. Any code relying on calling these private methods (by suppressing TypeScript errors), will break.

Mrtenz · 2022-12-09T14:16:30Z

packages/keyring-controller/src/KeyringController.ts

-      this.updateIdentities(await this.#keyring.getAccounts());
-      this.fullUpdate();
+      this.#updateIdentities(await this.#keyring.getAccounts());
+      await this.fullUpdate();


This potentially changes the behaviour of this function. We may want to do this instead:

Suggested change

await this.fullUpdate();

this.fullUpdate().catch(console.error);

This makes sense to me.

Mrtenz · 2022-12-09T14:16:36Z

packages/keyring-controller/src/KeyringController.ts

-      this.updateIdentities(await this.#keyring.getAccounts());
-      this.fullUpdate();
+      this.#updateIdentities(await this.#keyring.getAccounts());
+      await this.fullUpdate();


This potentially changes the behaviour of this function. We may want to do this instead:

Suggested change

await this.fullUpdate();

this.fullUpdate().catch(console.error);

Makes sense to me.

Mrtenz · 2022-12-09T14:18:07Z

packages/keyring-controller/src/KeyringController.ts

  type: string;
  accounts: string[];
-  getAccounts(): string[];
+  getAccounts(): string[] | Promise<string[]>;


This is an assumption I made, since getAccounts() is awaited further on.

Mrtenz · 2022-12-09T15:32:13Z

packages/approval-controller/src/ApprovalController.ts

      opts.requestData,
    );
-    this._showApprovalRequest();
+    await this.#showApprovalRequest();


This potentially changes the behaviour of this function. We may want to do this instead:

Suggested change

await this.#showApprovalRequest();

this.#showApprovalRequest().catch(console.error);

I think this makes sense.

Aside: Original looks like it could be a mistake? 🤔

I'd expect the two promises (this.#showApprovalRequest and this.#add) to be either serially resolved (await or then), or returned together via a Promise.all?

It may be, but that's probably something we can decide later.

legobeat · 2022-12-10T00:14:31Z

Can the (potential) behavior changes be replaced with eslint-ignore directives for this PR to make it purely about linting, and split them out into one or a couple separate PRs?

mcmire

Good work on this. Most of the comments here are minor but there are a few cases that are more blocking than others.

packages/assets-controllers/src/AccountTrackerController.ts

packages/assets-controllers/src/NftDetectionController.ts

mcmire · 2022-12-13T22:46:01Z

packages/assets-controllers/src/TokensController.test.ts

        'Invalid decimals "-1": must be 0 <= 36.',
      );

+      // eslint-disable-next-line require-atomic-updates


What's the lint message here? Wondering if there is a way to fix this other than bypassing.

ESLint: Possible race condition: asset.decimals might be assigned based on an outdated state of asset.(require-atomic-updates)

packages/assets-controllers/src/TokensController.ts

packages/assets-controllers/src/token-service.ts

mcmire · 2022-12-13T23:17:58Z

packages/subject-metadata-controller/src/SubjectMetadataController.ts

+    this.#subjectsWithoutPermissionsEncounteredSinceStartup.add(origin);

    this.update((draftState) => {
      // Typecast: ts(2589)


Does this comment still apply?

Nope, I will remove it.

mcmire · 2022-12-13T23:18:18Z

packages/subject-metadata-controller/src/SubjectMetadataController.ts

   * Stores domain metadata for the given origin (subject). Deletes metadata for
   * subjects without permissions in a FIFO manner once more than
-   * {@link SubjectMetadataController.subjectCacheLimit} distinct origins have
+   * {@link SubjectMetadataController.#subjectCacheLimit} distinct origins have


Is this worth linking to if it's a private method? (Such a link may not be accessible in the generated docs.)

There may be other examples of this that I came across but didn't comment on. I'll take a look later to see if I can find them.

Good point, probably not. I think either way (even with the private modifier), this wouldn't be included in the doc.

mcmire · 2022-12-13T23:19:50Z

packages/transaction-controller/src/TransactionController.ts

+      chainId: parseInt(chainId, NaN),
+      networkId: parseInt(networkId, NaN),


Is NaN appropriate here? Should this be 16 or 10?

I'm not sure. NaN is the equivalent of undefined, so that's why I used that.

Interesting... I did not know that.

tsconfig.json

mcmire · 2022-12-13T23:22:18Z

tsconfig.json

    "esModuleInterop": true,
-    "noEmit": true
+    "noEmit": true,
+    "strictNullChecks": true


Huh... I thought this was covered by strict.

@typescript-eslint was complaining without this.

This is covered by strict, but we don't have strict enabled here. It's just in the packages tsconfig.

Ah. Makes sense.

mcmire · 2022-12-20T17:50:01Z

@Mrtenz I would love to see this merged! That said, I do agree with @legobeat in that the breaking changes in this PR are a bit difficult to discern due to the size. I also wonder if it would be beneficial to bypass these violations for now and then add them in a future PR so that they are more visible. Maybe we could even override the private field rule so that it's ignored for now and add that in its own PR since it affects so many files. I am happy to help with this, just let me know.

legobeat · 2023-07-13T23:26:45Z

New attempt: #1498

mcmire · 2023-07-31T16:52:16Z

Since #1498 has been merged, should we close this pull request?

Mrtenz commented Dec 9, 2022

View reviewed changes

Mrtenz force-pushed the mrtenz/bump-eslint branch from f1acb29 to 70c1a33 Compare December 9, 2022 15:32

Mrtenz marked this pull request as ready for review December 9, 2022 15:36

Mrtenz requested a review from a team as a code owner December 9, 2022 15:36

mcmire reviewed Dec 13, 2022

View reviewed changes

Mrtenz added 20 commits December 21, 2022 12:06

Bump ESLint and related dependencies

c20a19e

Update configs and run yarn lint:fix

9be42bc

Update naming conventions rule

f442515

Fix address-book-controller

a164a67

Disable @typescript-eslint/require-await

ef18423

Fix assets-controller

0f99129

Fix base-controller

5da25dd

Fix composable-controller

5442b90

Fix controller-utils

e3ce446

Fix gas-fee-controller

91ef34b

Fix keyring-controller

5ea4b01

Fix message-manager

0772b6f

Fix network-controller

57bbc64

Fix notification-controller

2dc724e

Fix permission-controller

5f6a434

Fix phishing-controller

f8a61a3

Fix preferences-controller

5161229

Fix rate-limit-controller

dd7e6b5

Fix subject-metadata-controller

545fec7

Fix transaction-controller

39af681

Mrtenz added 12 commits December 21, 2022 12:12

Fix remaining errors

7d54a30

Revert TypeScript version bump

aed7873

Fix requested changes in assets-controllers

159edfa

Disable no-shadow for assets-controllers

77a4ad4

Fix requested changes in controller-utils

ac20f81

Remove unnecessary parseInt

301d7c9

Fix requested changes in gas-fee-controller

84c7a1f

Remove unused import

c4a46db

Use if statement

4a1cf68

Revert change to _executeRestrictedMethod

706445e

Fix requested changes in subject-metadata-controller

5c5336e

Fix issues after rebase

9c62a28

Mrtenz force-pushed the mrtenz/bump-eslint branch from 94a0e7e to 9c62a28 Compare December 21, 2022 11:14

Gudahtt force-pushed the main branch from 296e084 to dec9125 Compare February 8, 2023 19:49

mcmire mentioned this pull request Apr 27, 2023

devDeps: upgrade eslint & friends #1277

Closed

3 tasks

legobeat mentioned this pull request Jul 13, 2023

devDeps: update eslint packages #1498

Merged

3 tasks

Mrtenz closed this Jan 15, 2025

Mrtenz deleted the mrtenz/bump-eslint branch January 15, 2025 12:04

	await this.fullUpdate();
	this.fullUpdate().catch(console.error);

	await this.#showApprovalRequest();
	this.#showApprovalRequest().catch(console.error);

		chainId: parseInt(chainId, NaN),
		networkId: parseInt(networkId, NaN),

Uh oh!

Conversation

Mrtenz commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

legobeat commented Dec 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcmire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcmire commented Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

legobeat commented Jul 13, 2023

Uh oh!

mcmire commented Jul 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Mrtenz commented Dec 9, 2022 •

edited

Loading

legobeat commented Dec 10, 2022 •

edited

Loading

mcmire commented Dec 20, 2022 •

edited

Loading