feat: allow custom ESLint rules and add get-text-anti-pattern #27591

HowardBraham · 2024-10-03T01:55:45Z

Description

UPDATE October 23: this is ready for comments now, but not ready for merging, waiting on #28041, #28043, and probably more

Allows us to write custom ESLint rules directly in our repo, without using an external NPM package.

I evaluated several packages to do this, including:

eslint-plugin-rulesdir seemed the best for our purposes. I tried for several days to get this working with TypeScript rules, involving @davidmurdoch and @naugtur, but I think it's not going to play nice with LavaMoat. When we upgrade to ESLint 9, we can get rid of eslint-plugin-rulesdir, start using the special "local" plugin, and write TypeScript rules.

get-text-anti-pattern.js serves as an example rule. It's something I've been thinking about for months, for how to combat this prevalent anti-pattern #19870.

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

github-actions · 2024-10-03T01:55:56Z

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.

socket-security · 2024-10-03T01:56:42Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@esbuild/aix-ppc64@0.23.1	None	`0`	11 MB	esbuild, evanw
npm/@esbuild/openbsd-arm64@0.23.1	None	`0`	9.37 MB	evanw
npm/@metamask/eslint-config-mocha@13.0.0	None	`0`	9.21 kB	metamaskbot
npm/eslint-plugin-rulesdir@0.2.2	filesystem	`0`	8.32 kB	not-an-aardvark
npm/get-tsconfig@4.8.1	filesystem	`0`	105 kB	hirokiosame
npm/tsx@4.19.1	None	`+23`	221 MB	hirokiosame

🚮 Removed packages: npm/@esbuild/aix-ppc64@0.19.12, npm/@metamask/eslint-config-mocha@9.0.0, npm/get-tsconfig@4.7.5, npm/tsx@4.7.1

View full report↗︎

HowardBraham · 2024-10-03T01:57:39Z

package.json

@@ -546,6 +546,7 @@
    "@types/yargs-parser": "^21.0.3",
    "@typescript-eslint/eslint-plugin": "^7.10.0",
    "@typescript-eslint/parser": "^7.10.0",
+    "@typescript-eslint/utils": "^8.8.0",


The package @typescript-eslint/utils was already implicitly included, but this makes it explicit

HowardBraham · 2024-10-03T01:58:40Z

package.json

-    "tsx": "^4.7.1",
+    "tsx": "^4.19.1",


The tsx package was updated to the latest version in order to support require('tsx/cjs/api').register(tsconfig);

HowardBraham · 2024-10-03T01:59:53Z

test/setup.js

+  // Allow TypeScript to be used in Mocha
+  require: ['tsx/esm'],
+  'node-option': ['import=tsx'],


Mocha unit tests used to use ts-node, this updates them to the newest tsx

sonarcloud · 2024-10-16T23:41:42Z

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

HowardBraham · 2024-10-17T03:20:02Z

.eslintrc.js

-      files: ['test/e2e/**/*.spec.js'],
+      files: ['test/e2e/**/*.spec.{js,jsx,ts,tsx}'],


Doing this caused hundreds of new errors and warnings ☹️

metamaskbot · 2024-10-23T18:29:33Z

