From 2ca9050c4d69a123252a09c8547e07db5fb6308d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20Ja=CC=88ppinen?= Date: Tue, 25 Jun 2019 07:48:25 +0300 Subject: [PATCH] refactor: remove support for chunking BREAKING CHANGE: Very long arguments strings are no longer chunked on Windows. Function linters should be used instead to customise this behaviour. --- package.json | 2 - src/calcChunkSize.js | 42 -------- src/resolveTaskFn.js | 59 ++--------- test/calcChunkSize.spec.js | 27 ----- test/resolveTaskFn-chunked.spec.js | 158 ----------------------------- 5 files changed, 6 insertions(+), 282 deletions(-) delete mode 100644 src/calcChunkSize.js delete mode 100644 test/calcChunkSize.spec.js delete mode 100644 test/resolveTaskFn-chunked.spec.js diff --git a/package.json b/package.json index 3fb9f917f..83c21e077 100644 --- a/package.json +++ b/package.json @@ -34,14 +34,12 @@ "del": "^3.0.0", "execa": "^1.0.0", "is-glob": "^4.0.0", - "is-windows": "^1.0.2", "listr": "^0.14.2", "listr-update-renderer": "^0.5.0", "lodash": "^4.17.11", "log-symbols": "^2.2.0", "micromatch": "^3.1.8", "npm-which": "^3.0.1", - "p-map": "^1.1.1", "path-is-inside": "^1.0.2", "please-upgrade-node": "^3.0.2", "string-argv": "^0.0.2", diff --git a/src/calcChunkSize.js b/src/calcChunkSize.js deleted file mode 100644 index bfd16b310..000000000 --- a/src/calcChunkSize.js +++ /dev/null @@ -1,42 +0,0 @@ -'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/resolveTaskFn.js b/src/resolveTaskFn.js index 3d27da403..6312be61b 100644 --- a/src/resolveTaskFn.js +++ b/src/resolveTaskFn.js @@ -1,13 +1,9 @@ 'use strict' -const chunk = require('lodash/chunk') const dedent = require('dedent') -const isWindows = require('is-windows') const execa = require('execa') const chalk = require('chalk') 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') @@ -108,55 +104,12 @@ module.exports = function resolveTaskFn(options) { execaOptions.cwd = gitDir } - if (!isWindows()) { - debug('%s OS: %s; File path chunking unnecessary', symbols.success, process.platform) - return ctx => - execLinter(bin, argsWithPaths, execaOptions).then(result => { - if (result.failed || result.killed || result.signal != null) { - throw makeErr(linter, result, ctx) - } - - return successMsg(linter) - }) - } - - const { chunkSize, subTaskConcurrency: concurrency } = options - - const filePathChunks = chunk(pathsToLint, calcChunkSize(pathsToLint, chunkSize)) - const mapper = execLinter.bind(null, bin, argsWithPaths, execaOptions) - - debug( - 'OS: %s; Creating linter task with %d chunked file paths', - process.platform, - filePathChunks.length - ) return ctx => - 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 || res.killed) - const failed = results.some(res => res.failed) - const killed = results.some(res => res.killed) - const signals = results.map(res => res.signal).filter(Boolean) - - if (failed || killed || signals.length > 0) { - const finalResult = { - stdout: errors.map(err => err.stdout).join(''), - stderr: errors.map(err => err.stderr).join(''), - failed, - killed, - signal: signals.join(', ') - } - - throw makeErr(linter, finalResult, ctx) - } + execLinter(bin, argsWithPaths, execaOptions).then(result => { + if (result.failed || result.killed || result.signal != null) { + throw makeErr(linter, result, ctx) + } - return successMsg(linter) - }) + return successMsg(linter) + }) } diff --git a/test/calcChunkSize.spec.js b/test/calcChunkSize.spec.js deleted file mode 100644 index d6bf7b0e7..000000000 --- a/test/calcChunkSize.spec.js +++ /dev/null @@ -1,27 +0,0 @@ -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/resolveTaskFn-chunked.spec.js b/test/resolveTaskFn-chunked.spec.js deleted file mode 100644 index c57313cdf..000000000 --- a/test/resolveTaskFn-chunked.spec.js +++ /dev/null @@ -1,158 +0,0 @@ -import execa from 'execa' -import isWindows from 'is-windows' -import pMap from 'p-map' -import resolveTaskFn from '../src/resolveTaskFn' - -jest.mock('is-windows', () => jest.fn(() => true)) -jest.mock('p-map') - -const defaultOpts = { - linter: 'test', - pathsToLint: ['test.js'], - chunkSize: 1, - subTaskConcurrency: 1 -} - -describe('resolveTaskFn', () => { - describe('chunked tasks', () => { - afterEach(() => { - execa.mockClear() - pMap.mockClear() - }) - - pMap.mockResolvedValue([ - { - stdout: 'a-ok', - stderr: '', - code: 0, - failed: false, - cmd: 'mock cmd' - } - ]) - - it('should invoke execa in mapper function', async () => { - expect.assertions(3) - - const taskFn = resolveTaskFn({ ...defaultOpts }) - await taskFn() - const [[[chunk], mapper]] = pMap.mock.calls - expect(pMap).toHaveBeenCalledWith([['test.js']], mapper, { concurrency: 1 }) - await mapper(chunk) - expect(execa).toHaveBeenCalledTimes(1) - expect(execa).toHaveBeenCalledWith('test', ['test.js'], { reject: false }) - }) - - it('should respect chunk size and concurrency', async () => { - expect.assertions(6) - - const taskFn = resolveTaskFn({ - ...defaultOpts, - pathsToLint: ['test1.js', 'test2.js'], - subTaskConcurrency: 2 - }) - await taskFn() - const [[chunks, mapper]] = pMap.mock.calls - expect(mapper).toBeInstanceOf(Function) - expect(pMap).toHaveBeenCalledWith([['test1.js'], ['test2.js']], mapper, { concurrency: 2 }) - - // Check that calling the mapper invokes execa - const [c1, c2] = chunks - await mapper(c1) - expect(execa).toHaveBeenCalledTimes(1) - expect(execa).lastCalledWith('test', ['test1.js'], { reject: false }) - await mapper(c2) - expect(execa).toHaveBeenCalledTimes(2) - expect(execa).lastCalledWith('test', ['test2.js'], { reject: false }) - }) - - it('should not return task fn with chunked execution if OS is not Windows', async () => { - expect.assertions(3) - isWindows.mockReturnValueOnce(false) - - const taskFn = resolveTaskFn({ - ...defaultOpts, - pathsToLint: ['test1.js', 'test2.js'] - }) - await taskFn() - expect(pMap).not.toHaveBeenCalled() - expect(execa).toHaveBeenCalledTimes(1) - expect(execa).toHaveBeenCalledWith('test', ['test1.js', 'test2.js'], { reject: false }) - }) - - it('should throw error for failed linters', async () => { - expect.assertions(1) - pMap.mockResolvedValueOnce([ - { - stdout: 'Mock error', - stderr: '', - code: 0, - failed: true, - killed: false, - signal: null, - cmd: 'mock cmd' - } - ]) - - const taskFn = resolveTaskFn({ - ...defaultOpts, - linter: 'mock-fail-linter' - }) - try { - await taskFn() - } catch (err) { - expect(err.privateMsg).toMatchInlineSnapshot(` -" - - -× mock-fail-linter found some errors. Please fix them and try committing again. -Mock error" -`) - } - }) - - it('should throw error for killed processes', async () => { - expect.assertions(1) - pMap.mockResolvedValueOnce([ - { - stdout: 'Mock error', - stderr: '', - code: 0, - failed: false, - killed: false, - signal: 'SIGINT', - cmd: 'mock cmd' - } - ]) - - const taskFn = resolveTaskFn({ - ...defaultOpts, - linter: 'mock-killed-linter' - }) - try { - await taskFn() - } catch (err) { - expect(err.privateMsg).toMatchInlineSnapshot(` -" - - -‼ mock-killed-linter was terminated with SIGINT" -`) - } - }) - - it('should handle unexpected error', async () => { - expect.assertions(1) - pMap.mockRejectedValueOnce(new Error('Unexpected Error')) - - const taskFn = resolveTaskFn({ ...defaultOpts }) - try { - await taskFn() - } catch (err) { - expect(err.message).toMatchInlineSnapshot(` -"× test got an unexpected error. -Unexpected Error" -`) - } - }) - }) -})