-
-
Notifications
You must be signed in to change notification settings - Fork 5
Upgrade to MM eslint-config-* v15 + ESLint 9 #168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
Ignoring alerts on:
|
|
@SocketSecurity ignore npm/@emnapi/[email protected] This package is used to read WASM files, which require the use of |
|
@SocketSecurity ignore npm/@tybys/[email protected] This package is also used to read WASM files, which requires the use of |
|
@SocketSecurity ignore npm/[email protected] This PR disables the post-install script for this package. |
|
@SocketSecurity ignore npm/@unrs/[email protected] This package is also used to read WASM files, which requires the use of |
|
@SocketSecurity ignore npm/@typescript-eslint/[email protected] This is a known false positive. |
|
@SocketSecurity ignore npm/@typescript-eslint/[email protected] This is a known false positive. |
The ESLint configuration is behind for this package. Keeping up to date with our current lint rules and lint config format helps to debug issues with ESLint now and in the future, and aligns this repo with our other repos. A list of changes: - Upgrade `@metamask/eslint-config` and `@metamask/eslint-config-*` packages to 15.0.0 - Upgrade ESLint to 9.x - Upgrade TypeScript ESLint packages to 8.25.x - Upgrade other ESLint packages to fulfill peer dependencies - Fix lint violations as a result of the upgrade Note that the rules for `jsdoc/require-jsdoc` have been refined. In performing this upgrade, many violations appeared that I thought didn't help readability.
|
@SocketSecurity ignore npm/[email protected] This package seems to download packages from the NPM registry as one of its tasks. It is used by |
| * @property path - The path to the executable representing the editor. | ||
| * @property args - Command-line arguments to pass to the executable when | ||
| * calling it. | ||
| * Properties: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@property is now no longer allowed as a tag. I wanted a consistent way that we could document properties going forward, accounting for intersections or unions, so I made up this format. Let me know what you think.
| * | ||
| * @returns The corresponding mock functions for each of the dependencies. | ||
| */ | ||
| function getDependencySpies() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing out the return type for this function was going to be a huge chore, so I refactored the setup of these mocks.
| minor = 'minor', | ||
| patch = 'patch', | ||
| } | ||
| export const IncrementableVersionParts = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to convert this to a "quasi-enum" as it made it easier to adapt to the new lint violations.
|
|
||
| rules: { | ||
| // Consider copying this to @metamask/eslint-config | ||
| 'jsdoc/require-jsdoc': [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated in the PR description, I found the new changes to this rule in MetaMask/eslint-config#394 rather strict. I removed TSPropertySignature as it began asking that properties in adhoc object types were documented. For instance:
function foo(): {
// This is now required to be documented
bar: string;
} {
// ...
}I also found a similar thing for arrow functions:
// The arrow function here is now required to be documented
foo(() => {
// ...
})
foo({
// The arrow function here is now required to be documented
bar: () => {
// ...
}
})So, I amended what this rule is looking for, which is explained in the comments below. I feel like this makes more sense, but happy to know y'all's thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reference, I used https://astexplorer.net/ to test out the selectors (make sure to select @typescript-eslint/parser and Transform > ESLint v8).
| }, | ||
| ], | ||
| // Consider copying this to @metamask/eslint-config | ||
| 'jsdoc/no-blank-blocks': 'error', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that this existed and I found this to be a helpful rule. It seemed that with the new JSDoc rules, running yarn eslint --fix added a bunch of empty JSDoc blocks to various symbols. With this enabled, I was able to quickly find those and fill them in.
| '@typescript-eslint/explicit-function-return-type': [ | ||
| 'error', | ||
| { | ||
| allowExpressions: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I found that this new rule was too strict. For instance:
return {
// A return type is now required for this function
message: () => 'foo',
pass: false,
}
The ESLint configuration is behind for this package. Keeping up to date with our current lint rules and lint config format helps to debug issues with ESLint now and in the future, and aligns this repo with our other repos.
A list of changes:
@metamask/eslint-configand@metamask/eslint-config-*packages to 15.0.0Note that the rules for
jsdoc/require-jsdochave been refined. In performing this upgrade, many violations appeared that I thought didn't help readability. We may consider porting these back to theeslint-configrepo.Note
Upgrade lint stack to ESLint 9 + MetaMask eslint-config v15 (flat config) and apply code/test fixes to satisfy new rules.
.eslintrc.cjswith flateslint.config.mjs; add ignores andimport-xsettings; override.cjs/.jstoscriptmode (exceptbin/create-release-branch.js).eslint@9,@metamask/eslint-config(-*)@15,@typescript-eslintv8; adopteslint-plugin-import-x,eslint-import-resolver-typescript,eslint-plugin-n,eslint-plugin-promise.package.jsonscripts (eslint .) and dev deps; add LavaMoat allowlist foreslint-plugin-import-x>unrs-resolver.jsdoc/require-jsdocandjsdoc/no-blank-blocks.@typescript-eslint/explicit-function-return-type(expressions allowed).Promise<void>,string), refine types and exports, reorder/import updates.getErrorMessagefor logging; small message tweaks and safer reductions.Written by Cursor Bugbot for commit b62321e. This will update automatically on new commits. Configure here.