-
-
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/core@1.7.1 This package is used to read WASM files, which require the use of |
|
@SocketSecurity ignore npm/@tybys/wasm-util@0.10.1 This package is also used to read WASM files, which requires the use of |
|
@SocketSecurity ignore npm/unrs-resolver@1.11.1 This PR disables the post-install script for this package. |
|
@SocketSecurity ignore npm/@unrs/resolver-binding-wasm32-wasi@1.11.1 This package is also used to read WASM files, which requires the use of |
|
@SocketSecurity ignore npm/@typescript-eslint/typescript-estree@8.49.0 This is a known false positive. |
|
@SocketSecurity ignore npm/@typescript-eslint/utils@8.49.0 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/napi-postinstall@0.3.4 This package seems to download packages from the NPM registry as one of its tasks. It is used by |
src/editor.ts
Outdated
| * @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.
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 thought the way to do it is to add inline comments?
{
/**
* The foo property.
*/
foo: string;
}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.
Yes, that would work for properties that appear because of object types. But if you bring in other properties via unions or intersections, then those properties won't be documented. I figured it was helpful to document all properties of a type, not ones that are "uninherited" — is that a desire?
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.
True, but in this case all properties can be documended inline, right?
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.
Hmm, I'm not sure I'm following. Properties that are inherited cannot be documented inline. For instance, when hovering over this type or generating docs for this type, only bar will show up as a property, but foo will be hidden.
type Base = {
/**
* The 'foo' property.
*/
foo: 'bar'
}
/**
* The Foo type.
*/
type Foo = Base & {
/**
* The 'bar' property.
*/
bar: 'baz'
}What I am proposing is a way to consistently document all properties of a type, e.g.:
/**
* The Foo type.
*
* Properties:
*
* - `foo` - The 'foo' property.
* - `bar` - The 'bar' property.
*/
type Foo = Base & {
bar: 'baz'
}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.
Yes, that would make more sense to use @property. I believe a while back I tried to propose this, but I don't remember what the argument against it was.
I know that the jsdoc plugin has a rule check-tag-names
that prohibits this, but we could disable it.
@Gudahtt Do you have an opinion on this?
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 was hesitant about @property because it's not supported by TSDoc, so some tooling might not work with it. But if Intellisense supports it, it sounds like a good alternative when inline blocks aren't feasible.
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.
Apparently typedoc does support @property, and we can add custom definitions for TSDoc.
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.
It's a bit confusing; there's typedoc and then there's TSDoc, which is a standard that Microsoft made (as a TypeScript-specific version of JSDoc, presumably). TSDoc doesn't support @property, but you can also add custom tags: https://tsdoc.org/pages/packages/tsdoc-config/
We don't need to worry about TSDoc yet, but it's a good point to keep it in mind.
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.
Okay, I replaced these with the use of @property: 4331589. I had to disable the check-tag-names rule (or rather, disable the type-aware checks within this rule) because it doesn't allow us to disable checking @property, it will complain. I used no-restricted-syntax to get around this.
| * | ||
| * @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.
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.
IntelliJ has a quick option for it to add the return type explicitly which mostly works, with small modifications sometimes. But your refactor looks good to me too.
| 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.
eslint.config.mjs
Outdated
|
|
||
| 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).
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.
Nice, this makes a lot of sense to me. We should definitely upstream this.
| }, | ||
| ], | ||
| // 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.
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.
Cool, definitely in favour of moving this to eslint-config, but maybe release a fix with non-breaking changes first.
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.
Yeah, good point.
| '@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,
}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 didn't know there was an option for this. It definitely was a bit too strict in these cases, so this looks great.
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 considered suggesting this option as well. I didn't ultimately because there are some function expressions that are complex where a return type would be useful, so I wasn't sure it was what we wanted.
But still, while imperfect, it seems like a good compromise.
| ]); | ||
|
|
||
| // Consider copying this to @metamask/eslint-config | ||
| const requireJsdocOverride = { |
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 updated these rules again. Now we don't require JSDoc for type interfaces, type aliases, enums, or classes defined within other scopes. The selector for ArrowFunctionExpression and FunctionExpression are also fixed as they had bugs.
|
@SocketSecurity ignore npm/globals@14.0.0 This just exports a JSON file, it does not make |
|
@SocketSecurity ignore npm/@emnapi/core@1.8.1 This package loads WASM code and requires the use of |
|
@SocketSecurity ignore npm/globals@15.15.0 This just exports a JSON file, it does not make |
|
Updated to fix merge conflicts. |
| } catch (fetchingPackagesError) { | ||
| setError(getErrorMessage(fetchingPackagesError)); | ||
| console.error('Error fetching packages:', 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.
Try-catch doesn't wrap fetch, leaving errors unhandled
Medium Severity
The try-catch block in fetchPackages only wraps the data processing code (lines 132-150), but the fetch() call, !response.ok check, and response.json() call (lines 122-130) are outside the try-catch. This means network errors, HTTP error responses (like 404 or 500), and JSON parsing errors will not be caught, and setError() will never be called, so users won't see any error message. The original promise-chain code used .catch() at the end which caught all errors. The other async functions like checkDependencies and handleSubmit correctly wrap their entire operation in try-catch.
| }, | ||
|
|
||
| { | ||
| files: ['src/ui/**.tsx'], |
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.
Invalid glob pattern won't match UI component files
Medium Severity
The glob pattern src/ui/**.tsx is non-standard and likely won't match files like src/ui/App.tsx, src/ui/PackageItem.tsx, etc. The ** globstar is designed for matching directory segments, and when followed directly by .tsx without a path separator, the pattern may not work as intended in minimatch (used by ESLint). The standard pattern for matching all .tsx files recursively is src/ui/**/*.tsx, or src/ui/*.tsx for files directly in that directory. As a result, the browser and React ESLint rules configured in this block won't be applied to any UI components.
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
Modernizes linting and aligns with current MM standards.
.eslintrc.cjswith flateslint.config.mjs; adopt@metamask/eslint-configv15 stack, ESLint 9,import-x, and updated TypeScript-ESLint; refinejsdoc/*and naming rules (esp. UI)package.jsonscripts (lint:eslint->eslint .) and devDependencies/allow-scripts; ensure config files are lintedgetErrorMessage, stableSemVerusage (.version), import ordering, async signatures, narrowed types, and improved Express/UI typing/handlersWritten by Cursor Bugbot for commit 34d3ec5. This will update automatically on new commits. Configure here.