Skip to content

Commit

Permalink
Only enable Node lint rules for Node files (#3672)
Browse files Browse the repository at this point in the history
In a previous commit, we mistakenly introduced a Node-specific function
for testing deep equality, and this ended up crashing the extension.
This would usually have been caught by our ESLint rules, as they
prohibit use of Node libraries by default. However, the ESLint
configuration for this repo imports rules from our
`@metamask/eslint-config-nodejs` package and applies them to all files,
marking everything as Node-compatible, thereby allowing such usage. This
is incorrect: these rules should only apply to test files and scripts.

This commit fix the ESLint configuration to match. Doing so revealed a
couple of categories of violations, which this commit also fixes:

- Some packages import and make use of EventEmitter from Node's `events`
module. We add a notice to the README for these packages which advises
consumers that these packages are designed to be used in a
Node-compatible environment.
- Some packages import and make use of Node's `assert` module to check
values at runtime. It isn't strictly necessary to use this module, and
so we replace its usage with simpler code.
- Some packages import and make use of Node's `inspect` utility
function. We don't strictly need this either, and we replace its usage
with simpler code as well.
  • Loading branch information
mcmire authored Nov 27, 2024
1 parent a3e603c commit a1100c2
Show file tree
Hide file tree
Showing 17 changed files with 100 additions and 59 deletions.
17 changes: 15 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
module.exports = {
root: true,
extends: ['@metamask/eslint-config', '@metamask/eslint-config-nodejs'],
extends: ['@metamask/eslint-config'],
ignorePatterns: [
'!.eslintrc.js',
'!jest.config.js',
'!.prettierrc.js',
'node_modules',
'**/dist',
'**/docs',
Expand All @@ -12,6 +12,19 @@ module.exports = {
'scripts/create-package/package-template',
],
overrides: [
{
files: [
'**/jest.config.js',
'**/jest.environment.js',
'**/tests/**/*.{ts,js}',
'*.js',
'*.test.{ts,js}',
'scripts/*.ts',
'scripts/create-package/*.ts',
'yarn.config.cjs',
],
extends: ['@metamask/eslint-config-nodejs'],
},
{
files: ['*.test.{ts,js}', '**/tests/**/*.{ts,js}'],
extends: ['@metamask/eslint-config-jest'],
Expand Down
4 changes: 4 additions & 0 deletions packages/message-manager/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ or

`npm install @metamask/message-manager`

## Compatibility

This package relies implicitly upon the `EventEmitter` module. This module is available natively in Node.js, but when using this package for the browser, make sure to use a polyfill such as `events`.

## Contributing

This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme).
2 changes: 2 additions & 0 deletions packages/message-manager/src/AbstractMessageManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type { BaseConfig, BaseState } from '@metamask/base-controller';
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
import { EventEmitter } from 'events';
import { v1 as random } from 'uuid';

Expand Down
95 changes: 42 additions & 53 deletions packages/network-controller/src/NetworkController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ import { errorCodes } from '@metamask/rpc-errors';
import { createEventEmitterProxy } from '@metamask/swappable-obj-proxy';
import type { SwappableProxy } from '@metamask/swappable-obj-proxy';
import type { Hex } from '@metamask/utils';
import { isStrictHexString, hasProperty, isPlainObject } from '@metamask/utils';
import { strict as assert } from 'assert';
import { hasProperty, isPlainObject, isStrictHexString } from '@metamask/utils';
import type { Draft } from 'immer';
import type { Logger } from 'loglevel';
import { createSelector } from 'reselect';
import * as URI from 'uri-js';
import { inspect } from 'util';
import { v4 as uuidV4 } from 'uuid';

import { INFURA_BLOCKED_KEY, NetworkStatus } from './constants';
Expand Down Expand Up @@ -515,7 +513,7 @@ function getDefaultNetworkConfigurationsByChainId(): Record<
>((obj, infuraNetworkType) => {
const chainId = ChainId[infuraNetworkType];
const rpcEndpointUrl =
// False negative - this is a string.
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`https://${infuraNetworkType}.infura.io/v3/{infuraProjectId}` as const;

Expand Down Expand Up @@ -788,9 +786,9 @@ function validateNetworkControllerState(state: NetworkState) {

if (!networkClientIds.includes(state.selectedNetworkClientId)) {
throw new Error(
`NetworkController state is invalid: \`selectedNetworkClientId\` ${inspect(
state.selectedNetworkClientId,
)} does not refer to an RPC endpoint within a network configuration`,
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`NetworkController state is invalid: \`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration`,
);
}
}
Expand Down Expand Up @@ -1354,19 +1352,16 @@ export class NetworkController extends BaseController<
* removed in a future release
*/
async setProviderType(type: InfuraNetworkType) {
assert.notStrictEqual(
type,
NetworkType.rpc,
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`NetworkController - cannot call "setProviderType" with type "${NetworkType.rpc}". Use "setActiveNetwork"`,
);
assert.ok(
isInfuraNetworkType(type),
// TODO: Either fix this lint violation or explain why it's necessary to ignore.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Unknown Infura provider type "${type}".`,
);
if ((type as unknown) === NetworkType.rpc) {
throw new Error(
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`NetworkController - cannot call "setProviderType" with type "${NetworkType.rpc}". Use "setActiveNetwork"`,
);
}
if (!isInfuraNetworkType(type)) {
throw new Error(`Unknown Infura provider type "${String(type)}".`);
}

await this.setActiveNetwork(type);
}
Expand Down Expand Up @@ -1635,9 +1630,7 @@ export class NetworkController extends BaseController<

if (existingNetworkConfiguration === undefined) {
throw new Error(
`Could not update network: Cannot find network configuration for chain ${inspect(
chainId,
)}`,
`Could not update network: Cannot find network configuration for chain '${chainId}'`,
);
}

Expand Down Expand Up @@ -1802,7 +1795,7 @@ export class NetworkController extends BaseController<
})
) {
throw new Error(
// False negative - this is a string.
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not update network: Cannot update RPC endpoints in such a way that the selected network '${this.state.selectedNetworkClientId}' would be removed without a replacement. Choose a different RPC endpoint as the selected network via the \`replacementSelectedRpcEndpointIndex\` option.`,
);
Expand Down Expand Up @@ -1899,7 +1892,7 @@ export class NetworkController extends BaseController<

if (existingNetworkConfiguration === undefined) {
throw new Error(
`Cannot find network configuration for chain ${inspect(chainId)}`,
`Cannot find network configuration for chain '${chainId}'`,
);
}

Expand Down Expand Up @@ -2031,9 +2024,7 @@ export class NetworkController extends BaseController<
!isSafeChainId(networkFields.chainId)
) {
throw new Error(
`${errorMessagePrefix}: Invalid \`chainId\` ${inspect(
networkFields.chainId,
)} (must start with "0x" and not exceed the maximum)`,
`${errorMessagePrefix}: Invalid \`chainId\` '${networkFields.chainId}' (must start with "0x" and not exceed the maximum)`,
);
}

Expand All @@ -2046,13 +2037,13 @@ export class NetworkController extends BaseController<
if (existingNetworkConfigurationViaChainId !== undefined) {
if (existingNetworkConfiguration === null) {
throw new Error(
// False negative - these are strings.
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not add network for chain ${args.networkFields.chainId} as another network for that chain already exists ('${existingNetworkConfigurationViaChainId.name}')`,
);
} else {
throw new Error(
// False negative - these are strings.
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Cannot move network from chain ${existingNetworkConfiguration.chainId} to ${networkFields.chainId} as another network for that chain already exists ('${existingNetworkConfigurationViaChainId.name}')`,
);
Expand Down Expand Up @@ -2082,9 +2073,9 @@ export class NetworkController extends BaseController<
for (const rpcEndpointFields of networkFields.rpcEndpoints) {
if (!isValidUrl(rpcEndpointFields.url)) {
throw new Error(
`${errorMessagePrefix}: An entry in \`rpcEndpoints\` has invalid URL ${inspect(
rpcEndpointFields.url,
)}`,
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`${errorMessagePrefix}: An entry in \`rpcEndpoints\` has invalid URL '${rpcEndpointFields.url}'`,
);
}
const networkClientId =
Expand Down Expand Up @@ -2113,13 +2104,9 @@ export class NetworkController extends BaseController<
)
) {
throw new Error(
`${errorMessagePrefix}: RPC endpoint '${
// This is a string.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
rpcEndpointFields.url
}' refers to network client ${inspect(
networkClientId,
)} that does not exist`,
// This is a string.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`${errorMessagePrefix}: RPC endpoint '${rpcEndpointFields.url}' refers to network client '${networkClientId}' that does not exist`,
);
}

Expand Down Expand Up @@ -2149,15 +2136,19 @@ export class NetworkController extends BaseController<
URI.equal(rpcEndpointFields.url, existingRpcEndpoint.url),
);
if (rpcEndpoint) {
throw new Error(
mode === 'update'
? // False negative - these are strings.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not update network to point to same RPC endpoint as existing network for chain ${networkConfiguration.chainId} ('${networkConfiguration.name}')`
: // False negative - these are strings.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not add network that points to same RPC endpoint as existing network for chain ${networkConfiguration.chainId} ('${networkConfiguration.name}')`,
);
if (mode === 'update') {
throw new Error(
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not update network to point to same RPC endpoint as existing network for chain ${networkConfiguration.chainId} ('${networkConfiguration.name}')`,
);
} else {
throw new Error(
// This ESLint rule mistakenly produces an error.
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions
`Could not add network that points to same RPC endpoint as existing network for chain ${networkConfiguration.chainId} ('${networkConfiguration.name}')`,
);
}
}
}
}
Expand Down Expand Up @@ -2561,7 +2552,7 @@ export class NetworkController extends BaseController<
/* istanbul ignore if */
if (!possibleAutoManagedNetworkClient) {
throw new Error(
`No Infura network client found with ID ${inspect(networkClientId)}`,
`No Infura network client found with ID '${networkClientId}'`,
);
}

Expand All @@ -2573,9 +2564,7 @@ export class NetworkController extends BaseController<
];

if (!possibleAutoManagedNetworkClient) {
throw new Error(
`No network client found with ID ${inspect(networkClientId)}`,
);
throw new Error(`No network client found with ID '${networkClientId}'`);
}

autoManagedNetworkClient = possibleAutoManagedNetworkClient;
Expand Down
4 changes: 4 additions & 0 deletions packages/signature-controller/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ or

`npm install @metamask/signature-controller`

## Compatibility

This package relies implicitly upon the `EventEmitter` module. This module is available natively in Node.js, but when using this package for the browser, make sure to use a polyfill such as `events`.

## Contributing

This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme).
2 changes: 2 additions & 0 deletions packages/signature-controller/src/SignatureController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ import {
} from '@metamask/logging-controller';
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
import EventEmitter from 'events';
import { v1 as random } from 'uuid';

Expand Down
4 changes: 4 additions & 0 deletions packages/transaction-controller/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ or

`npm install @metamask/transaction-controller`

## Compatibility

This package relies implicitly upon the `EventEmitter` module. This module is available natively in Node.js, but when using this package for the browser, make sure to use a polyfill such as `events`.

## Contributing

This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme).
2 changes: 2 additions & 0 deletions packages/transaction-controller/src/TransactionController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ import { errorCodes, rpcErrors, providerErrors } from '@metamask/rpc-errors';
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
import { EventEmitter } from 'events';
import { cloneDeep, mapValues, merge, pickBy, sortBy } from 'lodash';
import { v1 as random } from 'uuid';
Expand Down
2 changes: 2 additions & 0 deletions packages/transaction-controller/src/helpers/GasFeePoller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import type {
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
import EventEmitter from 'events';

import { projectLogger } from '../logger';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type { AccountsController } from '@metamask/accounts-controller';
import type { BlockTracker } from '@metamask/network-controller';
import type { Hex } 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
import EventEmitter from 'events';

import {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import type { NetworkClientId, Provider } from '@metamask/network-controller';
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
import EventEmitter from 'events';

import { projectLogger } from '../logger';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import type {
BlockTracker,
NetworkClientId,
} from '@metamask/network-controller';
// This package purposefully relies on Node's EventEmitter module.
// eslint-disable-next-line import/no-nodejs-modules
import EventEmitter from 'events';
import { cloneDeep, merge } from 'lodash';

Expand Down
4 changes: 4 additions & 0 deletions packages/user-operation-controller/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ or

`npm install @metamask/user-operation-controller`

## Compatibility

This package relies implicitly upon the `EventEmitter` module. This module is available natively in Node.js, but when using this package for the browser, make sure to use a polyfill such as `events`.

## Contributing

This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme).
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import {
type TransactionType,
} 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
import EventEmitter from 'events';
import type { Patch } from 'immer';
import { cloneDeep } from 'lodash';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type {
} from '@metamask/network-controller';
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
import EventEmitter from 'events';

import { projectLogger } from '../logger';
Expand Down
9 changes: 7 additions & 2 deletions scripts/create-package/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ jest.mock('fs', () => ({
mkdir: jest.fn(),
readFile: jest.fn(),
writeFile: jest.fn(),
stat: jest.fn(),
},
}));

Expand Down Expand Up @@ -86,7 +87,9 @@ describe('create-package/utils', () => {
nodeVersions: '>=18.0.0',
};

(fs.existsSync as jest.Mock).mockReturnValueOnce(false);
(fs.promises.stat as jest.Mock).mockResolvedValueOnce({
isDirectory: () => false,
});

(fsUtils.readAllFiles as jest.Mock).mockResolvedValueOnce({
'src/index.ts': 'export default 42;',
Expand Down Expand Up @@ -167,7 +170,9 @@ describe('create-package/utils', () => {
nodeVersions: '20.0.0',
};

(fs.existsSync as jest.Mock).mockReturnValueOnce(true);
(fs.promises.stat as jest.Mock).mockResolvedValueOnce({
isDirectory: () => true,
});

await expect(
finalizeAndWriteData(packageData, monorepoFileData),
Expand Down
Loading

0 comments on commit a1100c2

Please sign in to comment.