feat: Chunked execution of linters on Windows only (#439)

* feat: Chunked execution of linters on Windows only

* test: Add tests for resolveTaskFn
  Move some tests from makeCmdTasks to resolveTaskFn.

* chore: Add babel-plugin-transform-object-rest-spread

.babelrc

@@ -5,5 +5,6 @@
         "node": 6
       }
     }]
-  ]
+  ],
+  "plugins": ["transform-object-rest-spread"]
 }

README.md

@@ -130,12 +130,12 @@ To set options and keep lint-staged extensible, advanced format can be used. Thi
 ## Options
 
 * `concurrent` — _true_ — runs linters for each glob pattern simultaneously. If you don’t want this, you can set `concurrent: false`
-* `chunkSize` — Max allowed chunk size based on number of files for glob pattern. This is important on windows based systems to avoid command length limitations. See [#147](https://github.com/okonet/lint-staged/issues/147)
+* `chunkSize` — Max allowed chunk size based on number of files for glob pattern. This option is only applicable on Windows based systems to avoid command length limitations. See [#147](https://github.com/okonet/lint-staged/issues/147)
 * `globOptions` — `{ matchBase: true, dot: true }` — [micromatch options](https://github.com/micromatch/micromatch#options) to
   customize how glob patterns match files.
 * `ignore` - `['**/docs/**/*.js']` - array of glob patterns to entirely ignore from any task.
 * `linters` — `Object` — keys (`String`) are glob patterns, values (`Array<String> | String`) are commands to execute.
-* `subTaskConcurrency` — `1` — Controls concurrency for processing chunks generated for each linter. Execution is **not** concurrent by default(see [#225](https://github.com/okonet/lint-staged/issues/225))
+* `subTaskConcurrency` — `1` — Controls concurrency for processing chunks generated for each linter. This option is only applicable on Windows. Execution is **not** concurrent by default(see [#225](https://github.com/okonet/lint-staged/issues/225))
 
 ## Filtering files
 

package.json

@@ -34,6 +34,7 @@
     "execa": "^0.9.0",
     "find-parent-dir": "^0.3.0",
     "is-glob": "^4.0.0",
+    "is-windows": "^1.0.2",
     "jest-validate": "^22.4.0",
     "listr": "^0.13.0",
     "lodash": "^4.17.5",
@@ -49,6 +50,7 @@
     "stringify-object": "^3.2.2"
   },
   "devDependencies": {
+    "babel-plugin-transform-object-rest-spread": "^6.26.0",
     "babel-preset-env": "^1.6.0",
     "commitizen": "^2.9.6",
     "consolemock": "^1.0.2",

src/index.js

@@ -12,6 +12,7 @@ const debug = require('debug')('lint-staged')
 // Force colors for packages that depend on https://www.npmjs.com/package/supports-color
 // but do this only in TTY mode
 if (process.stdout.isTTY) {
+  // istanbul ignore next
   process.env.FORCE_COLOR = true
 }
 

src/makeCmdTasks.js

@@ -0,0 +1,37 @@
+'use strict'
+
+const resolveTaskFn = require('./resolveTaskFn')
+const resolveGitDir = require('./resolveGitDir')
+
+const debug = require('debug')('lint-staged:make-cmd-tasks')
+
+/**
+ * Creates and returns an array of listr tasks which map to the given commands.
+ *
+ * @param {Array<string>|string} commands
+ * @param {Array<string>} pathsToLint
+ * @param {Object} [options]
+ * @param {number} options.chunkSize
+ * @param {number} options.subTaskConcurrency
+ */
+module.exports = function makeCmdTasks(
+  commands,
+  pathsToLint,
+  { chunkSize = Number.MAX_SAFE_INTEGER, subTaskConcurrency = 1 } = {}
+) {
+  debug('Creating listr tasks for commands %o', commands)
+
+  const gitDir = resolveGitDir()
+  const lintersArray = Array.isArray(commands) ? commands : [commands]
+
+  return lintersArray.map(linter => ({
+    title: linter,
+    task: resolveTaskFn({
+      linter,
+      gitDir,
+      pathsToLint,
+      chunkSize,
+      subTaskConcurrency
+    })
+  }))
+}

src/printErrors.js

@@ -1,5 +1,6 @@
 'use strict'
 
+// istanbul ignore next
 // Work-around for duplicated error logs, see #142
 const errMsg = err => (err.privateMsg != null ? err.privateMsg : err.message)
 

src/resolveTaskFn.js

@@ -0,0 +1,118 @@
+'use strict'
+
+const chunk = require('lodash/chunk')
+const dedent = require('dedent')
+const isWindows = require('is-windows')
+const execa = require('execa')
+const symbols = require('log-symbols')
+const pMap = require('p-map')
+const calcChunkSize = require('./calcChunkSize')
+const findBin = require('./findBin')
+
+const debug = require('debug')('lint-staged:task')
+
+/**
+ * Execute the given linter binary with arguments and file paths using execa and
+ * return the promise.
+ *
+ * @param {string} bin
+ * @param {Array<string>} args
+ * @param {Object} execaOptions
+ * @param {Array<string>} pathsToLint
+ * @return {Promise}
+ */
+function execLinter(bin, args, execaOptions, pathsToLint) {
+  const binArgs = args.concat(pathsToLint)
+
+  debug('bin:', bin)
+  debug('args: %O', binArgs)
+  debug('opts: %o', execaOptions)
+
+  return execa(bin, binArgs, Object.assign({}, execaOptions))
+}
+
+const successMsg = linter => `${symbols.success} ${linter} passed!`
+
+/**
+ * Create and returns an error instance with given stdout and stderr. If we set
+ * the message on the error instance, it gets logged multiple times(see #142).
+ * So we set the actual error message in a private field and extract it later,
+ * log only once.
+ *
+ * @param {string} linter
+ * @param {string} errStdout
+ * @param {string} errStderr
+ * @returns {Error}
+ */
+function makeErr(linter, errStdout, errStderr) {
+  const err = new Error()
+  err.privateMsg = dedent`
+    ${symbols.error} "${linter}" found some errors. Please fix them and try committing again.
+    ${errStdout}
+    ${errStderr}
+  `
+  return err
+}
+
+/**
+ * Returns the task function for the linter. It handles chunking for file paths
+ * if the OS is Windows.
+ *
+ * @param {Object} options
+ * @param {string} options.linter
+ * @param {string} options.gitDir
+ * @param {Array<string>} options.pathsToLint
+ * @param {number} options.chunkSize
+ * @param {number} options.subTaskConcurrency
+ * @returns {function(): Promise<string>}
+ */
+module.exports = function resolveTaskFn(options) {
+  const { linter, gitDir, pathsToLint } = options
+  const { bin, args } = findBin(linter)
+
+  const execaOptions = { reject: false }
+  // 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(bin) && gitDir !== process.cwd()) {
+    execaOptions.cwd = gitDir
+  }
+
+  if (!isWindows()) {
+    debug('%s  OS: %s; File path chunking unnecessary', symbols.success, process.platform)
+    return () =>
+      execLinter(bin, args, execaOptions, pathsToLint).then(result => {
+        if (!result.failed) return successMsg(linter)
+
+        throw makeErr(linter, result.stdout, result.stderr)
+      })
+  }
+
+  const { chunkSize, subTaskConcurrency: concurrency } = options
+
+  const filePathChunks = chunk(pathsToLint, calcChunkSize(pathsToLint, chunkSize))
+  const mapper = execLinter.bind(null, bin, args, execaOptions)
+
+  debug(
+    'OS: %s; Creating linter task with %d chunked file paths',
+    process.platform,
+    filePathChunks.length
+  )
+  return () =>
+    pMap(filePathChunks, mapper, { concurrency })
+      .catch(err => {
+        /* This will probably never be called. But just in case.. */
+        throw new Error(dedent`
+        ${symbols.error} ${linter} got an unexpected error.
+        ${err.message}
+      `)
+      })
+      .then(results => {
+        const errors = results.filter(res => res.failed)
+        if (errors.length === 0) return successMsg(linter)
+
+        const errStdout = errors.map(err => err.stdout).join('')
+        const errStderr = errors.map(err => err.stderr).join('')
+
+        throw makeErr(linter, errStdout, errStderr)
+      })
+}

src/runAll.js

@@ -4,7 +4,7 @@ const sgf = require('staged-git-files')
 const Listr = require('listr')
 const has = require('lodash/has')
 const pify = require('pify')
-const runScript = require('./runScript')
+const makeCmdTasks = require('./makeCmdTasks')
 const generateTasks = require('./generateTasks')
 const resolveGitDir = require('./resolveGitDir')
 
@@ -22,7 +22,7 @@ module.exports = function runAll(config) {
     throw new Error('Invalid config provided to runAll! Use getConfig instead.')
   }
 
-  const { concurrent, renderer } = config
+  const { concurrent, renderer, chunkSize, subTaskConcurrency } = config
   const gitDir = resolveGitDir()
   debug('Resolved git directory to be `%s`', gitDir)
 
@@ -35,13 +35,19 @@ module.exports = function runAll(config) {
     const tasks = generateTasks(config, filenames).map(task => ({
       title: `Running tasks for ${task.pattern}`,
       task: () =>
-        new Listr(runScript(task.commands, task.fileList, config), {
-          // In sub-tasks we don't want to run concurrently
-          // and we want to abort on errors
-          dateFormat: false,
-          concurrent: false,
-          exitOnError: true
-        }),
+        new Listr(
+          makeCmdTasks(task.commands, task.fileList, {
+            chunkSize,
+            subTaskConcurrency
+          }),
+          {
+            // In sub-tasks we don't want to run concurrently
+            // and we want to abort on errors
+            dateFormat: false,
+            concurrent: false,
+            exitOnError: true
+          }
+        ),
       skip: () => {
         if (task.fileList.length === 0) {
           return `No staged files match ${task.pattern}`

test/findBin.spec.js

@@ -2,8 +2,6 @@ import makeConsoleMock from 'consolemock'
 import npmWhichMock from 'npm-which'
 import findBin from '../src/findBin'
 
-jest.mock('npm-which')
-
 describe('findBin', () => {
   it('should return path to bin', () => {
     const { bin, args } = findBin('my-linter')

test/index.spec.js

@@ -8,8 +8,6 @@ const replaceSerializer = (from, to) => ({
   print: val => val.replace(from, to)
 })
 
-jest.mock('cosmiconfig')
-
 const mockCosmiconfigWith = result => {
   cosmiconfig.mockImplementationOnce(() => ({
     load: () => Promise.resolve(result)

test/makeCmdTasks.spec.js

@@ -0,0 +1,44 @@
+import execa from 'execa'
+import makeCmdTasks from '../src/makeCmdTasks'
+
+describe('makeCmdTasks', () => {
+  beforeEach(() => {
+    execa.mockClear()
+  })
+
+  it('should return an array', () => {
+    expect(makeCmdTasks('test', ['test.js'])).toBeInstanceOf(Array)
+  })
+
+  it('should work with a single command', async () => {
+    expect.assertions(4)
+    const res = makeCmdTasks('test', ['test.js'])
+    expect(res.length).toBe(1)
+    const [linter] = res
+    expect(linter.title).toBe('test')
+    expect(linter.task).toBeInstanceOf(Function)
+    const taskPromise = linter.task()
+    expect(taskPromise).toBeInstanceOf(Promise)
+    await taskPromise
+  })
+
+  it('should work with multiple commands', async () => {
+    expect.assertions(9)
+    const res = makeCmdTasks(['test', 'test2'], ['test.js'])
+    expect(res.length).toBe(2)
+    const [linter1, linter2] = res
+    expect(linter1.title).toBe('test')
+    expect(linter2.title).toBe('test2')
+
+    let taskPromise = linter1.task()
+    expect(taskPromise).toBeInstanceOf(Promise)
+    await taskPromise
+    expect(execa).toHaveBeenCalledTimes(1)
+    expect(execa).lastCalledWith('test', ['test.js'], { reject: false })
+    taskPromise = linter2.task()
+    expect(taskPromise).toBeInstanceOf(Promise)
+    await taskPromise
+    expect(execa).toHaveBeenCalledTimes(2)
+    expect(execa).lastCalledWith('test2', ['test.js'], { reject: false })
+  })
+})