MetaMask / MetaMask/core

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

Open
#1,333 2 comments 0 reactions 1 assignee Claimed by @mcmire View on GitHub
team-wallet-framework wf-ci-tooling
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

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:

* Full run (check or fix):
* Run ESLint on all `*.js` and `*.ts` files, omitting those in `.gitignore`
* Run Prettier on all `*.json`, `*.md`, and `*.yml` files, omitting those in `.gitignore`, as well as `CHANGELOG.md`s in any packages, `CHANGELOG.old.md`, and `.yarnrc.yml`
* Partial run (changed files only):
* Run ESLint on all _changed_ `*.js` and `*.ts` files, omitting those in `.gitignore`
* Run Prettier on all _changed_ `*.json`, `*.md`, and `*.yml` files, omitting those in `.gitignore`, as well as `CHANGELOG.md`s in any packages, `CHANGELOG.old.md`, and `.yarnrc.yml`

More details below:

---

The Snaps monorepo attempts to reduce the runtime by using [`lint-staged`](https://github.com/okonet/lint-staged) to [only linting files that have been staged](https://github.com/MetaMask/snaps-monorepo/blob/b9c6aec01954e8dfd5bd094e9776b87ca59fabc9/package.json#L37). 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`](https://github.com/artsy/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](https://github.com/MetaMask/core/blob/65af9a440ff957dafdc681cc6ef2929831f62365/package.json#L25).

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:

``` json
{
"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:

``` json
{
"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:

``` json
{
"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:

* figure out which file extensions go with which command, and specify them in the way that the tool understands
* accept a set of file patterns, or else default to `.` if no file patterns were specified, and pass the result off to the tool

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

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.