chore: bump ESLint stack to v15 and migrate to flat config - #317
chore: bump ESLint stack to v15 and migrate to flat config#317cryptodev-2s wants to merge 4 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution MetaMask internal reviewing guidelines:
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6551ea9. Configure here.
6551ea9 to
311128a
Compare
311128a to
4adc549
Compare
16d390f to
9eb1d3a
Compare
Replaces .eslintrc.js with eslint.config.mjs and moves the whole ESLint
stack to the versions core uses.
eslint ^8.44.0 -> ^9.39.1
@metamask/eslint-config* ^12.0.0 -> ^15.0.0
@metamask/auto-changelog ^3.1.0 -> ^6.1.0
eslint-plugin-jest ^27.2.2 -> ^28.8.3
eslint-plugin-jsdoc ^39.9.1 -> ^50.2.4
eslint-plugin-n ^15.7.0 -> ^17.10.3
eslint-plugin-promise ^6.1.1 -> ^7.1.0
eslint-plugin-import removed, replaced by import-x
@typescript-eslint/{parser,plugin} removed, replaced by typescript-eslint
Added: typescript-eslint ^8.48.0, eslint-plugin-import-x ^4.3.0,
eslint-import-resolver-typescript ^3.6.3, @types/eslint ^9.6.1.
The flat config carries over the prettier/prettier and import-x/order
disables set in .eslintrc.js by the previous commit, since Oxfmt owns
formatting.
Also adds @types/semver ^7. It was previously reaching us transitively via
@typescript-eslint/eslint-plugin, so dropping that package broke the build
until it was declared directly.
Config v15 surfaced problems v12 never enforced. Most are stylistic and are
turned off with TODO comments, matching core. The rest were stale
eslint-disable directives pointing at rules typescript-eslint v8 renamed or
removed:
@typescript-eslint/no-throw-literal -> only-throw-error (8 sites)
@typescript-eslint/ban-types -> now unnecessary (3 sites)
jest/no-if -> removed in v28 (1 site)
Two autofixes were reverted because they changed behaviour rather than style:
hasProperty's constraint was rewritten from `Object` to `object`. The
original carried an explicit suppression, so it was deliberate: `Object`
accepts boxed primitives, and narrowing it would break callers passing a
string or number.
In json.test-d.ts the fixer stripped `as any` and `as number | undefined`
from tsd assertions. Those assertions are the test. no-unnecessary-type-
assertion is now off for *.test-d.ts so it cannot happen again.
jsdoc/require-jsdoc stays off for the same reason core keeps it off: its
autofixer inserts empty JSDoc blocks and mangles the surrounding code.
Verified: eslint, build, 23 suites / 1993 tests at 100% coverage, tsd, and
the full yarn lint all pass.
It was copied from smart-transactions-controller#595 without checking whether this package needs it. It does not: nothing here imports eslint types, eslint.config.mjs carries no annotations, and it is only an optional peer of eslint-plugin-prettier. Core does not have it at its root either. Removing it changes nothing: eslint, build, tests and lint all still pass.
0418ea3 to
cb576bc
Compare
Removing the stale eslint-disable comments left their newlines in place, so 16 stray blank lines ended up between JSDoc blocks and the declarations they document, and inside parameter lists. The cause is the autofixer for reportUnusedDisableDirectives: it deletes the directive text but not the line it sat on. I ran eslint --fix and did not review the whitespace only part of the diff. Nothing in the toolchain flags this. lines-around-comment, @typescript-eslint/lines-around-comment and no-multiple-empty-lines are all off in the shared config, and Oxfmt only collapses two or more consecutive blank lines, treating a single one as deliberate. Cosmetic rather than functional: TypeScript still associates a JSDoc block with the declaration across a blank line, verified by emitting declarations for both shapes and confirming the comment survives in the .d.ts.
| export * from './coercers'; | ||
| export * from './collections'; | ||
| export * from './encryption-types'; | ||
| export type * from './encryption-types'; |
There was a problem hiding this comment.
@typescript-eslint/consistent-type-exports
…ion emit no-unnecessary-type-assertion flagged `JsonRpcErrorStruct as Struct<JsonRpcError>` and its autofixer removed it. The rule is right that the assertion does not change assignability, and wrong that it is unnecessary: it pins what TypeScript emits. Without it the declaration inlines the structure instead of referring to the named type, so JsonRpcFailure and JsonRpcResponse changed from error: JsonRpcError; to an anonymous object literal, which also pushed the unexported internal ExactOptionalGuard further into the published types (9 occurrences in json.d.cts before, 11 after). Restored with a suppression explaining why. json.d.cts is now byte identical to the one built from main. Found by diffing the emitted declarations against main rather than by any test: assignability is unchanged, so nothing fails. Only the shape of the published types differs.
mcmire
left a comment
mcmire
left a comment
There was a problem hiding this comment.
Thanks for doing this. This mostly looks good but I called out some things below. I need to do another pass on this but I do have a suggestion to consider below for now.
| @@ -1,3 +1,8 @@ | |||
| /* eslint-disable @typescript-eslint/no-unnecessary-type-assertion -- | |||
| The `as Hex` assertions in the `it.each` tables below look redundant per | |||
There was a problem hiding this comment.
What if we convert as Hex to as const instead, would we then need this?
| /** | ||
| * Predefined sizes (in Bytes) of specific parts of JSON structure. | ||
| */ | ||
| /* eslint-disable @typescript-eslint/no-duplicate-enum-values -- |
There was a problem hiding this comment.
This is fine for now. When we ban enums finally this should go away, so nothing to worry about.
| rules: { | ||
| // TODO: Re-enable these rules. | ||
| // Newly surfaced by eslint-config v15; not enforced under v12. | ||
| '@typescript-eslint/explicit-function-return-type': 'off', |
There was a problem hiding this comment.
Hmm, I thought I fixed this already. I guess not. This is okay for now.
| settings: { | ||
| // `uuid` ships an exports map that the TypeScript resolver cannot read, | ||
| // so `import-x` sees no named exports and false positives on `uuid.v4`. | ||
| 'import-x/ignore': ['uuid'], |
There was a problem hiding this comment.
Hmm, interesting. Are we using the ESM version of uuid? We've never had to do this before so it feels like there ought to be another way to do this.
| { | ||
| files: ['**/*.test.{js,ts}'], | ||
| rules: { | ||
| // These tests deliberately reach for `crypto` and `crypto.webcrypto`, |
There was a problem hiding this comment.
Probably okay, but I need to double-check this is the right thing to do here.
2 participants
Stacked on #314.
Replaces
.eslintrc.jswith a flateslint.config.mjsand moves the ESLint stack to core's versions.eslint^8.44.0^9.39.1@metamask/eslint-config{,-jest,-nodejs,-typescript}^12.0.0^15.0.0@metamask/auto-changelog^3.1.0^6.1.0eslint-plugin-jest^27.2.2^28.8.3eslint-plugin-jsdoc^39.9.1^50.2.4eslint-plugin-n^15.7.0^17.10.3eslint-plugin-promise^6.1.1^7.1.0Drops
eslint-plugin-importand@typescript-eslint/{parser,eslint-plugin}foreslint-plugin-import-xandtypescript-eslint. Adds@types/semver@^7, which had been arriving transitively through@typescript-eslint/eslint-pluginand broke the build once that was removed.Config v15 surfaced 351 problems v12 never enforced. Stylistic ones are off with TODO comments, matching core. The rest were stale directives naming rules that typescript-eslint v8 renamed or removed, so the code under them was going unchecked:
@typescript-eslint/no-throw-literalonly-throw-error@typescript-eslint/ban-typesimport/no-nodejs-modulesimport-x/no-nodejs-modulesjest/no-ifTwo autofixes are reverted because they changed behaviour rather than style:
hasProperty's constraint was rewritten fromObjecttoobject. The original carried an explicit suppression, soObjectwas deliberate: it accepts boxed primitives, and narrowing it breaks callers passing a string or number.json.test-d.tslostas anyandas number | undefinedfrom its tsd assertions. In type tests the assertion is the test.no-unnecessary-type-assertionis now off for*.test-d.ts.jsdoc/require-jsdocstays off rather than suppressed: its autofixer inserted 193 empty JSDoc blocks on the first pass. Core disables it for the same reason.Note
Low Risk
Tooling and lint-only source edits; no intentional runtime behavior changes beyond type-only export syntax for a few modules.
Overview
Migrates linting from legacy
.eslintrc.jsto ESLint 9 flat config ineslint.config.mjs, bumping@metamask/eslint-config(and jest/nodejs/typescript presets) to v15 and aligning related devDependencies (eslint-plugin-import-x,typescript-eslint, plugin major bumps,@types/semveras a direct dep).lint:eslintdrops--ext js,ts; new config adds file-specific overrides (JSDoc/TS rules left off with TODOs like core,*.test-d.tsand crypto polyfill tests exempted,uuidignored for import-x).Source updates are mostly lint hygiene: renamed rule directives (
only-throw-error,import-x/no-nodejs-modules), removed staleeslint-disableblocks in fixtures, targeted suppressions where autofix would change behavior (hasPropertystill usesObject,JsonRpcFailureStructkeeps a type assertion for published types).src/index.tsswitches several modules toexport type *so only types are re-exported.Reviewed by Cursor Bugbot for commit 66665c1. Bugbot is set up for automated code reviews on this repo. Configure here.