Refactor ESLint config #13482

mcmire · 2022-02-01T20:47:32Z

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.

Testing steps:

Run yarn lint. This command should not fail. (This is probably checked on CI anyway.)
Run yarn start. This command should not fail with a linting error.
Run yarn dist. This command should not fail with a linting error.

github-actions · 2022-02-01T20:47:48Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

mcmire · 2022-02-01T22:37:05Z

.eslintrc.js

+      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',


In refactoring this config, I discovered that ESLint was confused over whether this file was a script or a module. It seemed that this was not possible because I thought that we'd configured eslint-plugin-import to work with CommonJS. That is true, but that setting only applies to the no-unresolved rule. The no-ambiguous rule is separate and only seems to work on ES module files . As far as I can tell, this is a long-standing bug and not a feature. In the meantime, we just have to turn this rule off for CommonJS files.

metamaskbot · 2022-02-01T23:09:34Z

Builds ready [ba551c5]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1066 ± 19 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	65	1087	222	240	115
		domContentLoaded	1022	1184	1066	40	19
		load	1022	1184	1066	40	19
		domInteractive	1022	1183	1066	40	19

metamaskbot · 2022-02-02T18:09:50Z

Builds ready [8a2321b]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1159 ± 67 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	67	1070	217	255	122
		domContentLoaded	1062	1595	1159	140	67
		load	1062	1595	1159	140	67
		domInteractive	1062	1595	1159	140	67

brad-decker

LGTM.

brad-decker · 2022-02-03T14:28:20Z

