Repository: okonet/lint-staged (js)

Issue:
Being able to bail on error.
### Description

Hi there, first of, awesome package!

This is a feature request.

I was wondering if there was a way to bail on error?
Currently, I have multiple linters set up, and one of them is quite long to run.
Sometimes, other short ones fail before this long one, but we have to wait after every linters to finish their job before exiting the process.

I'm using `lint-staged` in a pre-commit hook with `husky`. 
It's very useful this way.

Maybe I'm missing a configuration that would let me bail on error, if so I'd love more info on it.

Thank you!

### Environment

<!-- Tell us about your development environment -->
- **OS:macOS Mojave** <!-- ex: Windows 10, Ubuntu 16.04, macOS High Sierra -->
- **Node.js:11.6.0** <!-- ex: v8.9.1 -->
- **`lint-staged`:8.1.0** <!-- ex: v6.0.0 -->


Pull request description:
fix: kill other running tasks on failure
This kills already running tasks as soon as an error is emitted by another task. Making it possible to interrupt concurrently running tasks on first failure.

Aside from the included unmocked test, you can also try it out by staging a linting error and changing the lint-staged configuration in this repo to `.lintstagedrc.mjs` with the following content:
```
export default {
  "*.js": "eslint",
  "*.{js,ts}": () => "npm run test",
}
```
... and running:
`node ./bin/lint-staged.js`

I have changed the precedence of the tags, since I think it would be confusing to users to see the signals when we consciously kill tasks. (With the previous order, they would have seen `SIGTERM`. This way, it is `KILLED`.)

