From 0dd0c9486bc72b5d13ea021c0148765fc30b262a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20J=C3=A4ppinen?= Date: Wed, 3 Jul 2019 07:03:09 +0300 Subject: [PATCH] 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. --- src/makeCmdTasks.js | 38 +++++++--------- src/resolveTaskFn.js | 68 +++++++++++++---------------- test/makeCmdTasks.spec.js | 22 +++++++--- test/resolveTaskFn.spec.js | 36 ++++++++++----- test/resolveTaskFn.unmocked.spec.js | 13 +++--- 5 files changed, 92 insertions(+), 85 deletions(-) diff --git a/src/makeCmdTasks.js b/src/makeCmdTasks.js index 5598baf34..8ac8d1882 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 6be3bd751..b6b52d9ae 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} 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} options.pathsToLint * @returns {function(): Promise>} */ -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) + }) } diff --git a/test/makeCmdTasks.spec.js b/test/makeCmdTasks.spec.js index ee6cfc2b5..037df25e0 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 1a1a03464..783d83a7e 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 01b7c3d2f..feab1bb24 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!"` + ) }) })