feat: Run linters with configurable concurrency (#149)

If the amount of staged files is big, passing them as arguments can exceed a max string limit (on Windows it's `8191`). This PR addresses it by splitting work into chunks based on this limit and running linters in parallel.

Closes #147

.babelrc

@@ -1,3 +1,16 @@
 {
-    "presets": ["es2015", "stage-0"]
+  "presets": [
+    ["env", {
+      "targets": {
+        "node": "current"
+      }
+    }]
+  ],
+  "plugins": [
+    ["transform-runtime", {
+      "helpers": false,
+      "polyfill": false,
+      "regenerator": true
+    }]
+  ]
 }

README.md

@@ -88,6 +88,8 @@ To set options and keep lint-staged extensible, advanced format can be used. Thi
 * `linters` — `Object` — keys (`String`) are glob patterns, values (`Array<String> | String`) are commands to execute.
 * `gitDir` — Sets the relative path to the `.git` root. Useful when your `package.json` is located in a subdirectory. See [working from a subdirectory](#working-from-a-subdirectory)
 * `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
+* `subTaskConcurrency` — `2` — Controls concurrency for processing chunks generated for each linter.
 * `verbose` — *false* — runs lint-staged in verbose mode. When `true` it will use https://github.com/SamVerschueren/listr-verbose-renderer.
 
 ## Filtering files

package.json

@@ -49,16 +49,19 @@
     "cosmiconfig": "^1.1.0",
     "execa": "^0.6.0",
     "listr": "^0.12.0",
+    "lodash.chunk": "^4.2.0",
     "minimatch": "^3.0.0",
     "npm-which": "^3.0.1",
+    "p-map": "^1.1.1",
     "staged-git-files": "0.0.4"
   },
   "devDependencies": {
     "babel-core": "^6.10.4",
     "babel-jest": "^20.0.0",
-    "babel-preset-es2015": "^6.9.0",
-    "babel-preset-stage-0": "^6.5.0",
+    "babel-plugin-transform-runtime": "^6.23.0",
+    "babel-preset-env": "^1.4.0",
     "babel-register": "^6.16.3",
+    "babel-runtime": "^6.23.0",
     "cz-conventional-changelog": "^1.2.0",
     "eslint": "^3.9.1",
     "eslint-config-okonet": "^1.1.1",

src/calcChunkSize.js

@@ -0,0 +1,42 @@
+'use strict'
+
+/**
+ * Calculates and returns the chunk size for given file paths and `chunkSize`
+ * option.
+ *
+ * It returns the minimum of the following:
+ *
+ *  - Total number of files
+ *  - Max allowed chunk size so that command length does not exceed the system
+ *    limitation on windows
+ *  - User specified chunk size or the default
+ *
+ * Worked example:
+ * **Assumption** - Our max file path length is 100, Hence max allowed chunk
+ * size is 80
+ *
+ *  - Case 1: Only 10 files are there, chunk size should be 10 only
+ *  - Case 2: There are 100 files and user has overridden the option with
+ *    chunk size 40. So chunk size should be 40
+ *  - Case 3: There are 100 files and user has overridden the option with
+ *    chunk size 100. So chunk size should be 80
+ *
+ * @param {Array<string>} paths The array of file paths
+ * @param {number} idealChunkSize User specified / default chunk size
+ * @returns {number} The chunk size
+ */
+module.exports = function calcChunkSize(paths, idealChunkSize) {
+    /* What is the longest file path? */
+    const maxPathLen = paths.reduce(
+        (maxLen, filePath) => Math.max(maxLen, filePath.length),
+        20 // safe initial value
+    )
+
+    /* In the worst case scenario, */
+    /* how many files can we process in a single command? */
+    /* For windows systems, command length is limited to 8192 */
+    const maxAllowedChunkSize = Math.floor(8000 / maxPathLen)
+
+    /* Configured chunk size / default - idealChunkSize */
+    return Math.min(paths.length, maxAllowedChunkSize, idealChunkSize)
+}

src/findBin.js

@@ -2,24 +2,20 @@
 
 const npmWhich = require('npm-which')(process.cwd())
 
-module.exports = function findBin(cmd, paths, packageJson, options) {
-    const defaultArgs = ['--'].concat(paths)
+module.exports = function findBin(cmd, packageJson, options) {
     /*
     * If package.json has script with cmd defined
     * we want it to be executed first
     */
     if (packageJson.scripts && packageJson.scripts[cmd] !== undefined) {
         // Support for scripts from package.json
-        return {
-            bin: 'npm',
-            args: [
-                'run',
-                options && options.verbose ? undefined : '--silent',
-                cmd
-            ]
-                .filter(Boolean)
-                .concat(defaultArgs)
-        }
+        const args = [
+            'run',
+            options && options.verbose ? undefined : '--silent',
+            cmd
+        ].filter(Boolean)
+
+        return { bin: 'npm', args }
     }
 
     /*
@@ -52,8 +48,5 @@ module.exports = function findBin(cmd, paths, packageJson, options) {
         throw new Error(`${ bin } could not be found. Try \`npm install ${ bin }\`.`)
     }
 
-    return {
-        bin,
-        args: args.concat(defaultArgs)
-    }
+    return { bin, args }
 }