lavamoat/build-system/policy-override.json

    },
    "@eslint/eslintrc": {
      "packages": {
+        "<root>": true,


[Q] What does this mean? Why was added and is it a good thing?

When running yarn lavamoat:auto after these changes, I saw this message show up:

[beta] scripts:core:prod:standardEntryPoints: (node:91080) UnhandledPromiseRejectionWarning: Error: Cannot read config file: /Users/elliot/code/metamask/metamask-extension/.eslintrc.base.js [beta] Error: LavaMoat - required package not in whitelist: package "@eslint/eslintrc" requested "<root>" as "/Users/elliot/code/metamask/metamask-extension/.eslintrc.base.js" [beta] Referenced from: BaseConfig#overrides[0] while parsing file: /Users/elliot/code/metamask/metamask-extension/ui/helpers/constants/routes.js [beta] at requireRelative (LavaMoat/core/kernel:519:15) [beta] at requireRelativeWithContext (LavaMoat/core/kernel:471:16) [beta] at loadJSConfigFile (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:225:16) [beta] at loadConfigFile (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:309:20) [beta] at ConfigArrayFactory._loadConfigData (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:609:42) [beta] at ConfigArrayFactory._loadExtendedShareableConfig (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:889:21) [beta] at ConfigArrayFactory._loadExtends (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:781:25) [beta] at ConfigArrayFactory._normalizeObjectConfigDataBody (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:720:25) [beta] at _normalizeObjectConfigDataBody.next (<anonymous>) [beta] at ConfigArrayFactory._normalizeObjectConfigData (/Users/elliot/code/metamask/metamask-extension/node_modules/@eslint/eslintrc/lib/config-array-factory.js:665:20)

I am not sure what LavaMoat needs access to in @eslint/eslintrc exactly. But after reviewing the documentation for the policy file, I think that <root> is a quirk of the format of the policy file and is a self-reference to @eslint/eslintrc within the config for @eslint/eslintrc. In other words I believe it instructs LavaMoat to process @eslint/eslintrc, as if you'd said this:

{ "packagesToProcess": { "@eslint/eslintrc": { "packagesToProcess": { "<this package>": true, "@babel/eslint-parser": true, } } } }

One would think that if you add a package to the list of resources that LavaMoat would process it, but it looks like that's not true and you have to be explicit.

That doesn't sound right. LavaMoat isn't asking for access to anything, it has access already. All packages have access to everything by default, that's what LavaMoat is preventing. LavaMoat is gating access, it's controlling which packages and APIs each package can access.

Here is the relevant portion of the quoted error message:

package "@eslint/eslintrc" requested "<root>" as "/Users/elliot/code/metamask/metamask-extension/.eslintrc.base.js"

Which means pretty much what it sounds like - the @eslint/eslintrc package is requesting the <root> package, but it wasn't explicitly granted access to that package so it was denied. The <root> package here is not @eslint/eslintrc, it's the root project, the MetaMask extension itself.

I'm guessing this is the case because now the .eslintrc.js file is extending another module in this repository. So when @eslint/eslintrc tries parsing that config file, it will have to import the base config as well, thus requiring access to the MetaMask extension itself. It didn't need that before, because we weren't extending from another config in this repo before. We were just extending from @metamask/eslint-config (which was already added to the policy override a few lines down from here, for this same reason).

Ahh, okay. That makes more sense.

mcmire · 2022-02-09T18:40:42Z

There were some good suggestions provided in the walkthrough call today on how we should add TS to ESLint in a way that doesn't decrease performance. I'm going to move this back into draft status as this will prompt some changes.

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.

test/e2e/tests/send-eth.spec.js

metamaskbot · 2022-02-15T23:24:56Z

Builds ready [fd16fdf]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1105 ± 40 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	68	1127	327	394	189
		domContentLoaded	1019	1396	1104	83	40
		load	1019	1396	1105	82	40
		domInteractive	1019	1396	1104	83	40

mcmire · 2022-02-25T19:19:33Z

.eslintrc.babel.js

@@ -0,0 +1,9 @@
+module.exports = {


Babel-specific configuration was pulled out into its own file from .eslintrc, but there should be no changes with what was there before.

mcmire · 2022-02-25T19:20:03Z

.eslintrc.base.js

@@ -0,0 +1,67 @@
+const path = require('path');


Base configuration was pulled out into its own file from .eslintrc, but there should be no changes with what was there before.

mcmire · 2022-02-25T19:20:52Z

.eslintrc.jsdoc.js

@@ -0,0 +1,23 @@
+module.exports = {


JSDoc-specific configuration was pulled out into its own file from .eslintrc, but there should be no changes with what was there before.

mcmire · 2022-02-25T19:22:45Z

development/build/transforms/utils.js

-// Sometimes we use `let` instead of `const` to assign variables depending on
-// the build type.
-eslintrc.rules['prefer-const'] = 'off';
+eslintrc.overrides.forEach((override) => {


Changes are necessary to this file (which is part of the build system) because we run ESLint during the build system. To do this we pull in the ESLint configuration object from .eslintrc.js and we explicitly modify that object to disable some rules as this would cause failures which we are not interested in. Because ALL ESLint rules are now in overrides, I have to change how these modifications work.

mcmire · 2022-02-25T19:25:15Z

test/e2e/tests/add-hide-token.spec.js

  });
 });

+/* eslint-disable-next-line mocha/max-top-level-suites */


With the updates to the ESLint config, these test files are now failing the lint step. I suspect this is the case because not all of the Mocha rules were getting applied to the e2e tests previously and now they are.

metamaskbot · 2022-02-25T20:02:21Z

Builds ready [62d95fe]

builds: chrome, firefox, opera
builds (beta): chrome, firefox, opera
builds (flask): chrome, firefox, opera
build viz: Build System
code coverage: Report
storybook: Storybook
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- phishing-detect: phishing-detect
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1235 ± 66 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	70	1267	501	437	210
		domContentLoaded	1060	1671	1234	137	66
		load	1060	1671	1235	137	66
		domInteractive	1060	1671	1234	137	66

brad-decker

LGTM

mcmire requested review from a team and kumavis as code owners February 1, 2022 20:47

mcmire requested a review from jpuri February 1, 2022 20:47

mcmire added the Team 🌳 label Feb 1, 2022

mcmire force-pushed the refactor-eslint-config branch from 81c1e52 to f410eda Compare February 1, 2022 21:02

mcmire requested a review from Gudahtt February 1, 2022 21:03

mcmire force-pushed the refactor-eslint-config branch 2 times, most recently from 56c33ea to ba551c5 Compare February 1, 2022 22:33

mcmire commented Feb 1, 2022

View reviewed changes

mcmire force-pushed the refactor-eslint-config branch from ba551c5 to 8a2321b Compare February 2, 2022 17:30

brad-decker previously approved these changes Feb 3, 2022

View reviewed changes

mcmire marked this pull request as draft February 9, 2022 18:40

mcmire dismissed brad-decker’s stale review via 4b74949 February 15, 2022 06:34

mcmire force-pushed the refactor-eslint-config branch from 8a2321b to 4b74949 Compare February 15, 2022 06:34

mcmire marked this pull request as ready for review February 15, 2022 06:34

mcmire force-pushed the refactor-eslint-config branch from 4b74949 to c983c6c Compare February 15, 2022 22:28

mcmire force-pushed the refactor-eslint-config branch from c983c6c to fd16fdf Compare February 15, 2022 22:46

mcmire commented Feb 15, 2022

View reviewed changes

test/e2e/tests/send-eth.spec.js Outdated Show resolved Hide resolved

mcmire added 2 commits February 25, 2022 12:23

Use eslint-disable-next-line, not just eslint-disable

c348ec3

Add builds/ to the ignore list

62d95fe

mcmire commented Feb 25, 2022

View reviewed changes

brad-decker approved these changes Feb 25, 2022

View reviewed changes

darkwing approved these changes Feb 25, 2022

View reviewed changes

mcmire merged commit 1e494f3 into develop Feb 28, 2022

mcmire deleted the refactor-eslint-config branch February 28, 2022 17:42

github-actions bot locked and limited conversation to collaborators Feb 28, 2022

Refactor ESLint config #13482

Refactor ESLint config #13482

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Feb 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 1, 2022

Uh oh!

metamaskbot commented Feb 2, 2022

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 Feb 9, 2022

Uh oh!

Uh oh!

metamaskbot commented Feb 15, 2022

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 Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 25, 2022

Uh oh!

brad-decker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mcmire commented Feb 1, 2022 •

edited

Loading

mcmire Feb 4, 2022 •

edited

Loading

mcmire Feb 25, 2022 •

edited

Loading