From 1e494f30042dd480faa4b74304f0dd6b3d0aabaf Mon Sep 17 00:00:00 2001
From: Elliot Winkler <elliot.winkler@gmail.com>
Date: Mon, 28 Feb 2022 10:42:09 -0700
Subject: [PATCH] Refactor ESLint config (#13482)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We would like to insert TypeScript into the ESLint configuration, and
because of the way that the current config is organized, that is not
easy to do.

Most files are assumed to be files that are suited for running in a
browser context. This isn't correct, as we should expect most files to
work in a Node context instead. This is because all browser-based files
will be run through a transpiler that is able to make use of
Node-specific variables anyway.

There are a couple of important ways we can categories files which our
ESLint config should be capable of handling well:

* Is the file a script or a module? In other words, does the file run
  procedurally or is the file intended to be brought into an existing
  file?
* If the file is a module, does it use the CommonJS syntax (`require()`)
  or does it use the ES syntax (`import`/`export`)?

When we introduce TypeScript, this set of questions will become:

* Is the file a script or a module?
* If the file is a module, is it a JavaScript module or a TypeScript
  module?
* If the file is a JavaScript module, does it use the CommonJS syntax
  (`require()`) or does it use the ES syntax (`import`/`export`)?

To represent these divisions, this commit removes global rules — so now
all of the rules are kept in `overrides` for explicitness — and sets up
rules for CommonJS- and ES-module-compatible files that intentionally do
not overlap with each other. This way TypeScript (which has its own set
of rules independent from JavaScript and therefore shouldn't overlap
with the other rules either) can be easily added later.

Finally, this commit splits up the ESLint config into separate files and
adds documentation to each section. This way sets of rules which are
connected to a particular plugin (`jsdoc`, `@babel`, etc.) can be easily
understood instead of being obscured.
---
 .eslintrc.babel.js                         |   9 +
 .eslintrc.base.js                          |  67 ++++
 .eslintrc.js                               | 366 ++++++++++-----------
 .eslintrc.jsdoc.js                         |  23 ++
 .eslintrc.node.js                          |  10 +
 .prettierignore                            |   1 +
 development/build/transforms/utils.js      |  16 +-
 lavamoat/build-system/policy-override.json |   1 +
 test/e2e/tests/add-hide-token.spec.js      |   1 +
 test/e2e/tests/send-eth.spec.js            |   1 +
 test/e2e/tests/signature-request.spec.js   |   1 +
 11 files changed, 298 insertions(+), 198 deletions(-)
 create mode 100644 .eslintrc.babel.js
 create mode 100644 .eslintrc.base.js
 create mode 100644 .eslintrc.jsdoc.js
 create mode 100644 .eslintrc.node.js

diff --git a/.eslintrc.babel.js b/.eslintrc.babel.js
new file mode 100644
index 000000000..b067db84b
--- /dev/null
+++ b/.eslintrc.babel.js
@@ -0,0 +1,9 @@
+module.exports = {
+  parser: '@babel/eslint-parser',
+  plugins: ['@babel'],
+  rules: {
+    '@babel/no-invalid-this': 'error',
+    // Prettier handles this
+    '@babel/semi': 'off',
+  },
+};
diff --git a/.eslintrc.base.js b/.eslintrc.base.js
new file mode 100644
index 000000000..47c379969
--- /dev/null
+++ b/.eslintrc.base.js
@@ -0,0 +1,67 @@
+const path = require('path');
+
+module.exports = {
+  extends: [
+    '@metamask/eslint-config',
+    path.resolve(__dirname, '.eslintrc.jsdoc.js'),
+  ],
+
+  globals: {
+    document: 'readonly',
+    window: 'readonly',
+  },
+
+  rules: {
+    'default-param-last': 'off',
+    'prefer-object-spread': 'error',
+    'require-atomic-updates': 'off',
+
+    // This is the same as our default config, but for the noted exceptions
+    'spaced-comment': [
+      'error',
+      'always',
+      {
+        markers: [
+          'global',
+          'globals',
+          'eslint',
+          'eslint-disable',
+          '*package',
+          '!',
+          ',',
+          // Local additions
+          '/:', // This is for our code fences
+        ],
+        exceptions: ['=', '-'],
+      },
+    ],
+
+    'no-invalid-this': 'off',
+
+    // TODO: remove this override
+    'padding-line-between-statements': [
+      'error',
+      {
+        blankLine: 'always',
+        prev: 'directive',
+        next: '*',
+      },
+      {
+        blankLine: 'any',
+        prev: 'directive',
+        next: 'directive',
+      },
+      // Disabled temporarily to reduce conflicts while PR queue is large
+      // {
+      //   blankLine: 'always',
+      //   prev: ['multiline-block-like', 'multiline-expression'],
+      //   next: ['multiline-block-like', 'multiline-expression'],
+      // },
+    ],
+
+    // It is common to import modules without assigning them to variables in
+    // a browser context. For instance, we may import polyfills which change
+    // global variables, or we may import stylesheets.
+    'import/no-unassigned-import': 'off',
+  },
+};
diff --git a/.eslintrc.js b/.eslintrc.js
index 9c0bc07e9..84b68eada 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -1,132 +1,112 @@
+const path = require('path');
 const { version: reactVersion } = require('react/package.json');
 
 module.exports = {
   root: true,
-  parser: '@babel/eslint-parser',
-  parserOptions: {
-    sourceType: 'module',
-    ecmaVersion: 2017,
-    ecmaFeatures: {
-      experimentalObjectRestSpread: true,
-      impliedStrict: true,
-      modules: true,
-      blockBindings: true,
-      arrowFunctions: true,
-      objectLiteralShorthandMethods: true,
-      objectLiteralShorthandProperties: true,
-      templateStrings: true,
-      classes: true,
-      jsx: true,
-    },
-  },
-
+  // Ignore files which are also in .prettierignore
   ignorePatterns: [
-    '!.eslintrc.js',
-    '!.mocharc.js',
-    'node_modules/**',
-    'dist/**',
-    'builds/**',
-    'test-*/**',
-    'docs/**',
-    'coverage/',
-    'jest-coverage/',
-    'development/chromereload.js',
     'app/vendor/**',
-    'test/e2e/send-eth-with-private-key-test/**',
-    'nyc_output/**',
-    '.vscode/**',
-    'lavamoat/*/policy.json',
-    'storybook-build/**',
+    'builds/**/*',
+    'dist/**/*',
+    'development/chromereload.js',
   ],
+  overrides: [
+    /**
+     * == Modules ==
+     *
+     * The first two sections here, which cover module syntax, are mutually
+     * exclusive: the set of files covered between them may NOT overlap. This is
+     * because we do not allow a file to use two different styles for specifying
+     * imports and exports (however theoretically possible it may be).
+     */
 
-  extends: ['@metamask/eslint-config', '@metamask/eslint-config-nodejs'],
-
-  plugins: ['@babel', 'import', 'jsdoc'],
-
-  globals: {
-    document: 'readonly',
-    window: 'readonly',
-  },
-
-  rules: {
-    'default-param-last': 'off',
-    'prefer-object-spread': 'error',
-    'require-atomic-updates': 'off',
-
-    // This is the same as our default config, but for the noted exceptions
-    'spaced-comment': [
-      'error',
-      'always',
-      {
-        markers: [
-          'global',
-          'globals',
-          'eslint',
-          'eslint-disable',
-          '*package',
-          '!',
-          ',',
-          // Local additions
-          '/:', // This is for our code fences
-        ],
-        exceptions: ['=', '-'],
+    {
+      /**
+       * Modules (CommonJS module syntax)
+       *
+       * This is code that uses `require()` and `module.exports` to import and
+       * export other modules.
+       */
+      files: [
+        '.eslintrc.js',
+        '.eslintrc.*.js',
+        '.mocharc.js',
+        '*.config.js',
+        'development/**/*.js',
+        'test/e2e/**/*.js',
+        'test/helpers/*.js',
+        'test/lib/wait-until-called.js',
+      ],
+      extends: [
+        path.resolve(__dirname, '.eslintrc.base.js'),
+        path.resolve(__dirname, '.eslintrc.node.js'),
+        path.resolve(__dirname, '.eslintrc.babel.js'),
+      ],
+      parserOptions: {
+        sourceType: 'module',
       },
-    ],
-
-    'import/no-unassigned-import': 'off',
-
-    'no-invalid-this': 'off',
-    '@babel/no-invalid-this': 'error',
-
-    // Prettier handles this
-    '@babel/semi': 'off',
-
-    'node/no-process-env': 'off',
-
-    // Allow tag `jest-environment` to work around Jest bug
-    // See: https://github.com/facebook/jest/issues/7780
-    'jsdoc/check-tag-names': ['error', { definedTags: ['jest-environment'] }],
-
-    // TODO: remove this override
-    'padding-line-between-statements': [
-      'error',
-      {
-        blankLine: 'always',
-        prev: 'directive',
-        next: '*',
+      rules: {
+        // This rule does not work with CommonJS modules. We will just have to
+        // trust that all of the files specified above are indeed modules.
+        'import/unambiguous': 'off',
       },
-      {
-        blankLine: 'any',
-        prev: 'directive',
-        next: 'directive',
+    },
+    /**
+     * Modules (ES module syntax)
+     *
+     * This is code that explicitly uses `import`/`export` instead of
+     * `require`/`module.exports`.
+     */
+    {
+      files: [
+        'app/**/*.js',
+        'shared/**/*.js',
+        'ui/**/*.js',
+        '**/*.test.js',
+        'test/lib/**/*.js',
+        'test/mocks/**/*.js',
+        'test/jest/**/*.js',
+        'test/stub/**/*.js',
+        'test/unit-global/**/*.js',
+      ],
+      // TODO: Convert these files to modern JS
+      excludedFiles: ['test/lib/wait-until-called.js'],
+      extends: [
+        path.resolve(__dirname, '.eslintrc.base.js'),
+        path.resolve(__dirname, '.eslintrc.node.js'),
+        path.resolve(__dirname, '.eslintrc.babel.js'),
+      ],
+      parserOptions: {
+        sourceType: 'module',
       },
-      // Disabled temporarily to reduce conflicts while PR queue is large
-      // {
-      //   blankLine: 'always',
-      //   prev: ['multiline-block-like', 'multiline-expression'],
-      //   next: ['multiline-block-like', 'multiline-expression'],
-      // },
-    ],
+    },
 
-    // TODO: re-enable these rules
-    'node/no-sync': 'off',
-    'node/no-unpublished-import': 'off',
-    'node/no-unpublished-require': 'off',
-    'jsdoc/match-description': 'off',
-    'jsdoc/require-description': 'off',
-    'jsdoc/require-jsdoc': 'off',
-    'jsdoc/require-param-description': 'off',
-    'jsdoc/require-param-type': 'off',
-    'jsdoc/require-returns-description': 'off',
-    'jsdoc/require-returns-type': 'off',
-    'jsdoc/require-returns': 'off',
-    'jsdoc/valid-types': 'off',
-  },
-  overrides: [
+    /**
+     * == Everything else ==
+     *
+     * The sections from here on out may overlap with each other in various
+     * ways depending on their function.
+     */
+
+    /**
+     * React-specific code
+     *
+     * Code in this category contains JSX and hence needs to be run through the
+     * React plugin.
+     */
     {
-      files: ['ui/**/*.js', 'test/lib/render-helpers.js', 'test/jest/*.js'],
-      plugins: ['react'],
+      files: [
+        'test/lib/render-helpers.js',
+        'test/jest/rendering.js',
+        'ui/**/*.js',
+      ],
       extends: ['plugin:react/recommended', 'plugin:react-hooks/recommended'],
+      parserOptions: {
+        ecmaFeatures: {
+          jsx: true,
+        },
+      },
+      plugins: ['react'],
       rules: {
         'react/no-unused-prop-types': 'error',
         'react/no-unused-state': 'error',
@@ -139,74 +119,100 @@ module.exports = {
         'react/default-props-match-prop-types': 'error',
         'react/jsx-no-duplicate-props': 'error',
       },
-    },
-    {
-      files: ['test/e2e/**/*.spec.js'],
-      extends: ['@metamask/eslint-config-mocha'],
-      rules: {
-        'mocha/no-hooks-for-single-case': 'off',
-        'mocha/no-setup-in-describe': 'off',
-      },
-    },
-    {
-      files: ['app/scripts/migrations/*.js', '*.stories.js'],
-      rules: {
-        'import/no-anonymous-default-export': ['error', { allowObject: true }],
-      },
-    },
-    {
-      files: ['app/scripts/migrations/*.js'],
-      rules: {
-        'node/global-require': 'off',
+      settings: {
+        react: {
+          // If this is set to 'detect', ESLint will import React in order to
+          // find its version. Because we run ESLint in the build system under
+          // LavaMoat, this means that detecting the React version requires a
+          // LavaMoat policy for all of React, in the build system. That's a
+          // no-go, so we grab it from React's package.json.
+          version: reactVersion,
+        },
       },
     },
+    /**
+     * Mocha tests
+     *
+     * These are files that make use of globals and syntax introduced by the
+     * Mocha library.
+     */
     {
-      files: ['**/*.test.js'],
+      files: [
+        '**/*.test.js',
+        'test/lib/wait-until-called.js',
+        'test/e2e/**/*.spec.js',
+      ],
       excludedFiles: [
-        'ui/**/*.test.js',
-        'ui/__mocks__/*.js',
-        'shared/**/*.test.js',
-        'development/**/*.test.js',
+        'app/scripts/controllers/network/**/*.test.js',
+        'app/scripts/controllers/permissions/**/*.test.js',
         'app/scripts/lib/**/*.test.js',
         'app/scripts/migrations/*.test.js',
         'app/scripts/platforms/*.test.js',
-        'app/scripts/controllers/network/**/*.test.js',
-        'app/scripts/controllers/permissions/**/*.test.js',
+        'development/**/*.test.js',
+        'shared/**/*.test.js',
+        'ui/**/*.test.js',
+        'ui/__mocks__/*.js',
       ],
       extends: ['@metamask/eslint-config-mocha'],
       rules: {
+        // In Mocha tests, it is common to use `this` to store values or do
+        // things like force the test to fail.
+        '@babel/no-invalid-this': 'off',
         'mocha/no-setup-in-describe': 'off',
       },
     },
-    {
-      files: ['**/__snapshots__/*.snap'],
-      plugins: ['jest'],
-      rules: {
-        'jest/no-large-snapshots': [
-          'error',
-          { maxSize: 50, inlineMaxSize: 50 },
-        ],
-      },
-    },
+    /**
+     * Jest tests
+     *
+     * These are files that make use of globals and syntax introduced by the
+     * Jest library.
+     */
     {
       files: [
-        'ui/**/*.test.js',
-        'ui/__mocks__/*.js',
-        'shared/**/*.test.js',
-        'development/**/*.test.js',
+        '**/__snapshots__/*.snap',
+        'app/scripts/controllers/network/**/*.test.js',
+        'app/scripts/controllers/permissions/**/*.test.js',
         'app/scripts/lib/**/*.test.js',
         'app/scripts/migrations/*.test.js',
         'app/scripts/platforms/*.test.js',
-        'app/scripts/controllers/network/**/*.test.js',
-        'app/scripts/controllers/permissions/**/*.test.js',
+        'development/**/*.test.js',
+        'shared/**/*.test.js',
+        'test/jest/*.js',
+        'test/helpers/*.js',
+        'ui/**/*.test.js',
+        'ui/__mocks__/*.js',
       ],
       extends: ['@metamask/eslint-config-jest'],
+      parserOptions: {
+        sourceType: 'module',
+      },
       rules: {
-        'jest/no-restricted-matchers': 'off',
         'import/unambiguous': 'off',
         'import/named': 'off',
+        'jest/no-large-snapshots': [
+          'error',
+          { maxSize: 50, inlineMaxSize: 50 },
+        ],
+        'jest/no-restricted-matchers': 'off',
+      },
+    },
+    /**
+     * Migrations
+     */
+    {
+      files: ['app/scripts/migrations/*.js', '**/*.stories.js'],
+      rules: {
+        'import/no-anonymous-default-export': ['error', { allowObject: true }],
       },
     },
+    /**
+     * Executables and related files
+     *
+     * These are files that run in a Node context. They are either designed to
+     * run as executables (in which case they will have a shebang at the top) or
+     * are dependencies of executables (in which case they may use
+     * `process.exit` to exit).
+     */
     {
       files: [
         'development/**/*.js',
@@ -218,27 +224,9 @@ module.exports = {
         'node/shebang': 'off',
       },
     },
-    {
-      files: [
-        '.eslintrc.js',
-        '.mocharc.js',
-        'babel.config.js',
-        'jest.config.js',
-        'nyc.config.js',
-        'stylelint.config.js',
-        'app/scripts/lockdown-run.js',
-        'app/scripts/lockdown-more.js',
-        'development/**/*.js',
-        'test/e2e/**/*.js',
-        'test/env.js',
-        'test/setup.js',
-        'test/helpers/protect-intrinsics-helpers.js',
-        'test/lib/wait-until-called.js',
-      ],
-      parserOptions: {
-        sourceType: 'script',
-      },
-    },
+    /**
+     * Lockdown files
+     */
     {
       files: [
         'app/scripts/lockdown-run.js',
@@ -251,19 +239,11 @@ module.exports = {
         Compartment: 'readonly',
       },
     },
-  ],
-
-  settings: {
-    jsdoc: {
-      mode: 'typescript',
-    },
-    react: {
-      // If this is set to 'detect', ESLint will import React in order to find
-      // its version. Because we run ESLint in the build system under LavaMoat,
-      // this means that detecting the React version requires a LavaMoat policy
-      // for all of React, in the build system. That's a no-go, so we grab it
-      // from React's package.json.
-      version: reactVersion,
+    {
+      files: ['app/scripts/lockdown-run.js', 'app/scripts/lockdown-more.js'],
+      parserOptions: {
+        sourceType: 'script',
+      },
     },
-  },
+  ],
 };
diff --git a/.eslintrc.jsdoc.js b/.eslintrc.jsdoc.js
new file mode 100644
index 000000000..862145853
--- /dev/null
+++ b/.eslintrc.jsdoc.js
@@ -0,0 +1,23 @@
+module.exports = {
+  // Note that jsdoc is already in the `plugins` array thanks to
+  // @metamask/eslint-config — this just extends the config there
+  rules: {
+    // Allow tag `jest-environment` to work around Jest bug
+    // See: https://github.com/facebook/jest/issues/7780
+    'jsdoc/check-tag-names': ['error', { definedTags: ['jest-environment'] }],
+    'jsdoc/match-description': 'off',
+    'jsdoc/require-description': 'off',
+    'jsdoc/require-jsdoc': 'off',
+    'jsdoc/require-param-description': 'off',
+    'jsdoc/require-param-type': 'off',
+    'jsdoc/require-returns-description': 'off',
+    'jsdoc/require-returns-type': 'off',
+    'jsdoc/require-returns': 'off',
+    'jsdoc/valid-types': 'off',
+  },
+  settings: {
+    jsdoc: {
+      mode: 'typescript',
+    },
+  },
+};
diff --git a/.eslintrc.node.js b/.eslintrc.node.js
new file mode 100644
index 000000000..78a12a346
--- /dev/null
+++ b/.eslintrc.node.js
@@ -0,0 +1,10 @@
+module.exports = {
+  extends: ['@metamask/eslint-config-nodejs'],
+  rules: {
+    'node/no-process-env': 'off',
+    // TODO: re-enable these rules
+    'node/no-sync': 'off',
+    'node/no-unpublished-import': 'off',
+    'node/no-unpublished-require': 'off',
+  },
+};
diff --git a/.prettierignore b/.prettierignore
index 2e0417ca7..a98c312ad 100644
--- a/.prettierignore
+++ b/.prettierignore
@@ -10,3 +10,4 @@ app/vendor/**
 .vscode/**
 test/e2e/send-eth-with-private-key-test/**
 *.scss
+development/chromereload.js
diff --git a/development/build/transforms/utils.js b/development/build/transforms/utils.js
index d7ab9654f..fb7f49c32 100644
--- a/development/build/transforms/utils.js
+++ b/development/build/transforms/utils.js
@@ -1,11 +1,17 @@
 const { ESLint } = require('eslint');
 const eslintrc = require('../../../.eslintrc.js');
 
-// We don't want linting to fail for purely stylistic reasons.
-eslintrc.rules['prettier/prettier'] = 'off';
-// Sometimes we use `let` instead of `const` to assign variables depending on
-// the build type.
-eslintrc.rules['prefer-const'] = 'off';
+eslintrc.overrides.forEach((override) => {
+  const rules = override.rules ?? {};
+
+  // We don't want linting to fail for purely stylistic reasons.
+  rules['prettier/prettier'] = 'off';
+  // Sometimes we use `let` instead of `const` to assign variables depending on
+  // the build type.
+  rules['prefer-const'] = 'off';
+
+  override.rules = rules;
+});
 
 // Remove all test-related overrides. We will never lint test files here.
 eslintrc.overrides = eslintrc.overrides.filter((override) => {
diff --git a/lavamoat/build-system/policy-override.json b/lavamoat/build-system/policy-override.json
index 34a2f3527..282e26ff0 100644
--- a/lavamoat/build-system/policy-override.json
+++ b/lavamoat/build-system/policy-override.json
@@ -14,6 +14,7 @@
     },
     "@eslint/eslintrc": {
       "packages": {
+        "<root>": true,
         "@babel/eslint-parser": true,
         "@babel/eslint-plugin": true,
         "@metamask/eslint-config": true,
diff --git a/test/e2e/tests/add-hide-token.spec.js b/test/e2e/tests/add-hide-token.spec.js
index 56fb2840c..066bd8782 100644
--- a/test/e2e/tests/add-hide-token.spec.js
+++ b/test/e2e/tests/add-hide-token.spec.js
@@ -55,6 +55,7 @@ describe('Hide token', function () {
   });
 });
 
+/* eslint-disable-next-line mocha/max-top-level-suites */
 describe('Add existing token using search', function () {
   const ganacheOptions = {
     accounts: [
diff --git a/test/e2e/tests/send-eth.spec.js b/test/e2e/tests/send-eth.spec.js
index 7c12e321b..f17f116d9 100644
--- a/test/e2e/tests/send-eth.spec.js
+++ b/test/e2e/tests/send-eth.spec.js
@@ -91,6 +91,7 @@ describe('Send ETH from inside MetaMask using default gas', function () {
   });
 });
 
+/* eslint-disable-next-line mocha/max-top-level-suites */
 describe('Send ETH from inside MetaMask using advanced gas modal', function () {
   const ganacheOptions = {
     accounts: [
diff --git a/test/e2e/tests/signature-request.spec.js b/test/e2e/tests/signature-request.spec.js
index 335826069..7cb53e233 100644
--- a/test/e2e/tests/signature-request.spec.js
+++ b/test/e2e/tests/signature-request.spec.js
@@ -86,6 +86,7 @@ describe('Sign Typed Data V4 Signature Request', function () {
   });
 });
 
+/* eslint-disable-next-line mocha/max-top-level-suites */
 describe('Sign Typed Data V3 Signature Request', function () {
   it('can initiate and confirm a Signature Request', async function () {
     const ganacheOptions = {