MetaMask / core

This monorepo is a collection of packages used across multiple MetaMask clients
MIT License
252 stars 173 forks source link

Update lint hook to only lint files that have been changed for the current branch #1333

Open mcmire opened 1 year ago

mcmire commented 1 year ago

The lint command takes around 10 seconds to run. This is fine when it's run on CI, but it feels a bit uncomfortable when run as a part of a push.

Ideally, what we want is something like this:

More details below:


The Snaps monorepo attempts to reduce the runtime by using lint-staged to only linting files that have been staged. There are two wrinkles with the approach that the Snaps monorepo takes that would prevent us from copying it to this repo, however.

First, lint-staged is designed to be paired with a pre-commit hook, but we use a pre-push hook instead, so we'd need a different solution. So minimally, we could use something like lint-changed instead.

However, lint-changed (and lint-staged, for that matter) assume that you don't want to customize the set of files that are processed when running a full lint. For ESLint, that's not a problem, as we can get away with asking it to lint the whole project and then use .gitignore to filter out undesired files. Prettier, however, doesn't use .gitignore by default, so we tell it to use .gitignore, but then we also filter out more files on top of that.

So if we were to start using lint-changed, we'd have to have a separate way of running Prettier. We'd need a way to run file paths that lint-changed has reported as changed, but continue to filter out undesired files that aren't covered by .gitignore.

The Prettier team has made this somewhat easier by allowing multiple ignore file paths to be passed: https://github.com/prettier/prettier/pull/14332. So we could pull out the list of files that we're excluding to a .prettierignore and then instruct the prettier command to use that in addition to .gitignore.

However, one problem would remain: we'd have to accommodate two ways of calling — and configuring — our lint commands.

Say we were able to reduce package.json to something like this:

{
  "scripts": {
    "lint": "yarn lint:js && yarn lint:misc --check && yarn constraints",
    "lint:fix": "yarn lint:js --fix && yarn lint:misc --write && yarn constraints --fix",
    "lint:js": "eslint . --cache --ext js,ts",
    "lint:misc": "prettier '**/*.json' '**/*.md' '**/*.yml' --ignore-path .gitignore --ignore-path .prettierignore"
  }
}

Now say we were to add lint-changed. That might look something like this:

{
  "scripts": {
    "lint": "yarn lint:js --check && yarn lint:misc --check && yarn constraints",
    "lint:fix": "yarn lint:js --fix && yarn lint:misc --write && yarn constraints --fix",
    "lint:js": "eslint --cache --ext js,ts .",
    "lint:misc": "prettier --ignore-path .gitignore --ignore-path .prettierignore '**/*.json' '**/*.md' '**/*.yml'"
  },
  "lint-staged": {
    "*.{js,ts}": "eslint --cache --ext js,ts",
    "*.{json,md,yml}": "prettier --ignore-path .gitignore --ignore-path .prettierignore"
  }
}

(We don't specify any file paths for lint-staged because it fills them in for us.)

Note how we have to specify two ways of running the eslint and prettier commands, and we have to specify acceptable file extensions twice. It'd be nice if we could just say:

{
  "scripts": {
    "lint": "scripts/lint.sh --check",
    "lint:fix": "scripts/lint.sh --fix"
  },
  "lint-staged": {
    "*": "scripts/lint.sh --check"
  }
}

To get this to work, such a lint script would have to:

So this ticket depends on the release of Prettier 3.0, but it also depends on writing such a lint script.

mcmire commented 1 year ago

I've started a branch to work on this: https://github.com/MetaMask/core/tree/use-lint-changed