src/index.js

@@ -12,6 +12,7 @@ const cosmiconfig = require('cosmiconfig')
 const packageJson = require(appRoot.resolve('package.json')) // eslint-disable-line
 const runScript = require('./runScript')
 const generateTasks = require('./generateTasks')
+const readConfigOption = require('./readConfigOption')
 
 // Force colors for packages that depend on https://www.npmjs.com/package/supports-color
 // but do this only in TTY mode
@@ -27,10 +28,11 @@ cosmiconfig('lint-staged', {
         // result.config is the parsed configuration object
         // result.filepath is the path to the config file that was found
         const config = result.config
+
         const verbose = config.verbose
         // Output config in verbose mode
         if (verbose) console.log(config)
-        const concurrent = typeof config.concurrent !== 'undefined' ? config.concurrent : true
+        const concurrent = readConfigOption(config, 'concurrent', true)
         const renderer = verbose ? 'verbose' : 'update'
         const gitDir = config.gitDir ? path.resolve(config.gitDir) : process.cwd()
         sgf.cwd = gitDir
@@ -56,7 +58,7 @@ cosmiconfig('lint-staged', {
                                 task.commands,
                                 task.fileList,
                                 packageJson,
-                                { gitDir, verbose }
+                                { gitDir, verbose, config }
                             ), {
                                 // In sub-tasks we don't want to run concurrently
                                 // and we want to abort on errors

src/readConfigOption.js

@@ -0,0 +1,20 @@
+'use strict'
+
+/**
+ * Helper function for reading config option.
+ * Returns the `config` option for given `key` or the given `defaultValue`
+ * if `config` does not have the given `key`.
+ *
+ * @param {Object} config
+ * @param {string} key
+ * @param {*} defaultValue
+ * @returns {*}
+ */
+module.exports = function readConfigOption(config, key, defaultValue) {
+    if (typeof config !== 'undefined' && typeof config[key] !== 'undefined') {
+        return config[key]
+    }
+
+    return defaultValue
+}
+

src/runScript.js

@@ -1,33 +1,70 @@
 'use strict'
 
-const findBin = require('./findBin')
+const chunk = require('lodash.chunk')
 const execa = require('execa')
+const pMap = require('p-map')
+
+const calcChunkSize = require('./calcChunkSize')
+const findBin = require('./findBin')
+const readConfigOption = require('./readConfigOption')
 
 module.exports = function runScript(commands, pathsToLint, packageJson, options) {
+    const config = readConfigOption(options, 'config', {})
+
+    const concurrency = readConfigOption(config, 'subTaskConcurrency', 2)
+    const chunkSize = calcChunkSize(
+        pathsToLint,
+        readConfigOption(config, 'chunkSize', Number.MAX_SAFE_INTEGER)
+    )
+
+    const filePathChunks = chunk(pathsToLint, chunkSize)
+
     const lintersArray = Array.isArray(commands) ? commands : [commands]
+
     return lintersArray.map(linter => ({
         title: linter,
         task: () => {
             try {
-                const res = findBin(linter, pathsToLint, packageJson, options)
+                const res = findBin(linter, packageJson, options)
+
                 // Only use gitDir as CWD if we are using the git binary
                 // e.g `npm` should run tasks in the actual CWD
                 const execaOptions =
-                    res.bin.endsWith('git') && options && options.gitDir ? { cwd: options.gitDir } : {}
-                return new Promise((resolve, reject) => {
-                    execa(res.bin, res.args, execaOptions)
-                        .then(() => {
-                            resolve(`✅ ${ linter } passed!`)
-                        })
+                    res.bin.endsWith('git') && options && options.gitDir
+                    ? { cwd: options.gitDir } : {}
+
+                const errors = []
+                const mapper = (pathsChunk) => {
+                    const args = res.args.concat(['--'], pathsChunk)
+
+                    return execa(res.bin, args, Object.assign({}, execaOptions))
+                        /* If we don't catch, pMap will terminate on first rejection */
+                        /* We want error information of all chunks */
                         .catch((err) => {
-                            reject(new Error(`🚫 ${ linter } found some errors. Please fix them and try committing again.
-${ err.stderr }
-${ err.stdout }`))
+                            errors.push(err)
                         })
-                })
+                }
+
+                return pMap(filePathChunks, mapper, { concurrency })
+                    .catch((err) => {
+                        /* This will probably never be called. But just in case.. */
+                        throw new Error(`🚫 ${ linter } got an unexpected error.
+${ err.message }`)
+                    })
+                    .then(() => {
+                        if (errors.length === 0) return `✅ ${ linter } passed!`
+
+                        const errStdout = errors.map(err => err.stdout).join('')
+                        const errStderr = errors.map(err => err.stderr).join('')
+
+                        throw new Error(`🚫 ${ linter } found some errors. Please fix them and try committing again.
+${ errStdout }
+${ errStderr }`)
+                    })
             } catch (err) {
                 throw err
             }
         }
     }))
 }
+

test/calcChunkSize.spec.js

@@ -0,0 +1,27 @@
+import calcChunkSize from '../src/calcChunkSize'
+
+// This is only ever used for length so the contents do not matter much
+const testFilePath =
+    'This-is-only-ever-used-for-length-so-the-contents-do-not-matter-much.length-is-100-for-simplicity.js'
+
+describe('calcChunkSize', () => {
+    it('should not return high chunk size for less files', () => {
+        let chunkSize = calcChunkSize([testFilePath], 50)
+        expect(chunkSize).toEqual(1)
+
+        chunkSize = calcChunkSize([testFilePath, testFilePath], 50)
+        expect(chunkSize).toEqual(2)
+    })
+
+    it('should not return chunk size which will fail max command length', () => {
+        const fakeFilePaths = Array(200).fill(testFilePath)
+        const chunkSize = calcChunkSize(fakeFilePaths, Number.MAX_SAFE_INTEGER)
+        expect(chunkSize).toEqual(80)
+    })
+
+    it('should respect option chunkSize where ever possible', () => {
+        const fakeFilePaths = Array(200).fill(testFilePath)
+        const chunkSize = calcChunkSize(fakeFilePaths, 50)
+        expect(chunkSize).toEqual(50)
+    })
+})

test/findBin.spec.js

@@ -18,9 +18,9 @@ describe('findBin', () => {
                 'my-linter': 'my-linter'
             }
         }
-        const { bin, args } = findBin('my-linter', 'test.js', packageJSONMock)
+        const { bin, args } = findBin('my-linter', packageJSONMock)
         expect(bin).toEqual('npm')
-        expect(args).toEqual(['run', '--silent', 'my-linter', '--', 'test.js'])
+        expect(args).toEqual(['run', '--silent', 'my-linter'])
     })
 
     it('should return npm run command without --silent in verbose mode', () => {
@@ -29,27 +29,27 @@ describe('findBin', () => {
                 eslint: 'eslint'
             }
         }
-        const { bin, args } = findBin('eslint', 'test.js', packageJSONMock, { verbose: true })
+        const { bin, args } = findBin('eslint', packageJSONMock, { verbose: true })
         expect(bin).toEqual('npm')
-        expect(args).toEqual(['run', 'eslint', '--', 'test.js'])
+        expect(args).toEqual(['run', 'eslint'])
     })
 
     it('should return path to bin if there is no `script` with name in package.json', () => {
-        const { bin, args } = findBin('my-linter', 'test.js test2.js', packageJSON)
+        const { bin, args } = findBin('my-linter', packageJSON)
         expect(bin).toEqual('my-linter')
-        expect(args).toEqual(['--', 'test.js test2.js'])
+        expect(args).toEqual([])
     })
 
     it('should throw an error if bin not found and there is no entry in scripts section', () => {
         expect(() => {
-            findBin('my-missing-linter', 'test.js', packageJSON)
+            findBin('my-missing-linter', packageJSON)
         }).toThrow('my-missing-linter could not be found. Try `npm install my-missing-linter`.')
     })
 
 
     it('should parse cmd and add arguments to args', () => {
-        const { bin, args } = findBin('my-linter task --fix', 'test.js test2.js', packageJSON)
+        const { bin, args } = findBin('my-linter task --fix', packageJSON)
         expect(bin).toEqual('my-linter')
-        expect(args).toEqual(['task', '--fix', '--', 'test.js test2.js'])
+        expect(args).toEqual(['task', '--fix'])
     })
 })

test/readConfigOption.spec.js

@@ -0,0 +1,20 @@
+import readConfigOption from '../src/readConfigOption'
+
+describe('readConfigOption', () => {
+
+    it('should return default value if config is undefined', () => {
+        const configOption = readConfigOption(undefined, 'my_key', 'default_value')
+        expect(configOption).toEqual('default_value')
+    })
+
+    it('should return default value if config option is undefined', () => {
+        const configOption = readConfigOption({}, 'my_key', 'default_value')
+        expect(configOption).toEqual('default_value')
+    })
+
+    it('should return config option if not undefined', () => {
+        const configOption = readConfigOption({ my_key: 'my_value' }, 'my_key', 'default_value')
+        expect(configOption).toEqual('my_value')
+    })
+
+})