Allowing non-zero exit code in some of the commands to support pre-commit autofixing

[kachkaev]

Description

This issue is essentially a reverse of #26 🙂

We use eslint and prettier as linting tools and our typical package.json looks like this:

{
  "scripts": {
    "fix": "yarn fix:eslint && yarn fix:prettier",
    "fix:eslint": "eslint --fix \"**/*\"",
    "fix:prettier": "prettier --write \"**/*\"",
    "lint": "yarn lint:eslint && yarn lint:prettier",
    "lint:eslint": "eslint \"**/*\"",
    "lint:prettier": "prettier --check \"**/*\""
  },
  "husky": {
    "hooks": {
      "pre-commit": "lint-staged"
    }
  },
  "lint-staged": {
    "**/*": [
      "eslint --fix",
      "prettier --write",
      "git add"
    ]
  },
  "devDependencies": {
    "eslint": "^5.16.0",
    "eslint-config-prettier": "^4.3.0",
    "husky": "^2.3.0",
    "lint-staged": "^8.1.7",
    "prettier": "^1.17.1"
  }
}

• We want to involve lint-staged as a pre-commit hook.
• We want eslint and prettier to fix autofixable problems, but not block the commit if unfixable errors or warnings are found.
• The reason why we use **/* everywhere is because we specify matched extensions in .eslintignore / .prettierignore – helps keep things DRY.

This strategy does not fully work at the moment: every time eslint --fix finds a problem, it exits with a non-zero exit code and so the commit is blocked. This is a reasonable default behaviour, however, it is not quite what we are after.

As in many other workflows, we merge PRs to master only when yarn lint passes in the CI. This means that the final diff in a PR needs to be crystal clean, however any work-in-progress commits to a new branch can include imperfections. Typical examples of acceptable issues would be console.log not yet removed from the code (because of ongoing debugging) or present unused variables (because the implementation is incomplete). A precommit hook that autofixes formatting and, say, sorts imports, is useful, however a superstrict guard that blocks a commit is an obstacle to the process. We want to commit and push imperfect code to new branches as soon as possible to avoid loss of work, we just want to stay as close to perfect as automation allows us.

What we can do now is to implement a wrapper around eslint --fix (e.g. wrapped-eslint-fix) that would always exit with zero. It would be used like this:

  "lint-staged": {
    "**/*": [
      "wrapped-eslint-fix",
      "prettier --write",
      "git add"
    ]
  },

However, to make it easier to re-use the same trick in multiple projects, it would be nice to be able to do something like this:

  "lint-staged": {
    "**/*": [
      // option 1
      ["eslint --fix", { ignoreExitCode: true }],
      // option 2
      { command: "eslint --fix", ignoreExitCode: true },
      // option 3
      { command: "eslint --fix", options: { ignoreExitCode: true }},
      // remaining commands can keep the current syntax, but may also switch to the new one
      "prettier --write",
      "git add"
    ]
  },

New syntax is similar to what we see in eslint and webpack configs:

"eslintRule1": "error",
"eslintRule2": [ "error", { someOption: true } ],

"use": [
  "some-webpack-loader",
  { loader: "some-other-loader", options: { foo: "bar" } },
]

The potential new feature seems backwards-compatible and may support the development process in teams like ours. WDYT?

[okonet]

I have similar thoughts on general workflow although I would not modify lint-staged. Instead I think it’s better to make 2 eslint confugs: one for local dev and one for CI. The reason is simple: not only it’s counter productive to enforce too strict config a during the pre-commit but also during the work in the IDE. Having more loose config locally could solve it. I proposed this to ESLint team but the declined proposal. I tried to work on this a while ago but due to some buggy behavior and inconsistencies in eslint APIs this wasn’t possible to do. Perhaps you want to give it another try?

[kachkaev]

I’m not sure that moving this feature to ESlint will be easier for us than having to write wrapped-eslint-fix (eslint --fix that always exits with zero). Two ESLint configs are quite hard to maintain and debate about.

