Skip to content

Commit

Permalink
style: Enforce using type instead of interface with `@typescript-…
Browse files Browse the repository at this point in the history
…eslint/consistent-type-definitions` rule (#23439)

## **Description**

### Motivation

`interface` usage is banned in MetaMask codebases. With TypeScript
conversion efforts in progress, it's time to enforce `type` usage over
`interface` in the extension codebase as well, especially for new
contributions in TypeScript.

There are a few reasons for banning `interface`:
- Interfaces do not extend `Record` or have a `string` index signature
by default, making them incompatible with the data/state objects we use.
- The feature set of type aliases is a strict superset of that of
interfaces (including `extends`, `implements`).
- Declaration merging is an exception, but this is not a practice we
want to encourage.

For more context, see these links for previous discussions on this
topic:
- MetaMask/eslint-config#216
- MetaMask/core#1933

### Explanation

The `@typescript-eslint/consistent-type-definitions` has been configured
with the `["error", "type"]` options, which will prevent the `interface`
keyword from being introduced into the codebase in the future.

Existing instances of `interface` usage in the codebase were converted
to type alias definitions.

For the following exceptions, the `interface` keyword was preserved with
a `eslint-disable` directive, and a TODO comment was added for future
conversion.

1. If the `interface` is declared with an `extends` keyword.
- This is because of possible type accuracy issues with the eslint
autofix appending the extending interface as an intersection (`&`).
2. If the `interface` is used for declaration merging. 
- This is the only use case for interfaces that is not supported by type
aliases, and is therefore a valid exception to the ESLint rule.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23439?quickstart=1)

## **Related issues**

- Closes #23442

## **Manual testing steps**

## **Screenshots/Recordings**

## **Pre-merge author checklist**

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [ ] I've included manual testing steps
- [ ] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [x] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [x] In case it's not yet "ready for review", I've set it to "draft".
- [x] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.
  • Loading branch information