Builds ready [9b3104d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1788 ± 71 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1604	2119	1784	136	65
		domContentLoaded	1578	2049	1750	135	65
		load	1590	2145	1788	147	71
		domInteractive	14	80	41	15	7
		backgroundConnect	8	78	34	23	11
		firstReactRender	44	204	87	45	21
		getState	4	77	17	19	9
		initialActions	0	0	0	0	0
		loadScripts	1148	1543	1287	115	55
		setupStore	11	68	30	22	11
		uiStartup	1765	2534	2004	193	92

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

@HowardBraham

## **Description** Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text. There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci. Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's [PR ](#27591 adding a lint rule which prevents introducing it again. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28043?quickstart=1) ## **Related issues** Fixes: (but doesn't yet closes) #19870 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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. ## **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.

@HowardBraham

## **Description** Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text. There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci. Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's [PR ](#27591 adding a lint rule which prevents introducing it again. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28043?quickstart=1) ## **Related issues** Fixes: (but doesn't yet closes) #19870 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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. ## **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.

@HowardBraham

## **Description** Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text. There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci. Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's [PR ](#27591 adding a lint rule which prevents introducing it again. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28062?quickstart=1) ## **Related issues** Fixes: (but doesn't yet closes) #19870 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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. ## **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.

@HowardBraham

## **Description** Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text. There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci. Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's [PR ](#27591 adding a lint rule which prevents introducing it again. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28043?quickstart=1) ## **Related issues** Fixes: (but doesn't yet closes) #19870 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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. ## **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.

@HowardBraham

## **Description** Continuing the work of removing the e2e anti-pattern of finding the element and then asserting its text. There are more occurrences, but this work is split in several PRs, for an easy review and a faster ci. Once all occurrences have been fixed, we'll be able to merge @HowardBraham 's [PR ](#27591 adding a lint rule which prevents introducing it again. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/28062?quickstart=1) ## **Related issues** Fixes: (but doesn't yet closes) #19870 ## **Manual testing steps** 1. Check ci ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] 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. ## **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.

HowardBraham requested a review from davidmurdoch October 3, 2024 01:55

github-actions bot added the team-contributor-experience MetaMask Contributor Experience Group label Oct 3, 2024

HowardBraham commented Oct 3, 2024

View reviewed changes

HowardBraham force-pushed the custom-eslint-rules branch from 3360e1e to de2e64b Compare October 3, 2024 02:23

HowardBraham self-assigned this Oct 16, 2024

HowardBraham changed the title ~~feat: allow custom ESLint rules~~ feat: allow custom ESLint rules and add get-text-anti-pattern Oct 16, 2024

HowardBraham force-pushed the custom-eslint-rules branch from de2e64b to 6be006a Compare October 16, 2024 23:30

HowardBraham added the type-tech-debt Technical debt label Oct 17, 2024

HowardBraham commented Oct 17, 2024

View reviewed changes

HowardBraham force-pushed the custom-eslint-rules branch 2 times, most recently from 7ae8a41 to 7e0e2e9 Compare October 18, 2024 09:00

MetaMask deleted a comment from metamaskbot Oct 18, 2024

HowardBraham force-pushed the custom-eslint-rules branch 2 times, most recently from 2406fe0 to 5dcf6b3 Compare October 18, 2024 19:59

MetaMask deleted a comment from metamaskbot Oct 18, 2024

HowardBraham added team-tiger Tiger team (for tech debt reduction + performance improvements) contributor experience An issue that impacts, or planned improvement to, the contributor experience. and removed team-contributor-experience MetaMask Contributor Experience Group labels Oct 18, 2024

HowardBraham force-pushed the custom-eslint-rules branch from 5dcf6b3 to 3c31f14 Compare October 22, 2024 22:09

This was referenced Oct 23, 2024

fix: flaky anti-pattern getText + assert #28041

Open

fix: flaky anti-pattern getText + assert 2 #28043

Merged

HowardBraham and others added 6 commits October 23, 2024 09:39

feat: allow custom ESLint rules and add get-text-anti-pattern

72bebf9

updated @metamask/eslint-config-mocha

51a613e

split {jsx,ts,tsx}

4d15d13

with a fixer

a9dd73a

Update LavaMoat policies

692b6c8

development/build require('eslint-plugin-rulesdir');

e2ca7cc

disable eslint-plugin-rulesdir in fenced code linting

be3dc57

HowardBraham force-pushed the custom-eslint-rules branch from 3c31f14 to be3dc57 Compare October 23, 2024 16:39

undo spec changes

9b3104d

HowardBraham added flaky tests and removed contributor experience An issue that impacts, or planned improvement to, the contributor experience. labels Oct 23, 2024

HowardBraham added the DO-NOT-MERGE Pull requests that should not be merged label Oct 23, 2024

HowardBraham requested review from vthomas13, seaona and hjetpoluru October 23, 2024 22:06

seaona mentioned this pull request Oct 24, 2024

fix: flaky anti-pattern getText + assert 3 #28062

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow custom ESLint rules and add get-text-anti-pattern #27591

feat: allow custom ESLint rules and add get-text-anti-pattern #27591

HowardBraham commented Oct 3, 2024 •

edited

Loading

github-actions bot commented Oct 3, 2024

socket-security bot commented Oct 3, 2024 •

edited

Loading

HowardBraham Oct 3, 2024

HowardBraham Oct 3, 2024

HowardBraham Oct 3, 2024

sonarcloud bot commented Oct 16, 2024

HowardBraham Oct 17, 2024

metamaskbot commented Oct 23, 2024

		files: ['test/e2e/*/.spec.js'],
		files: ['test/e2e/*/.spec.{js,jsx,ts,tsx}'],

feat: allow custom ESLint rules and add get-text-anti-pattern #27591

Are you sure you want to change the base?

feat: allow custom ESLint rules and add get-text-anti-pattern #27591

Conversation

HowardBraham commented Oct 3, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 3, 2024

socket-security bot commented Oct 3, 2024 • edited Loading

HowardBraham Oct 3, 2024

Choose a reason for hiding this comment

HowardBraham Oct 3, 2024

Choose a reason for hiding this comment

HowardBraham Oct 3, 2024

Choose a reason for hiding this comment

sonarcloud bot commented Oct 16, 2024

Quality Gate failed

HowardBraham Oct 17, 2024

Choose a reason for hiding this comment

metamaskbot commented Oct 23, 2024

HowardBraham commented Oct 3, 2024 •

edited

Loading

socket-security bot commented Oct 3, 2024 •

edited

Loading