From abe4b92d7f6213b59d756d172298bc29bb2bd44c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20J=C3=A4ppinen?= Date: Fri, 21 Feb 2020 09:35:50 +0200 Subject: [PATCH 1/2] fix: evaluate functional configuration only once --- lib/makeCmdTasks.js | 37 ++++++++------- lib/validateConfig.js | 17 +------ .../__snapshots__/validateConfig.spec.js.snap | 12 ----- test/makeCmdTasks.spec.js | 45 ++++++++++--------- test/runAll.unmocked.spec.js | 4 +- test/validateConfig.spec.js | 7 --- 6 files changed, 49 insertions(+), 73 deletions(-) diff --git a/lib/makeCmdTasks.js b/lib/makeCmdTasks.js index 703611973..2b1682f3c 100644 --- a/lib/makeCmdTasks.js +++ b/lib/makeCmdTasks.js @@ -1,6 +1,7 @@ 'use strict' const resolveTaskFn = require('./resolveTaskFn') +const { createError } = require('./validateConfig') const debug = require('debug')('lint-staged:make-cmd-tasks') @@ -15,32 +16,34 @@ const debug = require('debug')('lint-staged:make-cmd-tasks') */ module.exports = async function makeCmdTasks({ commands, files, gitDir, shell }) { debug('Creating listr tasks for commands %o', commands) - const commandsArray = Array.isArray(commands) ? commands : [commands] + const commandArray = Array.isArray(commands) ? commands : [commands] const cmdTasks = [] - for (const cmd of commandsArray) { + for (const cmd of commandArray) { // command function may return array of commands that already include `stagedFiles` const isFn = typeof cmd === 'function' const resolved = isFn ? await cmd(files) : cmd const resolvedArray = Array.isArray(resolved) ? resolved : [resolved] // Wrap non-array command as array - // Function command should not be used as the task title as-is - // because the resolved string it might be very long - // Create a matching command array with [file] in place of file names - let mockCmdTasks - if (isFn) { - const mockFileList = Array(files.length).fill('[file]') - const resolved = await cmd(mockFileList) - mockCmdTasks = Array.isArray(resolved) ? resolved : [resolved] - } - - for (const [i, command] of resolvedArray.entries()) { + for (const command of resolvedArray) { let title = isFn ? '[Function]' : command - if (isFn && mockCmdTasks[i]) { - // If command is a function, use the matching mock command as title, - // but since might include multiple [file] arguments, shorten to one - title = mockCmdTasks[i].replace(/\[file\].*\[file\]/, '[file]') + + if (isFn) { + // If the function linter didn't return string | string[] it won't work + // Do the validation here instead of `validateConfig` to skip evaluating the function multiple times + if (typeof command !== 'string') { + throw new Error( + createError( + title, + 'Function task should return a string or an array of strings', + resolved + ) + ) + } + + const [startOfFn] = command.split(' ') + title += ` ${startOfFn} ...` // Append function name, like `[Function] eslint ...` } cmdTasks.push({ diff --git a/lib/validateConfig.js b/lib/validateConfig.js index 0c5b8db68..45908124a 100644 --- a/lib/validateConfig.js +++ b/lib/validateConfig.js @@ -80,21 +80,6 @@ module.exports = function validateConfig(config) { ) ) } - - entries.forEach(([, task]) => { - if (typeof task !== 'function') return - const resolved = task(['[filename]']) - if (typeof resolved === 'string') return - if (!Array.isArray(resolved) || resolved.some(subtask => typeof subtask !== 'string')) { - errors.push( - createError( - task, - 'Function task should return a string or an array of strings', - resolved - ) - ) - } - }) }) } @@ -104,3 +89,5 @@ module.exports = function validateConfig(config) { return config } + +module.exports.createError = createError diff --git a/test/__snapshots__/validateConfig.spec.js.snap b/test/__snapshots__/validateConfig.spec.js.snap index fe5213fd6..98b1b885c 100644 --- a/test/__snapshots__/validateConfig.spec.js.snap +++ b/test/__snapshots__/validateConfig.spec.js.snap @@ -150,15 +150,3 @@ Please refer to https://github.com/okonet/lint-staged#configuration for more inf `; exports[`validateConfig should throw when detecting deprecated advanced configuration 2`] = `""`; - -exports[`validateConfig should throw when function task returns incorrect values 1`] = ` -"● Validation Error: - - Invalid value for 'filenames => filenames.map(file => [\`eslint --fix \${file}\`, \`git add \${file}\`])'. - - Function task should return a string or an array of strings. - - Configured value is: [['eslint --fix [filename]', 'git add [filename]']] - -Please refer to https://github.com/okonet/lint-staged#configuration for more information..." -`; diff --git a/test/makeCmdTasks.spec.js b/test/makeCmdTasks.spec.js index 88d675482..b88f290d4 100644 --- a/test/makeCmdTasks.spec.js +++ b/test/makeCmdTasks.spec.js @@ -61,7 +61,7 @@ describe('makeCmdTasks', () => { it('should work with function task returning a string', async () => { const res = await makeCmdTasks({ commands: () => 'test', gitDir, files: ['test.js'] }) expect(res.length).toBe(1) - expect(res[0].title).toEqual('test') + expect(res[0].title).toEqual('[Function] test ...') }) it('should work with function task returning array of string', async () => { @@ -71,8 +71,8 @@ describe('makeCmdTasks', () => { files: ['test.js'] }) expect(res.length).toBe(2) - expect(res[0].title).toEqual('test') - expect(res[1].title).toEqual('test2') + expect(res[0].title).toEqual('[Function] test ...') + expect(res[1].title).toEqual('[Function] test2 ...') }) it('should work with function task accepting arguments', async () => { @@ -82,8 +82,8 @@ describe('makeCmdTasks', () => { files: ['test.js', 'test2.js'] }) expect(res.length).toBe(2) - expect(res[0].title).toEqual('test [file]') - expect(res[1].title).toEqual('test [file]') + expect(res[0].title).toEqual('[Function] test ...') + expect(res[1].title).toEqual('[Function] test ...') }) it('should work with array of mixed string and function tasks', async () => { @@ -93,26 +93,31 @@ describe('makeCmdTasks', () => { files: ['test.js', 'test2.js', 'test3.js'] }) expect(res.length).toBe(5) - expect(res[0].title).toEqual('test') + expect(res[0].title).toEqual('[Function] test ...') expect(res[1].title).toEqual('test2') - expect(res[2].title).toEqual('test [file]') - expect(res[3].title).toEqual('test [file]') - expect(res[4].title).toEqual('test [file]') - }) - - it('should generate short names for function tasks with long file list', async () => { - const res = await makeCmdTasks({ - commands: filenames => `test ${filenames.map(file => `--file ${file}`).join(' ')}`, - gitDir, - files: Array(100).fill('file.js') // 100 times `file.js` - }) - expect(res.length).toBe(1) - expect(res[0].title).toEqual('test --file [file]') + expect(res[2].title).toEqual('[Function] test ...') + expect(res[3].title).toEqual('[Function] test ...') + expect(res[4].title).toEqual('[Function] test ...') }) it('should work with async function tasks', async () => { const res = await makeCmdTasks({ commands: async () => 'test', gitDir, files: ['test.js'] }) expect(res.length).toBe(1) - expect(res[0].title).toEqual('test') + expect(res[0].title).toEqual('[Function] test ...') + }) + + it("should throw when function task doesn't return string | string[]", async () => { + await expect(makeCmdTasks({ commands: () => null, gitDir, files: ['test.js'] })).rejects + .toThrowErrorMatchingInlineSnapshot(` +"● Validation Error: + + Invalid value for '[Function]'. + + Function task should return a string or an array of strings. + + Configured value is: null + +Please refer to https://github.com/okonet/lint-staged#configuration for more information..." +`) }) }) diff --git a/test/runAll.unmocked.spec.js b/test/runAll.unmocked.spec.js index afff84736..52af3b574 100644 --- a/test/runAll.unmocked.spec.js +++ b/test/runAll.unmocked.spec.js @@ -710,8 +710,8 @@ describe('runAll', () => { LOG Preparing... [completed] LOG Running tasks... [started] LOG Running tasks for *.js [started] - LOG git stash drop [started] - LOG git stash drop [completed] + LOG [Function] git ... [started] + LOG [Function] git ... [completed] LOG Running tasks for *.js [completed] LOG Running tasks... [completed] LOG Applying modifications... [started] diff --git a/test/validateConfig.spec.js b/test/validateConfig.spec.js index eeb6f9365..92c32c793 100644 --- a/test/validateConfig.spec.js +++ b/test/validateConfig.spec.js @@ -49,13 +49,6 @@ describe('validateConfig', () => { expect(console.printHistory()).toMatchSnapshot() }) - it('should throw when function task returns incorrect values', () => { - const invalidConfig = { - '*.js': filenames => filenames.map(file => [`eslint --fix ${file}`, `git add ${file}`]) - } - expect(() => validateConfig(invalidConfig)).toThrowErrorMatchingSnapshot() - }) - it('should throw when detecting deprecated advanced configuration', () => { const advancedConfig = { chunkSize: 10, From f5893365409bf935db058a4f41aeaccc90cd3a18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20J=C3=A4ppinen?= Date: Fri, 21 Feb 2020 09:59:41 +0200 Subject: [PATCH 2/2] fix: do not drop backup stash when reverting to original state fails --- lib/gitWorkflow.js | 1 + lib/runAll.js | 72 ++++++++++++++++++++++++++++++++------------- test/runAll.spec.js | 18 +++++++++++- 3 files changed, 69 insertions(+), 22 deletions(-) diff --git a/lib/gitWorkflow.js b/lib/gitWorkflow.js index c8570f29a..2b39aca67 100644 --- a/lib/gitWorkflow.js +++ b/lib/gitWorkflow.js @@ -264,6 +264,7 @@ class GitWorkflow { // Restore meta information about ongoing git merge await this.restoreMergeStatus() } catch (error) { + ctx.gitRestoreOriginalStateError = true handleError(error, ctx) } } diff --git a/lib/runAll.js b/lib/runAll.js index 8d3a9ae37..17f4fb9d4 100644 --- a/lib/runAll.js +++ b/lib/runAll.js @@ -23,6 +23,40 @@ const getRenderer = ({ debug, quiet }) => { return 'update' } +const MESSAGES = { + TASK_ERROR: 'Skipped because of errors from tasks.', + GIT_ERROR: 'Skipped because of previous git error.' +} + +const shouldSkipApplyModifications = ctx => { + // Should be skipped in case of git errors + if (ctx.gitError) { + return MESSAGES.GIT_ERROR + } + // Should be skipped when tasks fail + if (ctx.taskError) { + return MESSAGES.TASK_ERROR + } +} + +const shouldSkipRevert = ctx => { + // Should be skipped in case of unknown git errors + if (ctx.gitError && !ctx.gitApplyEmptyCommit && !ctx.gitApplyModificationsError) { + return MESSAGES.GIT_ERROR + } +} + +const shouldSkipCleanup = ctx => { + // Should be skipped in case of unknown git errors + if (ctx.gitError && !ctx.gitApplyEmptyCommit && !ctx.gitApplyModificationsError) { + return MESSAGES.GIT_ERROR + } + // Should be skipped when reverting to original state fails + if (ctx.gitRestoreOriginalStateError) { + return MESSAGES.GIT_ERROR + } +} + /** * Executes all tasks and either resolves or rejects the promise * @@ -39,7 +73,7 @@ const getRenderer = ({ debug, quiet }) => { * @param {Logger} logger * @returns {Promise} */ -module.exports = async function runAll( +const runAll = async ( { allowEmpty = false, config, @@ -52,7 +86,7 @@ module.exports = async function runAll( concurrent = true }, logger = console -) { +) => { debugLog('Running all linter scripts') const { gitDir, gitConfigDir } = await resolveGitRepo(cwd) @@ -127,7 +161,7 @@ module.exports = async function runAll( task: () => new Listr(chunkListrTasks, { ...listrOptions, concurrent }), skip: (ctx = {}) => { // Skip if the first step (backup) failed - if (ctx.gitError) return 'Skipped because of previous git error.' + if (ctx.gitError) return MESSAGES.GIT_ERROR // Skip chunk when no every task is skipped (due to no matches) if (chunkListrTasks.every(task => task.skip())) return 'No tasks to run.' return false @@ -151,15 +185,6 @@ module.exports = async function runAll( const git = new GitWorkflow({ allowEmpty, gitConfigDir, gitDir, stagedFileChunks }) - // Running git reset or dropping the backup stash should be skipped - // when there are git errors NOT related to applying unstaged modifications. - // In the latter case, the original state is restored. - const cleanupNotSafe = ctx => - ctx.gitError && - !ctx.gitApplyEmptyCommit && - !ctx.gitApplyModificationsError && - 'Skipped because of previous git error.' - const runner = new Listr( [ { @@ -169,22 +194,19 @@ module.exports = async function runAll( ...listrTasks, { title: 'Applying modifications...', - skip: ctx => { - if (ctx.gitError) return 'Skipped because of previous git error.' - if (ctx.taskError) return 'Skipped because of errors from tasks.' - }, - task: ctx => git.applyModifications(ctx) + task: ctx => git.applyModifications(ctx), + skip: shouldSkipApplyModifications }, { title: 'Reverting to original state...', + task: ctx => git.restoreOriginalState(ctx), enabled: ctx => ctx.taskError || ctx.gitApplyEmptyCommit || ctx.gitApplyModificationsError, - skip: cleanupNotSafe, - task: ctx => git.restoreOriginalState(ctx) + skip: shouldSkipRevert }, { title: 'Cleaning up...', - skip: cleanupNotSafe, - task: ctx => git.dropBackup(ctx) + task: ctx => git.dropBackup(ctx), + skip: shouldSkipCleanup } ], listrOptions @@ -219,3 +241,11 @@ module.exports = async function runAll( throw error } } + +module.exports = runAll + +module.exports.shouldSkip = { + shouldSkipApplyModifications, + shouldSkipRevert, + shouldSkipCleanup +} diff --git a/test/runAll.spec.js b/test/runAll.spec.js index 7a89aebdf..f19b7562a 100644 --- a/test/runAll.spec.js +++ b/test/runAll.spec.js @@ -5,7 +5,7 @@ import path from 'path' import resolveGitRepo from '../lib/resolveGitRepo' import getStagedFiles from '../lib/getStagedFiles' -import runAll from '../lib/runAll' +import runAll, { shouldSkip } from '../lib/runAll' jest.mock('../lib/resolveGitRepo') jest.mock('../lib/getStagedFiles') @@ -100,3 +100,19 @@ describe('runAll', () => { expect(console.printHistory()).toMatchSnapshot() }) }) + +describe('shouldSkip', () => { + describe('shouldSkipRevert', () => { + it('should return error message when there is an unkown git error', () => { + const result = shouldSkip.shouldSkipRevert({ gitError: true }) + expect(typeof result === 'string').toEqual(true) + }) + }) + + describe('shouldSkipCleanup', () => { + it('should return error message when reverting to original state fails', () => { + const result = shouldSkip.shouldSkipCleanup({ gitRestoreOriginalStateError: true }) + expect(typeof result === 'string').toEqual(true) + }) + }) +})