MajorLift authored Mar 14, 2024
1 parent c0dfd05 commit 9b62590
Show file tree
Hide file tree
Showing 103 changed files with 282 additions and 173 deletions.
4 changes: 2 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,9 @@ module.exports = {
'import/namespace': 'off',
'import/default': 'off',
'import/no-named-as-default-member': 'off',
// Disabled due to incompatibility with Record<string, unknown>.
// Set to ban interfaces due to their incompatibility with Record<string, unknown>.
// See: <https://github.com/Microsoft/TypeScript/issues/15300#issuecomment-702872440>
'@typescript-eslint/consistent-type-definitions': 'off',
'@typescript-eslint/consistent-type-definitions': ['error', 'type'],
// Modified to include the 'ignoreRestSiblings' option.
// TODO: Migrate this rule change back into `@metamask/eslint-config`
'@typescript-eslint/no-unused-vars': [
Expand Down
5 changes: 2 additions & 3 deletions app/scripts/controllers/mmi-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import MetaMetricsController from './metametrics';
import { getPermissionBackgroundApiMethods } from './permissions';
import { PreferencesController } from './preferences';

interface UpdateCustodianTransactionsParameters {
type UpdateCustodianTransactionsParameters = {
keyring: CustodyKeyring;
type: string;
txList: string[];
Expand All @@ -51,7 +51,7 @@ interface UpdateCustodianTransactionsParameters {
txStateManager: any;
getPendingNonce: (address: string) => Promise<number>;
setTxHash: (txId: string, txHash: string) => void;
}
};

export default class MMIController extends EventEmitter {
public opts: MMIControllerOptions;
Expand Down Expand Up @@ -806,7 +806,6 @@ export default class MMIController extends EventEmitter {
});
}

// @ts-expect-error not relevant
this.signatureController.setMessageMetadata(messageId, signature);

return this.getState();
Expand Down
8 changes: 4 additions & 4 deletions app/scripts/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,11 @@ export const isValidDate = (d: Date | number) => {
* @property {() => void} reject - A function that rejects the Promise.
*/

interface DeferredPromise {
type DeferredPromise = {
promise: Promise<any>;
resolve?: () => void;
reject?: () => void;
}
};

/**
* Create a defered Promise.
Expand Down Expand Up @@ -274,7 +274,7 @@ export function isWebUrl(urlString: string): boolean {
);
}

interface FormattedTransactionMeta {
type FormattedTransactionMeta = {
blockHash: string | null;
blockNumber: string | null;
from: string;
Expand All @@ -293,7 +293,7 @@ interface FormattedTransactionMeta {
type: TransactionEnvelopeType;
accessList: AccessList | null;
transactionIndex: string | null;
}
};

export function formatTxMetaForRpcResult(
txMeta: TransactionMeta,
Expand Down
4 changes: 2 additions & 2 deletions app/scripts/migrations/096.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ export async function migrate(
return versionedData;
}

interface NetworkConfiguration {
type NetworkConfiguration = {
chainId: Record<string, any>;
}
};

function transformState(state: Record<string, unknown>) {
if (
Expand Down
8 changes: 4 additions & 4 deletions app/scripts/migrations/105.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,15 @@ function addressToUUID(address: string): string {
});
}

interface Identity {
type Identity = {
name: string;
address: string;
lastSelected?: number;
}
};

interface Identities {
type Identities = {
[key: string]: Identity;
}
};

function createMockPreferenceControllerState(
identities: Identity[] = [{ name: 'Account 1', address: MOCK_ADDRESS }],
Expand Down
4 changes: 2 additions & 2 deletions app/scripts/migrations/105.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ type VersionedData = {
data: Record<string, unknown>;
};

interface Identity {
type Identity = {
name: string;
address: string;
lastSelected?: number;
}
};

export const version = 105;

Expand Down
8 changes: 4 additions & 4 deletions app/scripts/migrations/107.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,17 @@ import { hasProperty, isObject } from '@metamask/utils';

export const version = 107;

interface AccountBalance {
type AccountBalance = {
address: string;
balance: string;
}
};

interface AccountTrackerControllerState {
type AccountTrackerControllerState = {
accountsByChainId: Record<string, Record<string, AccountBalance>>;
accounts: Record<string, AccountBalance>;
currentBlockGasLimit: string;
currentBlockGasLimitByChainId: Record<string, string>;
}
};

/**
* Migrates state from the now removed CachedBalancesController to the AccountTrackerController and formats it accordingly.
Expand Down
4 changes: 2 additions & 2 deletions development/fitness-functions/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ const RULES: IRule[] = [
},
];

interface IRule {
type IRule = {
name: string;
fn: (diff: string) => boolean;
docURL?: string;
}
};

function runFitnessFunctionRule(rule: IRule, diff: string): void {
const { name, fn, docURL } = rule;
Expand Down
4 changes: 2 additions & 2 deletions shared/constants/gas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export enum NetworkCongestionThresholds {
busy = 0.66,
}

export interface TxGasFees {
export type TxGasFees = {
/** Maxmimum number of units of gas to use for this transaction. */
gasLimit: string;
/** Price per gas for legacy txs */
Expand All @@ -111,4 +111,4 @@ export interface TxGasFees {
userEditedGasLimit: string;
/** Estimate level user selected. */
userFeeLevel: string;
}
};
28 changes: 14 additions & 14 deletions shared/constants/mmi-controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import AppStateController from '../../app/scripts/controllers/app-state';
import AccountTracker from '../../app/scripts/lib/account-tracker';
import MetaMetricsController from '../../app/scripts/controllers/metametrics';

export interface MMIControllerOptions {
export type MMIControllerOptions = {
mmiConfigurationController: MmiConfigurationController;
keyringController: any;
preferencesController: PreferencesController;
Expand All @@ -36,36 +36,36 @@ export interface MMIControllerOptions {
setTxStatusSubmitted: (txId: string) => void;
setTxStatusFailed: (txId: string) => void;
updateTransaction: (txMeta: any) => void;
}
};

export interface ISignedEvent {
export type ISignedEvent = {
signature: Signature;
messageId: string;
}
};

export interface IInteractiveRefreshTokenChangeEvent {
export type IInteractiveRefreshTokenChangeEvent = {
url: string;
oldRefreshToken: string;
}
};

export interface IConnectCustodyAddresses {
export type IConnectCustodyAddresses = {
custodianType: string;
custodianName: string;
accounts: string[];
}
};

export interface Label {
export type Label = {
key: string;
value: string;
}
};

export interface Signature {
export type Signature = {
custodian_transactionId?: string;
from: string;
}
};

export interface NetworkConfiguration {
export type NetworkConfiguration = {
id: string;
chainId: string;
setActiveNetwork: (chainId: string) => void;
}
};
4 changes: 2 additions & 2 deletions shared/constants/swaps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export const MAX_ALLOWED_SLIPPAGE = 15;
// in place of the token address that ERC-20 tokens have
const DEFAULT_TOKEN_ADDRESS = '0x0000000000000000000000000000000000000000';

export interface SwapsTokenObject {
export type SwapsTokenObject = {
/**
* The symbol of token object
*/
Expand All @@ -47,7 +47,7 @@ export interface SwapsTokenObject {
* URL for token icon
*/
iconUrl: string;
}
};

export const ETH_SWAPS_TOKEN_OBJECT: SwapsTokenObject = {
symbol: CURRENCY_SYMBOLS.ETH,
Expand Down
12 changes: 6 additions & 6 deletions shared/modules/i18n.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,23 @@ const fetchWithTimeout = getFetchWithTimeout();
// and in i18n lib, the translated message is an object (I18NMessage) with message & description -
// message is the string that will replace the translationKey, and that message may contain replacement variables such as $1, $2, etc.
// Description is key describing the usage of the message.
export interface I18NMessage {
export type I18NMessage = {
message: string;
description?: string;
}
};

// The overall translation file is made of same entries
// translationKey (string) and the I18NMessage as the value.
export interface I18NMessageDict {
export type I18NMessageDict = {
[translationKey: string]: I18NMessage;
}
};

export type I18NSubstitution = string | (() => any) | object;

// A parameterized type (or generic type) of maps that use the same structure (translationKey) key
interface I18NMessageDictMap<R> {
type I18NMessageDictMap<R> = {
[translationKey: string]: R;
}
};

export const FALLBACK_LOCALE = 'en';

Expand Down
10 changes: 6 additions & 4 deletions types/global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@ declare class MessageSender {
url?: string;
}

export interface LedgerIframeMissingResponse {
export type LedgerIframeMissingResponse = {
success: false;
payload: {
error: Error;
};
}
};

type ResponseType =
| Unsuccessful
Expand All @@ -60,7 +60,7 @@ type ResponseType =
* input values so that the correct type can be inferred in the callback
* method
*/
interface sendMessage {
type sendMessage = {
(
extensionId: string,
message: Record<string, unknown>,
Expand Down Expand Up @@ -196,7 +196,7 @@ interface sendMessage {
message: Record<string, unknown>,
callback?: (response: ResponseType) => void,
): void;
}
};

declare class Runtime {
onMessage: {
Expand Down Expand Up @@ -237,6 +237,8 @@ export declare global {
var chrome: Chrome;

namespace jest {
// The interface is being used for declaration merging, which is an acceptable exception to this rule.
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
interface Matchers<R> {
toBeFulfilled(): Promise<R>;
toNeverResolve(): Promise<R>;
Expand Down
4 changes: 2 additions & 2 deletions types/react-tippy.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ declare module 'react-tippy' {
export type Size = 'small' | 'regular' | 'big';
export type Theme = 'dark' | 'light' | 'transparent';

export interface TooltipProps {
export type TooltipProps = {
title?: string;
disabled?: boolean;
open?: boolean;
Expand Down Expand Up @@ -60,7 +60,7 @@ declare module 'react-tippy' {
theme?: Theme;
className?: string;
style?: React.CSSProperties;
}
};

export class Tooltip extends React.Component<TooltipProps> {}

Expand Down
3 changes: 3 additions & 0 deletions types/react.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ declare namespace React {
import * as CSS from 'csstype';

// Add custom CSS properties so that they can be used in `style` props
// This interface was created before this ESLint rule was added.
// Convert to a `type` in a future major version.
// eslint-disable-next-line @typescript-eslint/consistent-type-definitions
export interface CSSProperties extends CSS.Properties<string | number> {
'--progress'?: string;
}
Expand Down
4 changes: 2 additions & 2 deletions ui/components/app/confirm/info/info.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,9 @@ export type ConfirmInfoRowConfig = {
variant?: ConfirmInfoRowVariant;
};

interface ConfirmInfoProps {
type ConfirmInfoProps = {
rowConfigs: ConfirmInfoRowConfig[];
}
};

/**
* ConfirmInfo receives a custom config object and displays a list of ConfirmInfoRow components
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ import { PolymorphicRef } from '../../component-library/box';
import { TEST_CHAINS } from '../../../../shared/constants/network';
import NetworkToggle from './network-toggle';

interface IncomingTransactionToggleProps {
type IncomingTransactionToggleProps = {
wrapperRef?: PolymorphicRef<any>;
incomingTransactionsPreferences: Record<string, boolean>;
allNetworks: Record<string, any>[];
setIncomingTransactionsPreferences: (
chainId: string,
isAllEnabledValue: boolean,
) => void;
}
};

const IncomingTransactionToggle = ({
wrapperRef,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@ import Tooltip from '../../ui/tooltip';

const MAXIMUM_CHARACTERS_WITHOUT_TOOLTIP = 20;

interface NetworkPreferences {
type NetworkPreferences = {
isShowIncomingTransactions: boolean;
label: string;
imageUrl: string;
}
};

interface NetworkToggleProps {
type NetworkToggleProps = {
networkPreferences: NetworkPreferences;
toggleSingleNetwork: (chainId: string, value: boolean) => void;
chainId: string;
}
};

const NetworkToggle = ({
networkPreferences,
Expand Down
Loading

0 comments on commit 9b62590

Please sign in to comment.