Repository: okonet/lint-staged
Pull request #650 (2019-07-03 04:08:53)

fix: run all commands returned by function linters
Function linters have to be evaluated at makeCmdTasks, and resolveTaskFn has to run for each of the returned commands.

Currently, `makeCmdTasks` creates only a single task per function linter, and `resolveTaskFn` returns an array of promises. This PR fixes the logic by evaluating functions at `makeCmdTasks` level, and running `resolveTaskFn` for each of the commands returned, simplifying `resolveTaskFn`.

This PR fixes issues where a function linter returning an array of commands might only run the first of the commands.

- [ ] https://github.com/okonet/lint-staged/issues/647
- [ ] https://github.com/okonet/lint-staged/issues/649

codecov[bot]: # [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=h1) Report
> Merging [#650](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=desc) into [master](https://codecov.io/gh/okonet/lint-staged/commit/146e6ce88987098c3478455dd8ad7f8bb1e83807?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/okonet/lint-staged/pull/650/graphs/tree.svg?width=650&token=LSoEgsjk4O&height=150&src=pr)](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
- Coverage   98.69%   98.67%   -0.02%     
==========================================
  Files          11       11              
  Lines         306      303       -3     
  Branches       59       57       -2     
==========================================
- Hits          302      299       -3     
  Misses          4        4
```


| [Impacted Files](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/makeCmdTasks.js](https://codecov.io/gh/okonet/lint-staged/pull/650/diff?src=pr&el=tree#diff-c3JjL21ha2VDbWRUYXNrcy5qcw==) | `100% <100%> (ø)` | :arrow_up: |
| [src/resolveTaskFn.js](https://codecov.io/gh/okonet/lint-staged/pull/650/diff?src=pr&el=tree#diff-c3JjL3Jlc29sdmVUYXNrRm4uanM=) | `100% <100%> (ø)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=footer). Last update [146e6ce...82bc169](https://codecov.io/gh/okonet/lint-staged/pull/650?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 2b3eaa6..198ada6 100644
--- a/README.md
+++ b/README.md
@@ -194,14 +194,14 @@ If for some reason you want to ignore files from the glob match, you can use `mi
 const micromatch = require('micromatch')
 module.exports = {
   '*.js': files => {
-    // from `files` filter those _NOT_ matching `*.test.js`
-    const match = micromatch.not(files, '*.test.js')
+    // from `files` filter those _NOT_ matching `*test.js`
+    const match = micromatch.not(files, '*test.js')
     return match.map(file => `eslint ${file}`)
   }
 }
 ```
 
-Please note that for most cases, globs can achieve the same effect. For the above example, a matching glob would be `!(*.test).js`.
+Please note that for most cases, globs can achieve the same effect. For the above example, a matching glob would be `!(*test).js`.
 
 ### Example: Use relative paths for commands
 
diff --git a/src/makeCmdTasks.js b/src/makeCmdTasks.js
index 5598baf..8ac8d18 100644
--- a/src/makeCmdTasks.js
+++ b/src/makeCmdTasks.js
@@ -4,19 +4,6 @@ const resolveTaskFn = require('./resolveTaskFn')
 
 const debug = require('debug')('lint-staged:make-cmd-tasks')
 
-/**
- * Get title for linter task. For a function, evaluate by passing single [file].
- * For strings, return as-is
- * @param {string|Function} linter
- */
-const getTitle = linter => {
-  if (typeof linter === 'function') {
-    const resolved = linter(['[file]'])
-    return Array.isArray(resolved) ? resolved[0] : resolved
-  }
-  return linter
-}
-
 /**
  * Creates and returns an array of listr tasks which map to the given commands.
  *
@@ -27,16 +14,23 @@ const getTitle = linter => {
  */
 module.exports = async function makeCmdTasks(commands, shell, gitDir, pathsToLint) {
   debug('Creating listr tasks for commands %o', commands)
+  const commandsArray = Array.isArray(commands) ? commands : [commands]
+
+  return commandsArray.reduce((tasks, command) => {
+    // linter function may return array of commands that already include `pathsToLit`
+    const isFn = typeof command === 'function'
+    const resolved = isFn ? command(pathsToLint) : command
+    const linters = Array.isArray(resolved) ? resolved : [resolved] // Wrap non-array linter as array
 
-  const lintersArray = Array.isArray(commands) ? commands : [commands]
+    linters.forEach(linter => {
+      const task = {
+        title: linter,
+        task: resolveTaskFn({ gitDir, isFn, linter, pathsToLint, shell })
+      }
 
-  return lintersArray.map(linter => ({
-    title: getTitle(linter),
-    task: resolveTaskFn({
-      linter,
-      shell,
-      gitDir,
-      pathsToLint
+      tasks.push(task)
     })
-  }))
+
+    return tasks
+  }, [])
 }
diff --git a/src/resolveTaskFn.js b/src/resolveTaskFn.js
index 6be3bd7..b6b52d9 100644
--- a/src/resolveTaskFn.js
+++ b/src/resolveTaskFn.js
@@ -13,9 +13,11 @@ const debug = require('debug')('lint-staged:task')
  * return the promise.
  *
  * @param {string} cmd
+ * @param {Array<string>} args
+ * @param {Object} execaOptions
  * @return {Promise} child_process
  */
-const execLinter = (cmd, args, execaOptions = {}) => {
+const execLinter = (cmd, args, execaOptions) => {
   debug('cmd:', cmd)
   if (args) debug('args:', args)
   debug('execaOptions:', execaOptions)
@@ -75,52 +77,42 @@ function makeErr(linter, result, context = {}) {
  * if the OS is Windows.
  *
  * @param {Object} options
+ * @param {string} options.gitDir
+ * @param {Boolean} options.isFn
  * @param {string} options.linter
  * @param {Boolean} options.shellMode
- * @param {string} options.gitDir
  * @param {Array<string>} options.pathsToLint
  * @returns {function(): Promise<Array<string>>}
  */
-module.exports = function resolveTaskFn({ gitDir, linter, pathsToLint, shell = false }) {
-  // If `linter` is a function, it should return a string when evaluated with `pathsToLint`.
-  // Else, it's a already a string
-  const fnLinter = typeof linter === 'function'
-  const linterString = fnLinter ? linter(pathsToLint) : linter
-  // Support arrays of strings/functions by treating everything as arrays
-  const linters = Array.isArray(linterString) ? linterString : [linterString]
-
+module.exports = function resolveTaskFn({ gitDir, isFn, linter, pathsToLint, shell = false }) {
   const execaOptions = { preferLocal: true, reject: false, shell }
 
-  const tasks = linters.map(command => {
-    let cmd
-    let args
+  let cmd
+  let args
 
-    if (shell) {
-      execaOptions.shell = true
-      // If `shell`, passed command shouldn't be parsed
-      // If `linter` is a function, command already includes `pathsToLint`.
-      cmd = fnLinter ? command : `${command} ${pathsToLint.join(' ')}`
-    } else {
-      const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(command)
-      cmd = parsedCmd
-      args = fnLinter ? parsedArgs : parsedArgs.concat(pathsToLint)
-    }
-
-    // Only use gitDir as CWD if we are using the git binary
-    // e.g `npm` should run tasks in the actual CWD
-    if (/^git(\.exe)?/i.test(command) && gitDir !== process.cwd()) {
-      execaOptions.cwd = gitDir
-    }
+  if (shell) {
+    execaOptions.shell = true
+    // If `shell`, passed command shouldn't be parsed
+    // If `linter` is a function, command already includes `pathsToLint`.
+    cmd = isFn ? linter : `${linter} ${pathsToLint.join(' ')}`
+  } else {
+    const [parsedCmd, ...parsedArgs] = stringArgv.parseArgsStringToArgv(linter)
+    cmd = parsedCmd
+    args = isFn ? parsedArgs : parsedArgs.concat(pathsToLint)
+  }
 
-    return ctx =>
-      execLinter(cmd, args, execaOptions).then(result => {
-        if (result.failed || result.killed || result.signal != null) {
-          throw makeErr(linter, result, ctx)
-        }
+  // Only use gitDir as CWD if we are using the git binary
+  // e.g `npm` should run tasks in the actual CWD
+  if (/^git(\.exe)?/i.test(linter) && gitDir !== process.cwd()) {
+    execaOptions.cwd = gitDir
+  }
 
-        return successMsg(linter)
-      })
-  })
+  return ctx =>
+    execLinter(cmd, args, execaOptions).then(result => {
+      if (result.failed || result.killed || result.signal != null) {
+        throw makeErr(linter, result, ctx)
+      }
 
-  return ctx => Promise.all(tasks.map(task => task(ctx)))
+      return successMsg(linter)
+    })
 }


Test patch:
diff --git a/test/makeCmdTasks.spec.js b/test/makeCmdTasks.spec.js
index ee6cfc2..037df25 100644
--- a/test/makeCmdTasks.spec.js
+++ b/test/makeCmdTasks.spec.js
@@ -61,8 +61,9 @@ describe('makeCmdTasks', () => {
 
   it('should work with function linter returning array of string', async () => {
     const res = await makeCmdTasks(() => ['test', 'test2'], false, gitDir, ['test.js'])
-    expect(res.length).toBe(1)
+    expect(res.length).toBe(2)
     expect(res[0].title).toEqual('test')
+    expect(res[1].title).toEqual('test2')
   })
 
   it('should work with function linter accepting arguments', async () => {
@@ -70,16 +71,25 @@ describe('makeCmdTasks', () => {
       filenames => filenames.map(file => `test ${file}`),
       false,
       gitDir,
-      ['test.js']
+      ['test.js', 'test2.js']
     )
-    expect(res.length).toBe(1)
-    expect(res[0].title).toEqual('test [file]')
+    expect(res.length).toBe(2)
+    expect(res[0].title).toEqual('test test.js')
+    expect(res[1].title).toEqual('test test2.js')
   })
 
   it('should work with array of mixed string and function linters', async () => {
-    const res = await makeCmdTasks([() => 'test', 'test2'], false, gitDir, ['test.js'])
-    expect(res.length).toBe(2)
+    const res = await makeCmdTasks(
+      [() => 'test', 'test2', files => files.map(file => `test ${file}`)],
+      false,
+      gitDir,
+      ['test.js', 'test2.js', 'test3.js']
+    )
+    expect(res.length).toBe(5)
     expect(res[0].title).toEqual('test')
     expect(res[1].title).toEqual('test2')
+    expect(res[2].title).toEqual('test test.js')
+    expect(res[3].title).toEqual('test test2.js')
+    expect(res[4].title).toEqual('test test3.js')
   })
 })
diff --git a/test/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js
index 1a1a034..783d83a 100644
--- a/test/resolveTaskFn.spec.js
+++ b/test/resolveTaskFn.spec.js
@@ -24,11 +24,12 @@ describe('resolveTaskFn', () => {
     })
   })
 
-  it('should support function linters that return string', async () => {
+  it('should not append pathsToLint when isFn', async () => {
     expect.assertions(2)
     const taskFn = resolveTaskFn({
       ...defaultOpts,
-      linter: filenames => `node --arg=true ./myscript.js ${filenames}`
+      isFn: true,
+      linter: 'node --arg=true ./myscript.js test.js'
     })
 
     await taskFn()
@@ -40,25 +41,38 @@ describe('resolveTaskFn', () => {
     })
   })
 
-  it('should support function linters that return array of strings', async () => {
-    expect.assertions(3)
+  it('should not append pathsToLint when isFn and shell', async () => {
+    expect.assertions(2)
     const taskFn = resolveTaskFn({
       ...defaultOpts,
-      pathsToLint: ['foo.js', 'bar.js'],
-      linter: filenames => filenames.map(filename => `node --arg=true ./myscript.js ${filename}`)
+      isFn: true,
+      shell: true,
+      linter: 'node --arg=true ./myscript.js test.js'
     })
 
     await taskFn()
-    expect(execa).toHaveBeenCalledTimes(2)
-    expect(execa).nthCalledWith(1, 'node', ['--arg=true', './myscript.js', 'foo.js'], {
+    expect(execa).toHaveBeenCalledTimes(1)
+    expect(execa).lastCalledWith('node --arg=true ./myscript.js test.js', {
       preferLocal: true,
       reject: false,
-      shell: false
+      shell: true
+    })
+  })
+
+  it('should work with shell', async () => {
+    expect.assertions(2)
+    const taskFn = resolveTaskFn({
+      ...defaultOpts,
+      shell: true,
+      linter: 'node --arg=true ./myscript.js'
     })
-    expect(execa).nthCalledWith(2, 'node', ['--arg=true', './myscript.js', 'bar.js'], {
+
+    await taskFn()
+    expect(execa).toHaveBeenCalledTimes(1)
+    expect(execa).lastCalledWith('node --arg=true ./myscript.js test.js', {
       preferLocal: true,
       reject: false,
-      shell: false
+      shell: true
     })
   })
 
diff --git a/test/resolveTaskFn.unmocked.spec.js b/test/resolveTaskFn.unmocked.spec.js
index 01b7c3d..feab1bb 100644
--- a/test/resolveTaskFn.unmocked.spec.js
+++ b/test/resolveTaskFn.unmocked.spec.js
@@ -6,16 +6,13 @@ describe('resolveTaskFn', () => {
   it('should call execa with shell when configured so', async () => {
     const taskFn = resolveTaskFn({
       pathsToLint: ['package.json'],
-      linter: () => 'node -e "process.exit(1)" || echo $?',
+      isFn: true,
+      linter: 'node -e "process.exit(1)" || echo $?',
       shell: true
     })
 
-    await expect(taskFn()).resolves.toMatchInlineSnapshot(`
-Array [
-  "√ function linter() {
-        return 'node -e \\"process.exit(1)\\" || echo $?';
-      } passed!",
-]
-`)
+    await expect(taskFn()).resolves.toMatchInlineSnapshot(
+      `"√ node -e \\"process.exit(1)\\" || echo $? passed!"`
+    )
   })
 })