Skip to content

Commit

Permalink
Only enable Node lint rules for Node files
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 can't assume this exists in a browser context, so we fix
  this by importing the `events` NPM package.
- 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.
  • Loading branch information
mcmire committed Jun 27, 2024
1 parent 676d28b commit 5484629
Show file tree
Hide file tree
Showing 16 changed files with 76 additions and 21 deletions.
20 changes: 18 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',
'tsup.config.ts',
],
extends: ['@metamask/eslint-config-nodejs'],
},
{
files: ['*.test.{ts,js}', '**/tests/**/*.{ts,js}'],
extends: ['@metamask/eslint-config-jest'],
Expand Down Expand Up @@ -121,5 +134,8 @@ module.exports = {
'import/resolver': {
typescript: {},
},
jsdoc: {
mode: 'typescript',
},
},
};
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
@@ -1,6 +1,8 @@
import type { BaseConfig, BaseState } from '@metamask/base-controller';
import { BaseControllerV1 } from '@metamask/base-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';

/**
Expand Down
32 changes: 17 additions & 15 deletions packages/network-controller/src/NetworkController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import {
hasProperty,
isPlainObject,
} from '@metamask/utils';
import { strict as assert } from 'assert';
import { v4 as random } from 'uuid';

import { INFURA_BLOCKED_KEY, NetworkStatus } from './constants';
Expand Down Expand Up @@ -131,7 +130,9 @@ function assertOfType<Type>(
validate: (value: unknown) => boolean,
message: string,
): asserts value is Type {
assert.ok(validate(value), message);
if (!validate(value)) {
throw new Error(message);
}
}

/**
Expand Down Expand Up @@ -878,19 +879,20 @@ 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(
// 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"`,
);
}
if (!isInfuraNetworkType(type)) {
throw new Error(
// 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}".`,
);
}

await this.setActiveNetwork(type);
}
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 @@ -42,6 +42,8 @@ import {
} from '@metamask/message-manager';
import { providerErrors } from '@metamask/rpc-errors';
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 { cloneDeep } from 'lodash';

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 @@ -47,6 +47,8 @@ import type { Hex } from '@metamask/utils';
import { add0x } 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 { cloneDeep, mapValues, merge, pickBy, sortBy, isEqual } 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 { incomingTransactionsLogger as log } 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 @@ -4,6 +4,8 @@ import type { NetworkClient, Provider } from '@metamask/network-controller';
import { BlockTrackerPollingControllerOnly } from '@metamask/polling-controller';
import type { Json } from '@metamask/utils';
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
4 changes: 2 additions & 2 deletions scripts/create-package/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import execa from 'execa';
import { existsSync, promises as fs } from 'fs';
import { promises as fs } from 'fs';
import path from 'path';
import { format as prettierFormat } from 'prettier';
import type { Options as PrettierOptions } from 'prettier';
Expand Down Expand Up @@ -94,7 +94,7 @@ export async function finalizeAndWriteData(
monorepoFileData: MonorepoFileData,
) {
const packagePath = path.join(PACKAGES_PATH, packageData.directoryName);
if (existsSync(packagePath)) {
if ((await fs.stat(packagePath)).isDirectory()) {
throw new Error(`The package directory already exists: ${packagePath}`);
}

Expand Down

0 comments on commit 5484629

Please sign in to comment.