Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bump ESLint and related dependencies #254

Merged
merged 2 commits into from
Nov 6, 2024
Merged

Bump ESLint and related dependencies #254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cf5f2d
Bump ESLint and related dependencies
Mrtenz Nov 4, 2024
da14e6a
Remove unnecessary ignores
Mrtenz Nov 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .depcheckrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,16 @@
"@lavamoat/allow-scripts",
"@lavamoat/preinstall-always-fail",
"@metamask/auto-changelog",
"@metamask/eslint-config",
"@metamask/eslint-config-*",
"@types/*",
"@yarnpkg/types",
"eslint-config-*",
"eslint-import-resolver-typescript",
"eslint-plugin-*",
"prettier-plugin-packagejson",
"ts-node",
"typedoc"
"typedoc",
"typescript-eslint"
]
}
50 changes: 0 additions & 50 deletions .eslintrc.js
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion .github/workflows/create-release-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ jobs:
uses: actions/setup-node@v4
with:
node-version-file: '.nvmrc'
- uses: MetaMask/action-create-release-pr@v3
- uses: MetaMask/action-create-release-pr@v4
with:
release-type: ${{ github.event.inputs.release-type }}
release-version: ${{ github.event.inputs.release-version }}
Expand Down
5 changes: 4 additions & 1 deletion .prettierrc.js → .prettierrc.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
// All of these are defaults except singleQuote, but we specify them
// for explicitness
module.exports = {
const config = {
mikesposito marked this conversation as resolved.
Show resolved Hide resolved
quoteProps: 'as-needed',
singleQuote: true,
tabWidth: 2,
trailingComma: 'all',
plugins: ['prettier-plugin-packagejson'],
};

export default config;
47 changes: 47 additions & 0 deletions eslint.config.mjs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that the createConfig function was already released and published. However, I can't help but wonder whether it will, in the long run, make our ESLint configurations unnecessarily difficult to understand and debug. ESLint already provides recommendations for combining configuration objects (and exporting configs so they can be properly combined), and these are what plugins like eslint-plugin-import-x, eslint-plugin-jsdoc, and eslint-plugin-prettier follow in switching to the flat config format.

Upon switching to ESLint 9, I would expect the config file to look like the following:

import baseConfigs from '@metamask/eslint-config';
import typescriptConfigs from '@metamask/eslint-config-typescript';
import nodejsConfigs from '@metamask/eslint-config-nodejs';
import jestConfigs from '@metamask/eslint-config-jest';

export default [
  ...baseConfigs,
  {
    ignores: ['dist/', 'docs/', '.yarn/'],
  },
  {
    languageOptions: {
      sourceType: 'module'
    },
    settings: {
      'import-x/extensions': ['.js', '.mjs'],
    },
  },
  ...typescriptConfigs.map((config) => ({
    files: ['**/*.ts'],
    languageOptions: {
      parserOptions: {
        tsconfigRootDir: import.meta.dirname,
        project: ['./tsconfig.json'],
      },
    },
  }}),
  ...nodeJsConfigs.map((config) => ({
    ...config,
    files: ['**/*.js', '**/*.cjs'],
    languageOptions: {
      sourceType: 'script',
    },
  })),
  ...nodeJsConfigs.map((config) => ({
    ...config,
    files: ['**/*.test.ts', '**/*.test.js'],
    languageOptions: {
      sourceType: 'script',
    },
  })),
  ...jestConfigs.map((config) => ({
    ...config,
    files: ['**/*.test.ts', '**/*.test.js'],
  })),
];

While using map is a bit more verbose, I think it's very clear that we are creating objects to add to the flat array that we are ultimately exporting. I am not sure that is so clear with the createConfig utility.

What are your thoughts on this? I believe there are also further improvements we can make to the ESLint packages so that we can streamline this file, but I will leave that for a future comment.

Copy link
Member Author

@Mrtenz Mrtenz Nov 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this, and after talking about it with @rekmarks, we decided to create a simple util function that handles it for you. I don't think it changes so much in understandability of the config, and because it's less verbose, I feel like it's easier to read.

The function is based on typescript-eslint's config function, so it's not like we're the only ones doing this.

Upon switching to ESLint 9, I would expect the config file to look like the following:

[...]

This gets very verbose if you actually want to override some rules or other settings. For example:

...typescript.map((config) => {
  ...config,
  rules: {
    ...config.rules,
    'some-rule': 'off',
  },

  languageOptions: {
    ...config.languageOptions,
    parserOptions: {
      ...config.languageOptions.parserOptions,
      ecmaVersion: 2023,
    }
  }
});

Of course we could use some deep merge function, but that's essentially what createConfig does, but easier.

I can't help but wonder whether it will, in the long run, make our ESLint configurations unnecessarily difficult to understand and debug.

I don't think this will be any more of a problem with createConfig compared to map and spreading the config. Either way it's quite easy to log the object to see what's going on.

Copy link
Member

@rekmarks rekmarks Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find ESLint's recommendations—i.e. the map() syntax—to be completely unreadable, and much prefer the utility function. In fact, during the migration of our monorepo to ESLint 9, not using the createConfig() function was more difficult and bug-prone, as opposed to the reverse.

Copy link
Contributor

@mcmire mcmire Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the replies. That helps me understand the reasoning better.

I do wish it were more obvious that an object with extends actually represents multiple objects; that seems to diverge from the central ideas behind flat config. But, if it succeeds in helping us write the config files we want to write, then perhaps it doesn't matter.

Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import base, { createConfig } from '@metamask/eslint-config';
import jest from '@metamask/eslint-config-jest';
import nodejs from '@metamask/eslint-config-nodejs';
import typescript from '@metamask/eslint-config-typescript';

const config = createConfig([
{
ignores: ['!.eslintrc.js', '!.prettierrc.js', 'dist/', 'docs/', '.yarn/'],
Mrtenz marked this conversation as resolved.
Show resolved Hide resolved
},

{
extends: base,

languageOptions: {
sourceType: 'module',
parserOptions: {
tsconfigRootDir: import.meta.dirname,
project: ['./tsconfig.json'],
},
},

settings: {
'import-x/extensions': ['.js', '.mjs'],
},
},

{
files: ['**/*.ts'],
extends: typescript,
},

{
files: ['**/*.js', '**/*.cjs'],
extends: nodejs,

languageOptions: {
sourceType: 'script',
},
},

{
files: ['**/*.test.ts', '**/*.test.js'],
extends: [jest, nodejs],
},
]);

export default config;
38 changes: 19 additions & 19 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
"lint:changelog": "auto-changelog validate --prettier",
"lint:constraints": "yarn constraints",
"lint:dependencies": "depcheck && yarn dedupe",
"lint:eslint": "eslint . --cache --ext js,cjs,ts",
"lint:eslint": "eslint . --cache",
"lint:fix": "yarn lint:eslint --fix && yarn lint:constraints --fix && yarn lint:misc --write && yarn lint:dependencies && yarn lint:changelog",
"lint:misc": "prettier '**/*.json' '**/*.md' '**/*.yml' '!.yarnrc.yml' --ignore-path .gitignore --no-error-on-unmatched-pattern",
"prepack": "./scripts/prepack.sh",
Expand All @@ -48,34 +48,34 @@
"@arethetypeswrong/cli": "^0.15.3",
"@lavamoat/allow-scripts": "^3.0.4",
"@lavamoat/preinstall-always-fail": "^2.0.0",
"@metamask/auto-changelog": "^3.4.3",
"@metamask/eslint-config": "^12.2.0",
"@metamask/eslint-config-jest": "^12.1.0",
"@metamask/eslint-config-nodejs": "^12.1.0",
"@metamask/eslint-config-typescript": "^12.1.0",
"@metamask/auto-changelog": "^4.0.0",
"@metamask/eslint-config": "^14.0.0",
"@metamask/eslint-config-jest": "^14.0.0",
"@metamask/eslint-config-nodejs": "^14.0.0",
"@metamask/eslint-config-typescript": "^14.0.0",
"@ts-bridge/cli": "^0.6.0",
"@types/jest": "^28.1.6",
"@types/node": "^18.18",
"@typescript-eslint/eslint-plugin": "^5.43.0",
"@typescript-eslint/parser": "^6.21.0",
"@yarnpkg/types": "^4.0.0-rc.52",
"depcheck": "^1.4.3",
"eslint": "^8.44.0",
"eslint-config-prettier": "^8.8.0",
"eslint-plugin-import": "~2.26.0",
"eslint-plugin-jest": "^27.2.2",
"eslint-plugin-jsdoc": "^39.9.1",
"eslint-plugin-n": "^15.7.0",
"eslint-plugin-prettier": "^4.2.1",
"eslint-plugin-promise": "^6.1.1",
"eslint": "^9.11.0",
"eslint-config-prettier": "^9.1.0",
"eslint-import-resolver-typescript": "^3.6.3",
"eslint-plugin-import-x": "^4.3.0",
"eslint-plugin-jest": "^28.8.3",
"eslint-plugin-jsdoc": "^50.2.4",
"eslint-plugin-n": "^17.10.3",
"eslint-plugin-prettier": "^5.2.1",
"eslint-plugin-promise": "^7.1.0",
"jest": "^28.1.3",
"jest-it-up": "^2.0.2",
"prettier": "^2.7.1",
"prettier": "^3.3.3",
"prettier-plugin-packagejson": "^2.3.0",
"ts-jest": "^28.0.7",
"ts-node": "^10.7.0",
"typedoc": "^0.23.15",
"typescript": "~5.4.5"
"typedoc": "^0.26.11",
"typescript": "~5.4.5",
"typescript-eslint": "^8.6.0"
},
"packageManager": "[email protected]",
"engines": {
Expand Down
Loading