To me making a few commits to a branch that I will later turn into a single squashed PR commit is essentially the same as pressing ‘save’ in my code editor while I’m still in the middle of something. The only differences are that commits are less frequent than cmd+s and span across multiple files. When I save a file in VSCode that has Prettier and ESLint extensions, some spaces and JS tokens shuffle around, but the save action is never blocked when something is wrong with the syntax. In our workflow, an array of lint-staged commands called by husky is essentially the same as the extensions called by VSCode. lint-staged ‘extensions’ are just editor-agonstic and are executed before a slightly different kind of ‘save’.

[okonet]

I would argue with that but I’m okay with accepting there are different workflow and I don’t want the tool to be too opionated in this regard. At the same time I resist adding new config flags to the config as much as possible since it causes confusion and additional maintenance costs. To me, the solution with a custom wrapper that always exits with 0 sounds like most acceptable unless I see the community asks for this kind of behavior on a constant basis. Hope you understand my arguments.

[kachkaev]

I totally understand your arguments and also value tools that are dead-simple on the surface. I've opened this issue to share our situation and to discover if anyone else faces a similar problem.

This will probably work for us:

 "lint-staged": {
   "**/*": [
-    "eslint --fix",
-    "prettier --write",
+    "suppress-exit-code eslint --fix",
+    "suppress-exit-code prettier --write",
     "git add"
   ]
},

Just published suppress-exit-code on npm.

@okonet feel free to close the issue if you want and thank you for taking part in this conversation. If anyone else seeks for suppressing exit codes, please give a shout below – this will help others know if extending lint-staged is worth it.

[lkraav]

@kachkaev maybe strategies found in #640 can retire suppress-exit-code now?

[maslade]

@lkraav I tried that approach and was having trouble. With lint-staged@11.0.0 I tried the following:

  '*.{ts,js}': (filenames) => {
    const cwd = process.cwd();
    const relativeFilenames = filenames
      .map((file) => relative(cwd, file))
      .join(' ');

    return [
      `./node_modules/.bin/eslint --fix ${relativeFilenames}; if [ $? -eq 1 ]; then exit 0; fi`,
    ];
  },

This is resulting in:

✖ ./node_modules/.bin/eslint --fix .lintstagedrc.js; if [ $? -eq 1 ]; then exit 0; fi:
Invalid option '-e' - perhaps you meant '-c'?

I simplified the command a bit and still get this:

✖ ./node_modules/.bin/eslint --fix .lintstagedrc.js; exit 0;:

Oops! Something went wrong! :(

[maslade]

I'm using pre-commit hooks to use linters as a gate. My team wanted those warnings/errors reported, but to still allow the commit through. I was struggling with Ikraav's suggestion and getting suppress-exit-code to work the way I wanted it to.

I found this solution to work well and simply:

pre-commit:

#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

if ! npx lint-staged ; then
  exit 0
fi

With my hook written this way, devs can always commit code, but they're also shown any linting warnings or errors that couldn't be fixed.

[tdkn]

I use this workaround:

// lint-staged.config.js

const silent = shell => `sh -c '${shell} || exit 0'`

module.exports = {
  "**/*": (files) => {
    return `prettier --write --ignore-unknown ${files.join(" ")}`;
  },
  "*.{js,jsx,ts,tsx}": (files) => {
    return silent(`eslint --fix ${files.join(" ")}`);
  },
  "*.{css,scss}": (files) => {
    return silent(`stylelint --fix ${files.join(" ")}`);
  },
};

The important points are:

• sh -c: avoids errors such as Invalid option '-e'
• || exit 0: Forces the exit code to 0

[kachkaev]

@maslade does your solution run all lint-staged hooks or stops on error? Say, if I have eslint --fix &prettier --write and eslint --fix exits with 1, is prettier reached or not?