There are two minor caveats:
- It doesn't work with `shell: true`.
- For this to work with npm scripts (e.g. vanilla tsc, jest and probably anything that deals properly with its children is just fine), [we need to kill all children ourselves](https://github.com/npm/npm/issues/4603). Which [execa currently doesn't do](https://github.com/sindresorhus/execa/issues/96) for [a good reason](https://github.com/nodejs/node/issues/40438#issuecomment-943591510). However, I would argue that while this makes sense for an all purpose tool like execa, given lint-staged's use case, I can't imagine danger from killing child processes within milliseconds.

Fixes #594

Patch:
diff --git a/lib/resolveTaskFn.js b/lib/resolveTaskFn.js
index f39e601..77b093b 100644
--- a/lib/resolveTaskFn.js
+++ b/lib/resolveTaskFn.js
@@ -2,14 +2,17 @@ import { redBright, dim } from 'colorette'
 import execa from 'execa'
 import debug from 'debug'
 import { parseArgsStringToArgv } from 'string-argv'
+import pidTree from 'pidtree'
 
 import { error, info } from './figures.js'
 import { getInitialState } from './state.js'
 import { TaskError } from './symbols.js'
 
+const ERROR_CHECK_INTERVAL = 200
+
 const debugLog = debug('lint-staged:resolveTaskFn')
 
-const getTag = ({ code, killed, signal }) => signal || (killed && 'KILLED') || code || 'FAILED'
+const getTag = ({ code, killed, signal }) => (killed && 'KILLED') || signal || code || 'FAILED'
 
 /**
  * Handle task console output.
@@ -43,6 +46,34 @@ const handleOutput = (command, result, ctx, isError = false) => {
   }
 }
 
+/**
+ * Interrupts the execution of the execa process that we spawned if
+ * another task adds an error to the context.
+ *
+ * @param {Object} ctx
+ * @param {execa.ExecaChildProcess<string>} execaChildProcess
+ * @returns {function(): void} Function that clears the interval that
+ * checks the context.
+ */
+const interruptExecutionOnError = (ctx, execaChildProcess) => {
+  async function loop() {
+    if (ctx.errors.size > 0) {
+      const ids = await pidTree(execaChildProcess.pid)
+      ids.forEach(process.kill)
+
+      // The execa process is killed separately in order
+      // to get the `KILLED` status.
+      execaChildProcess.kill()
+    }
+  }
+
+  const loopIntervalId = setInterval(loop, ERROR_CHECK_INTERVAL)
+
+  return () => {
+    clearInterval(loopIntervalId)
+  }
+}
+
 /**
  * Create a error output dependding on process result.
  *
@@ -101,9 +132,13 @@ export const resolveTaskFn = ({
   debugLog('execaOptions:', execaOptions)
 
   return async (ctx = getInitialState()) => {
-    const result = await (shell
+    const execaChildProcess = shell
       ? execa.command(isFn ? command : `${command} ${files.join(' ')}`, execaOptions)
-      : execa(cmd, isFn ? args : args.concat(files), execaOptions))
+      : execa(cmd, isFn ? args : args.concat(files), execaOptions)
+
+    const quitInterruptCheck = interruptExecutionOnError(ctx, execaChildProcess)
+    const result = await execaChildProcess
+    quitInterruptCheck()
 
     if (result.failed || result.killed || result.signal != null) {
       throw makeErr(command, result, ctx)
diff --git a/package-lock.json b/package-lock.json
index 0ffda7f..93375dc 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -19,6 +19,7 @@
         "micromatch": "^4.0.4",
         "normalize-path": "^3.0.0",
         "object-inspect": "^1.12.0",
+        "pidtree": "^0.5.0",
         "string-argv": "^0.3.1",
         "supports-color": "^9.2.1",
         "yaml": "^1.10.2"
@@ -7494,6 +7495,17 @@
         "url": "https://github.com/sponsors/jonschlinkert"
       }
     },
+    "node_modules/pidtree": {
+      "version": "0.5.0",
+      "resolved": "https://registry.npmjs.org/pidtree/-/pidtree-0.5.0.tgz",
+      "integrity": "sha512-9nxspIM7OpZuhBxPg73Zvyq7j1QMPMPsGKTqRc2XOaFQauDvoNz9fM1Wdkjmeo7l9GXOZiRs97sPkuayl39wjA==",
+      "bin": {
+        "pidtree": "bin/pidtree.js"
+      },
+      "engines": {
+        "node": ">=0.10"
+      }
+    },
     "node_modules/pirates": {
       "version": "4.0.4",
       "resolved": "https://registry.npmjs.org/pirates/-/pirates-4.0.4.tgz",
@@ -14258,6 +14270,11 @@
       "resolved": "https://registry.npmjs.org/picomatch/-/picomatch-2.3.0.tgz",
       "integrity": "sha512-lY1Q/PiJGC2zOv/z391WOTD+Z02bCgsFfvxoXXf6h7kv9o+WmsmzYqrAwY63sNgOxE4xEdq0WyUnXfKeBrSvYw=="
     },
+    "pidtree": {
+      "version": "0.5.0",
+      "resolved": "https://registry.npmjs.org/pidtree/-/pidtree-0.5.0.tgz",
+      "integrity": "sha512-9nxspIM7OpZuhBxPg73Zvyq7j1QMPMPsGKTqRc2XOaFQauDvoNz9fM1Wdkjmeo7l9GXOZiRs97sPkuayl39wjA=="
+    },
     "pirates": {
       "version": "4.0.4",
       "resolved": "https://registry.npmjs.org/pirates/-/pirates-4.0.4.tgz",
diff --git a/package.json b/package.json
index dfbc359..0ff97d1 100644
--- a/package.json
+++ b/package.json
@@ -42,6 +42,7 @@
     "micromatch": "^4.0.4",
     "normalize-path": "^3.0.0",
     "object-inspect": "^1.12.0",
+    "pidtree": "^0.5.0",
     "string-argv": "^0.3.1",
     "supports-color": "^9.2.1",
     "yaml": "^1.10.2"


Test patch:
diff --git a/test/__mocks__/execa.js b/test/__mocks__/execa.js
index 4f204a3..5d0d450 100644
--- a/test/__mocks__/execa.js
+++ b/test/__mocks__/execa.js
@@ -1,5 +1,7 @@
+import { createExecaReturnValue } from '../utils/createExecaReturnValue'
+
 const execa = jest.fn(() =>
-  Promise.resolve({
+  createExecaReturnValue({
     stdout: 'a-ok',
     stderr: '',
     code: 0,
diff --git a/test/__mocks__/pidtree.js b/test/__mocks__/pidtree.js
new file mode 100644
index 0000000..26ca352
--- /dev/null
+++ b/test/__mocks__/pidtree.js
@@ -0,0 +1,3 @@
+module.exports = () => {
+  return Promise.resolve([])
+}
diff --git a/test/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js
index 1c4e70e..0f3cc29 100644
--- a/test/resolveTaskFn.spec.js
+++ b/test/resolveTaskFn.spec.js
@@ -4,8 +4,14 @@ import { resolveTaskFn } from '../lib/resolveTaskFn'
 import { getInitialState } from '../lib/state'
 import { TaskError } from '../lib/symbols'
 
+import { createExecaReturnValue } from './utils/createExecaReturnValue'
+
 const defaultOpts = { files: ['test.js'] }
 
+function mockExecaImplementationOnce(value) {
+  execa.mockImplementationOnce(() => createExecaReturnValue(value))
+}
+
 describe('resolveTaskFn', () => {
   beforeEach(() => {
     execa.mockClear()
@@ -135,7 +141,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for failed linters', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -149,7 +155,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for interrupted processes', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -167,7 +173,7 @@ describe('resolveTaskFn', () => {
 
   it('should throw error for killed processes without signal', async () => {
     expect.assertions(1)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -192,7 +198,7 @@ describe('resolveTaskFn', () => {
   })
 
   it('should add TaskError on error', async () => {
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock error',
       stderr: '',
       code: 0,
@@ -210,7 +216,7 @@ describe('resolveTaskFn', () => {
 
   it('should not add output when there is none', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: '',
       code: 0,
@@ -236,7 +242,7 @@ describe('resolveTaskFn', () => {
 
   it('should add output even when task succeeds if `verbose: true`', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: 'Mock success',
       stderr: '',
       code: 0,
@@ -266,7 +272,7 @@ describe('resolveTaskFn', () => {
 
   it('should not add title to output when task errors while quiet', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: 'stderr',
       code: 1,
@@ -296,7 +302,7 @@ describe('resolveTaskFn', () => {
 
   it('should not print anything when task errors without output while quiet', async () => {
     expect.assertions(2)
-    execa.mockResolvedValueOnce({
+    mockExecaImplementationOnce({
       stdout: '',
       stderr: '',
       code: 1,
@@ -321,4 +327,29 @@ describe('resolveTaskFn', () => {
       }
     `)
   })
+
+  it('should kill a long running task when an error is added to the context', async () => {
+    execa.mockImplementationOnce(() =>
+      createExecaReturnValue(
+        {
+          stdout: 'a-ok',
+          stderr: '',
+          code: 0,
+          cmd: 'mock cmd',
+          failed: false,
+          killed: false,
+          signal: null,
+        },
+        1000
+      )
+    )
+
+    const context = getInitialState()
+    const taskFn = resolveTaskFn({ command: 'node' })
+    const taskPromise = taskFn(context)
+
+    context.errors.add({})
+
+    await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)
+  })
 })
diff --git a/test/resolveTaskFn.unmocked.spec.js b/test/resolveTaskFn.unmocked.spec.js
index 2727afc..ecef88e 100644
--- a/test/resolveTaskFn.unmocked.spec.js
+++ b/test/resolveTaskFn.unmocked.spec.js
@@ -1,4 +1,5 @@
 import { resolveTaskFn } from '../lib/resolveTaskFn'
+import { getInitialState } from '../lib/state'
 
 jest.unmock('execa')
 
@@ -13,4 +14,25 @@ describe('resolveTaskFn', () => {
 
     await expect(taskFn()).resolves.toMatchInlineSnapshot(`undefined`)
   })
+
+  it('should kill a long running task when another fails', async () => {
+    const context = getInitialState()
+
+    const taskFn = resolveTaskFn({
+      command: 'node',
+      isFn: true,
+    })
+    const taskPromise = taskFn(context)
+
+    const taskFn2 = resolveTaskFn({
+      command: 'node -e "process.exit(1)"',
+      isFn: true,
+    })
+    const task2Promise = taskFn2(context)
+
+    await expect(task2Promise).rejects.toThrowErrorMatchingInlineSnapshot(
+      `"node -e \\"process.exit(1)\\" [FAILED]"`
+    )
+    await expect(taskPromise).rejects.toThrowErrorMatchingInlineSnapshot(`"node [KILLED]"`)
+  })
 })
diff --git a/test/runAll.spec.js b/test/runAll.spec.js
index 84fa9c4..6865939 100644
--- a/test/runAll.spec.js
+++ b/test/runAll.spec.js
@@ -12,6 +12,8 @@ import { ConfigNotFoundError, GitError } from '../lib/symbols'
 import * as searchConfigsNS from '../lib/searchConfigs'
 import * as getConfigGroupsNS from '../lib/getConfigGroups'
 
+import { createExecaReturnValue } from './utils/createExecaReturnValue'
+
 jest.mock('../lib/file')
 jest.mock('../lib/getStagedFiles')
 jest.mock('../lib/gitWorkflow')
@@ -192,7 +194,7 @@ describe('runAll', () => {
     expect.assertions(2)
     getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
     execa.mockImplementation(() =>
-      Promise.resolve({
+      createExecaReturnValue({
         stdout: '',
         stderr: 'Linter finished with error',
         code: 1,
@@ -229,7 +231,7 @@ describe('runAll', () => {
     expect.assertions(2)
     getStagedFiles.mockImplementationOnce(async () => ['sample.js'])
     execa.mockImplementation(() =>
-      Promise.resolve({
+      createExecaReturnValue({
         stdout: '',
         stderr: '',
         code: 0,
@@ -252,8 +254,8 @@ describe('runAll', () => {
       LOG [STARTED]  — 1 file
       LOG [STARTED] *.js — 1 file
       LOG [STARTED] echo \\"sample\\"
-      ERROR [FAILED] echo \\"sample\\" [SIGINT]
-      ERROR [FAILED] echo \\"sample\\" [SIGINT]
+      ERROR [FAILED] echo \\"sample\\" [KILLED]
+      ERROR [FAILED] echo \\"sample\\" [KILLED]
       LOG [SUCCESS] Running tasks for staged files...
       LOG [STARTED] Applying modifications from tasks...
       INFO [SKIPPED] Skipped because of errors from tasks.
diff --git a/test/utils/createExecaReturnValue.js b/test/utils/createExecaReturnValue.js
new file mode 100644
index 0000000..7660082
--- /dev/null
+++ b/test/utils/createExecaReturnValue.js
@@ -0,0 +1,20 @@
+export function createExecaReturnValue(value, executionTime) {
+  const returnValue = { ...value }
+  let triggerResolve
+  let resolveTimeout
+
+  const returnedPromise = executionTime
+    ? new Promise((resolve) => {
+        triggerResolve = resolve.bind(null, returnValue)
+        resolveTimeout = setTimeout(triggerResolve, executionTime)
+      })
+    : Promise.resolve(returnValue)
+
+  returnedPromise.kill = () => {
+    returnValue.killed = true
+    clearTimeout(resolveTimeout)
+    triggerResolve()
+  }
+
+  return returnedPromise
+}