diff --git a/.babelrc b/.babelrc index 341114c60..d9b12702c 100644 --- a/.babelrc +++ b/.babelrc @@ -1,3 +1,16 @@ { - "presets": ["es2015", "stage-0"] + "presets": [ + ["env", { + "targets": { + "node": "current" + } + }] + ], + "plugins": [ + ["transform-runtime", { + "helpers": false, + "polyfill": false, + "regenerator": true + }] + ] } diff --git a/README.md b/README.md index dafd985c9..ee292b89d 100644 --- a/README.md +++ b/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`) 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 diff --git a/package.json b/package.json index 5d9cbcba0..3e3d406b2 100644 --- a/package.json +++ b/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", diff --git a/src/calcChunkSize.js b/src/calcChunkSize.js new file mode 100644 index 000000000..e4d0de9e3 --- /dev/null +++ b/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} 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) +} diff --git a/src/findBin.js b/src/findBin.js index f08389cfe..e8009105e 100644 --- a/src/findBin.js +++ b/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 } } diff --git a/src/index.js b/src/index.js index 2e6ffe005..88dc4e633 100644 --- a/src/index.js +++ b/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 diff --git a/src/readConfigOption.js b/src/readConfigOption.js new file mode 100644 index 000000000..538a85feb --- /dev/null +++ b/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 +} + diff --git a/src/runScript.js b/src/runScript.js index 970ef88c0..eb924688c 100644 --- a/src/runScript.js +++ b/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 } } })) } + diff --git a/test/calcChunkSize.spec.js b/test/calcChunkSize.spec.js new file mode 100644 index 000000000..9313ed349 --- /dev/null +++ b/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) + }) +}) diff --git a/test/findBin.spec.js b/test/findBin.spec.js index 2f6500b04..6e875d90b 100644 --- a/test/findBin.spec.js +++ b/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']) }) }) diff --git a/test/readConfigOption.spec.js b/test/readConfigOption.spec.js new file mode 100644 index 000000000..ac863e287 --- /dev/null +++ b/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') + }) + +}) diff --git a/test/runScript-mock-findBin.spec.js b/test/runScript-mock-findBin.spec.js index 139d69129..dc7f5f876 100644 --- a/test/runScript-mock-findBin.spec.js +++ b/test/runScript-mock-findBin.spec.js @@ -10,7 +10,7 @@ import runScript from '../src/runScript' jest.mock('execa') // Mock findBin to return an absolute path -jest.mock('../src/findBin', () => (commands, paths) => { +jest.mock('../src/findBin', () => (commands) => { const [ bin, ...otherArgs @@ -18,7 +18,7 @@ jest.mock('../src/findBin', () => (commands, paths) => { return ({ bin: `/usr/local/bin/${ bin }`, - args: otherArgs.concat(['--']).concat(paths) + args: otherArgs }) }, { virtual: true }) @@ -44,18 +44,20 @@ const packageJSON = { describe.only('runScript with absolute paths', () => { beforeEach(() => { mockFn.mockReset() - mockFn.mockImplementation(() => new Promise(() => {})) + mockFn.mockImplementation(() => Promise.resolve(true)) }) - it('can pass `gitDir` as `cwd` to `execa()` when git is called via absolute path', () => { + it('can pass `gitDir` as `cwd` to `execa()` when git is called via absolute path', async () => { const res = runScript( ['git add'], - 'test.js', + ['test.js'], packageJSON, { gitDir: '../' } ) - expect(res[0].task()).toBeAPromise() + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0][0]).toMatch('/usr/local/bin/git') expect(mockFn.mock.calls[0][1]).toEqual(['add', '--', 'test.js']) diff --git a/test/runScript-mock-pMap.spec.js b/test/runScript-mock-pMap.spec.js new file mode 100644 index 000000000..f2125dee8 --- /dev/null +++ b/test/runScript-mock-pMap.spec.js @@ -0,0 +1,48 @@ +import expect from 'expect' +import pMap from 'p-map' +import runScript from '../src/runScript' + +jest.mock('p-map', () => jest.fn(() => Promise.resolve(true))) + +const packageJSON = { + scripts: { + test: 'noop', + test2: 'noop' + }, + 'lint-staged': {} +} + +describe('runScript', () => { + + it('should respect concurrency', () => { + const res = runScript( + ['test'], + ['test1.js', 'test2.js'], + packageJSON, + { config: { chunkSize: 1, subTaskConcurrency: 1 } } + ) + res[0].task() + expect(pMap.mock.calls.length).toEqual(1) + const pMapArgs = pMap.mock.calls[0] + expect(pMapArgs[0]).toEqual([['test1.js'], ['test2.js']]) + expect(pMapArgs[1]).toBeA('function') + expect(pMapArgs[2]).toEqual({ concurrency: 1 }) + }) + + it('should handle unexpected error', async () => { + pMap.mockImplementation(() => Promise.reject(new Error('Unexpected Error'))) + + const res = runScript( + ['test'], + ['test.js'], + packageJSON + ) + try { + await res[0].task() + } catch (err) { + expect(err.message).toMatch(`🚫 test got an unexpected error. +Unexpected Error`) + } + }) + +}) diff --git a/test/runScript.spec.js b/test/runScript.spec.js index a12251509..4d5a00859 100644 --- a/test/runScript.spec.js +++ b/test/runScript.spec.js @@ -30,121 +30,178 @@ describe('runScript', () => { beforeEach(() => { mockFn.mockReset() - mockFn.mockImplementation(() => new Promise(() => {})) + mockFn.mockImplementation(() => Promise.resolve(true)) }) it('should return an array', () => { - expect(runScript('test', 'test.js', packageJSON)).toBeA('array') + expect(runScript('test', ['test.js'], packageJSON)).toBeA('array') }) it('should throw for non-existend script', () => { expect(() => { - runScript('missing-module', 'test.js', packageJSON)[0].task() + runScript('missing-module', ['test.js'], packageJSON)[0].task() }).toThrow() }) - it('should work with a single command', () => { - const res = runScript('test', 'test.js', packageJSON) + it('should work with a single command', async () => { + const res = runScript('test', ['test.js'], packageJSON) expect(res.length).toBe(1) expect(res[0].title).toBe('test') expect(res[0].task).toBeA('function') - expect(res[0].task()).toBeAPromise() + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise }) - it('should work with multiple commands', () => { - const res = runScript(['test', 'test2'], 'test.js', packageJSON) + it('should work with multiple commands', async () => { + const res = runScript(['test', 'test2'], ['test.js'], packageJSON) expect(res.length).toBe(2) expect(res[0].title).toBe('test') expect(res[1].title).toBe('test2') - expect(res[0].task()).toBeAPromise() + let taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0]).toEqual( ['npm', ['run', '--silent', 'test', '--', 'test.js'], {}] ) - expect(res[1].task()).toBeAPromise() + taskPromise = res[1].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(2) expect(mockFn.mock.calls[1]).toEqual( ['npm', ['run', '--silent', 'test2', '--', 'test.js'], {}] ) }) - it('should support non npm scripts', () => { - const res = runScript(['node --arg=true ./myscript.js', 'git add'], 'test.js', packageJSON) + it('should respect chunk size', async () => { + const res = runScript( + ['test'], + ['test1.js', 'test2.js'], + packageJSON, + { config: { chunkSize: 1 } } + ) + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise + expect(mockFn.mock.calls.length).toEqual(2) + expect(mockFn.mock.calls[0]).toEqual( + ['npm', ['run', '--silent', 'test', '--', 'test1.js'], {}] + ) + expect(mockFn.mock.calls[1]).toEqual( + ['npm', ['run', '--silent', 'test', '--', 'test2.js'], {}] + ) + }) + + it('should support non npm scripts', async () => { + const res = runScript(['node --arg=true ./myscript.js', 'git add'], ['test.js'], packageJSON) expect(res.length).toBe(2) expect(res[0].title).toBe('node --arg=true ./myscript.js') expect(res[1].title).toBe('git add') - expect(res[0].task()).toBeAPromise() + let taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0][0]).toContain('node') expect(mockFn.mock.calls[0][1]).toEqual(['--arg=true', './myscript.js', '--', 'test.js']) - expect(res[1].task()).toBeAPromise() + taskPromise = res[1].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(2) expect(mockFn.mock.calls[1][0]).toContain('git') expect(mockFn.mock.calls[1][1]).toEqual(['add', '--', 'test.js']) }) - it('should pass cwd to execa if gitDir option is set for non-npm tasks', () => { + it('should pass cwd to execa if gitDir option is set for non-npm tasks', async () => { const res = runScript( ['test', 'git add'], - 'test.js', + ['test.js'], packageJSON, { gitDir: '../' } ) - expect(res[0].task()).toBeAPromise() + let taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0]).toEqual( ['npm', ['run', '--silent', 'test', '--', 'test.js'], {}] ) - expect(res[1].task()).toBeAPromise() + taskPromise = res[1].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(2) expect(mockFn.mock.calls[1][0]).toMatch(/git$/) expect(mockFn.mock.calls[1][1]).toEqual(['add', '--', 'test.js']) expect(mockFn.mock.calls[1][2]).toEqual({ cwd: '../' }) }) - it('should not pass `gitDir` as `cwd` to `execa()` if a non-git binary is called', () => { + it('should not pass `gitDir` as `cwd` to `execa()` if a non-git binary is called', async () => { const res = runScript( ['jest'], - 'test.js', + ['test.js'], packageJSON, { gitDir: '../' } ) - expect(res[0].task()).toBeAPromise() + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0]).toEqual( ['jest', ['--', 'test.js'], {}] ) }) - it('should use --silent in non-verbose mode', () => { + it('should use --silent in non-verbose mode', async () => { const res = runScript( 'test', - 'test.js', + ['test.js'], packageJSON, { verbose: false } ) - expect(res[0].task()).toBeAPromise() + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0]).toEqual( ['npm', ['run', '--silent', 'test', '--', 'test.js'], {}] ) }) - it('should not use --silent in verbose mode', () => { + it('should not use --silent in verbose mode', async () => { const res = runScript( 'test', - 'test.js', + ['test.js'], packageJSON, { verbose: true } ) - expect(res[0].task()).toBeAPromise() + const taskPromise = res[0].task() + expect(taskPromise).toBeAPromise() + await taskPromise expect(mockFn.mock.calls.length).toEqual(1) expect(mockFn.mock.calls[0]).toEqual( ['npm', ['run', 'test', '--', 'test.js'], {}] ) }) + + it('should throw error for failed linters', async () => { + const linteErr = new Error() + linteErr.stdout = 'Mock error' + linteErr.stderr = '' + mockFn.mockImplementation(() => Promise.reject(linteErr)) + + const res = runScript('mock-fail-linter', ['test.js'], packageJSON) + const taskPromise = res[0].task() + try { + await taskPromise + } catch (err) { + expect(err.message).toMatch(`🚫 mock-fail-linter found some errors. Please fix them and try committing again. +${ linteErr.stdout } +${ linteErr.stderr }`) + } + }) }) +