From fea80331c768b3642e90fc687e5aceaa419d2b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iiro=20J=C3=A4ppinen?= Date: Thu, 22 Jul 2021 16:13:43 +0300 Subject: [PATCH] feat: allow a path to be supplied to the --shell option (#994) --- README.md | 4 +- bin/lint-staged.js | 4 +- lib/index.js | 40 ++++-- lib/messages.js | 23 +++- lib/symbols.js | 4 + lib/validateOptions.js | 31 +++++ test/__snapshots__/index.spec.js.snap | 81 ------------ test/index.spec.js | 179 +++++++++++++++++++------- test/resolveTaskFn.spec.js | 17 +++ test/validateOptions.spec.js | 65 ++++++++++ 10 files changed, 301 insertions(+), 147 deletions(-) create mode 100644 lib/validateOptions.js delete mode 100644 test/__snapshots__/index.spec.js.snap create mode 100644 test/validateOptions.spec.js diff --git a/README.md b/README.md index 449828b0a..59dee6c92 100644 --- a/README.md +++ b/README.md @@ -71,7 +71,7 @@ Options: tasks serially (default: true) -q, --quiet disable lint-staged’s own console output (default: false) -r, --relative pass relative filepaths to tasks (default: false) - -x, --shell skip parsing of tasks for better shell support (default: + -x, --shell skip parsing of tasks for better shell support (default: false) -v, --verbose show task output even when tasks succeed; by default only failed output is shown (default: false) @@ -90,7 +90,7 @@ Options: - **`--no-stash`**: By default a backup stash will be created before running the tasks, and all task modifications will be reverted in case of an error. This option will disable creating the stash, and instead leave all modifications in the index when aborting the commit. - **`--quiet`**: Supress all CLI output, except from tasks. - **`--relative`**: Pass filepaths relative to `process.cwd()` (where `lint-staged` runs) to tasks. Default is `false`. -- **`--shell`**: By default linter commands will be parsed for speed and security. This has the side-effect that regular shell scripts might not work as expected. You can skip parsing of commands with this option. +- **`--shell`**: By default linter commands will be parsed for speed and security. This has the side-effect that regular shell scripts might not work as expected. You can skip parsing of commands with this option. To use a specific shell, use a path like `--shell "/bin/bash"`. - **`--verbose`**: Show task output even when tasks succeed. By default only failed output is shown. ## Configuration diff --git a/bin/lint-staged.js b/bin/lint-staged.js index 28c3a692b..0b1724c17 100755 --- a/bin/lint-staged.js +++ b/bin/lint-staged.js @@ -42,7 +42,7 @@ cmdline ) .option('-q, --quiet', 'disable lint-staged’s own console output', false) .option('-r, --relative', 'pass relative filepaths to tasks', false) - .option('-x, --shell', 'skip parsing of tasks for better shell support', false) + .option('-x, --shell ', 'skip parsing of tasks for better shell support', false) .option( '-v, --verbose', 'show task output even when tasks succeed; by default only failed output is shown', @@ -85,7 +85,7 @@ const options = { stash: !!cmdlineOptions.stash, // commander inverts `no-` flags to `!x` quiet: !!cmdlineOptions.quiet, relative: !!cmdlineOptions.relative, - shell: !!cmdlineOptions.shell, + shell: cmdlineOptions.shell /* Either a boolean or a string pointing to the shell */, verbose: !!cmdlineOptions.verbose, } diff --git a/lib/index.js b/lib/index.js index f1dd0ddc7..a1907eff5 100644 --- a/lib/index.js +++ b/lib/index.js @@ -8,13 +8,18 @@ const stringifyObject = require('stringify-object') const { PREVENTED_EMPTY_COMMIT, GIT_ERROR, RESTORE_STASH_EXAMPLE } = require('./messages') const printTaskOutput = require('./printTaskOutput') const runAll = require('./runAll') -const { ApplyEmptyCommitError, GetBackupStashError, GitError } = require('./symbols') +const { + ApplyEmptyCommitError, + ConfigNotFoundError, + GetBackupStashError, + GitError, + InvalidOptionsError, +} = require('./symbols') const formatConfig = require('./formatConfig') const validateConfig = require('./validateConfig') +const validateOptions = require('./validateOptions') -const errConfigNotFound = new Error('Config could not be found') - -function resolveConfig(configPath) { +const resolveConfig = (configPath) => { try { return require.resolve(configPath) } catch { @@ -22,7 +27,7 @@ function resolveConfig(configPath) { } } -function loadConfig(configPath) { +const loadConfig = (configPath) => { const explorer = cosmiconfig('lint-staged', { searchPlaces: [ 'package.json', @@ -56,14 +61,14 @@ function loadConfig(configPath) { * @param {number} [options.maxArgLength] - Maximum argument string length * @param {boolean} [options.quiet] - Disable lint-staged’s own console output * @param {boolean} [options.relative] - Pass relative filepaths to tasks - * @param {boolean} [options.shell] - Skip parsing of tasks for better shell support + * @param {boolean|string} [options.shell] - Skip parsing of tasks for better shell support * @param {boolean} [options.stash] - Enable the backup stash, and revert in case of errors * @param {boolean} [options.verbose] - Show task output even when tasks succeed; by default only failed output is shown * @param {Logger} [logger] * * @returns {Promise} Promise of whether the linting passed or failed */ -module.exports = async function lintStaged( +const lintStaged = async ( { allowEmpty = false, concurrent = true, @@ -79,20 +84,27 @@ module.exports = async function lintStaged( verbose = false, } = {}, logger = console -) { +) => { try { + await validateOptions({ shell }, logger) + debugLog('Loading config using `cosmiconfig`') const resolved = configObject ? { config: configObject, filepath: '(input)' } : await loadConfig(configPath) - if (resolved == null) throw errConfigNotFound + + if (resolved == null) { + throw ConfigNotFoundError + } debugLog('Successfully loaded config from `%s`:\n%O', resolved.filepath, resolved.config) + // resolved.config is the parsed configuration object // resolved.filepath is the path to the config file that was found const formattedConfig = formatConfig(resolved.config) const config = validateConfig(formattedConfig) + if (debug) { // Log using logger to be able to test through `consolemock`. logger.log('Running lint-staged with the following config:') @@ -148,7 +160,13 @@ module.exports = async function lintStaged( throw runAllError } } catch (lintStagedError) { - if (lintStagedError === errConfigNotFound) { + /** throw early because `validateOptions` options contains own logging */ + if (lintStagedError === InvalidOptionsError) { + throw InvalidOptionsError + } + + /** @todo move logging to `validateConfig` and remove this try/catch block */ + if (lintStagedError === ConfigNotFoundError) { logger.error(`${lintStagedError.message}.`) } else { // It was probably a parsing error @@ -168,3 +186,5 @@ module.exports = async function lintStaged( throw lintStagedError } } + +module.exports = lintStaged diff --git a/lib/messages.js b/lib/messages.js index 13d001434..aa5b5b265 100644 --- a/lib/messages.js +++ b/lib/messages.js @@ -27,6 +27,14 @@ const SKIPPED_GIT_ERROR = 'Skipped because of previous git error.' const GIT_ERROR = `\n ${error} ${chalk.red(`lint-staged failed due to a git error.`)}` +const invalidOption = (name, value, message) => `${chalk.redBright(`${error} Validation Error:`)} + + Invalid value for option ${chalk.bold(name)}: ${chalk.bold(value)} + + ${message} + +See https://github.com/okonet/lint-staged#command-line-flags` + const PREVENTED_EMPTY_COMMIT = ` ${warning} ${chalk.yellow(`lint-staged prevented an empty git commit. Use the --allow-empty option to continue, or check your task configuration`)} @@ -42,16 +50,17 @@ const RESTORE_STASH_EXAMPLE = ` Any lost modifications can be restored from a g const CONFIG_STDIN_ERROR = 'Error: Could not read config from stdin.' module.exports = { - NOT_GIT_REPO, + CONFIG_STDIN_ERROR, + DEPRECATED_GIT_ADD, FAILED_GET_STAGED_FILES, + GIT_ERROR, + invalidOption, NO_STAGED_FILES, NO_TASKS, - skippingBackup, - DEPRECATED_GIT_ADD, - TASK_ERROR, - SKIPPED_GIT_ERROR, - GIT_ERROR, + NOT_GIT_REPO, PREVENTED_EMPTY_COMMIT, RESTORE_STASH_EXAMPLE, - CONFIG_STDIN_ERROR, + SKIPPED_GIT_ERROR, + skippingBackup, + TASK_ERROR, } diff --git a/lib/symbols.js b/lib/symbols.js index ea124b975..f2acda60e 100644 --- a/lib/symbols.js +++ b/lib/symbols.js @@ -1,11 +1,13 @@ 'use strict' const ApplyEmptyCommitError = Symbol('ApplyEmptyCommitError') +const ConfigNotFoundError = new Error('Config could not be found') const GetBackupStashError = Symbol('GetBackupStashError') const GetStagedFilesError = Symbol('GetStagedFilesError') const GitError = Symbol('GitError') const GitRepoError = Symbol('GitRepoError') const HideUnstagedChangesError = Symbol('HideUnstagedChangesError') +const InvalidOptionsError = new Error('Invalid Options') const RestoreMergeStatusError = Symbol('RestoreMergeStatusError') const RestoreOriginalStateError = Symbol('RestoreOriginalStateError') const RestoreUnstagedChangesError = Symbol('RestoreUnstagedChangesError') @@ -13,10 +15,12 @@ const TaskError = Symbol('TaskError') module.exports = { ApplyEmptyCommitError, + ConfigNotFoundError, GetBackupStashError, GetStagedFilesError, GitError, GitRepoError, + InvalidOptionsError, HideUnstagedChangesError, RestoreMergeStatusError, RestoreOriginalStateError, diff --git a/lib/validateOptions.js b/lib/validateOptions.js new file mode 100644 index 000000000..d0ab724f2 --- /dev/null +++ b/lib/validateOptions.js @@ -0,0 +1,31 @@ +const { promises: fs, constants } = require('fs') + +const { invalidOption } = require('./messages') +const { InvalidOptionsError } = require('./symbols') + +const debug = require('debug')('lint-staged:options') + +/** + * Validate lint-staged options, either from the Node.js API or the command line flags. + * @param {*} options + * @param {boolean|string} [options.shell] - Skip parsing of tasks for better shell support + * + * @throws {InvalidOptionsError} + */ +const validateOptions = async (options = {}, logger) => { + debug('Validating options...') + + /** Ensure the passed shell option is executable */ + if (typeof options.shell === 'string') { + try { + await fs.access(options.shell, constants.X_OK) + } catch (error) { + logger.error(invalidOption('shell', options.shell, error.message)) + throw InvalidOptionsError + } + } + + debug('Validated options!') +} + +module.exports = validateOptions diff --git a/test/__snapshots__/index.spec.js.snap b/test/__snapshots__/index.spec.js.snap deleted file mode 100644 index fab8a387c..000000000 --- a/test/__snapshots__/index.spec.js.snap +++ /dev/null @@ -1,81 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`lintStaged should load an npm config package when specified 1`] = ` -" -LOG Running lint-staged with the following config: -LOG { - '*': 'mytask' -}" -`; - -exports[`lintStaged should load config file when specified 1`] = ` -" -LOG Running lint-staged with the following config: -LOG { - '*': 'mytask' -}" -`; - -exports[`lintStaged should not output config in normal mode 1`] = `""`; - -exports[`lintStaged should output config in debug mode 1`] = ` -" -LOG Running lint-staged with the following config: -LOG { - '*': 'mytask' -}" -`; - -exports[`lintStaged should parse function linter from js config 1`] = ` -" -LOG Running lint-staged with the following config: -LOG { - '*.css': filenames => \`echo \${filenames.join(' ')}\`, - '*.js': filenames => filenames.map(filename => \`echo \${filename}\`) -}" -`; - -exports[`lintStaged should print helpful error message when config file is not found 2`] = ` -" -ERROR Config could not be found. -ERROR -ERROR Please make sure you have created it correctly. -See https://github.com/okonet/lint-staged#configuration." -`; - -exports[`lintStaged should print helpful error message when explicit config file is not found 1`] = ` - -ERROR Could not parse lint-staged config. - -Error: ENOENT: no such file or directory, open 'fake-config-file.yml' -ERROR -ERROR Please make sure you have created it correctly. -See https://github.com/okonet/lint-staged#configuration. -`; - -exports[`lintStaged should throw when invalid config is provided 2`] = ` -" -ERROR Could not parse lint-staged config. - -Error: Configuration should not be empty! -ERROR -ERROR Please make sure you have created it correctly. -See https://github.com/okonet/lint-staged#configuration." -`; - -exports[`lintStaged should use config object 1`] = `""`; - -exports[`lintStaged should use cosmiconfig if no params are passed 1`] = ` -" -ERROR × Failed to get staged files!" -`; - -exports[`lintStaged should use use the console if no logger is passed 1`] = ` -" -ERROR Could not parse lint-staged config. - -Error: Configuration should not be empty! -ERROR -ERROR Please make sure you have created it correctly. -See https://github.com/okonet/lint-staged#configuration." -`; diff --git a/test/index.spec.js b/test/index.spec.js index 1233ef1b2..6a98a629a 100644 --- a/test/index.spec.js +++ b/test/index.spec.js @@ -8,27 +8,21 @@ jest.unmock('execa') import getStagedFiles from '../lib/getStagedFiles' // eslint-disable-next-line import/first import lintStaged from '../lib/index' +// eslint-disable-next-line import/first +import { InvalidOptionsError } from '../lib/symbols' +// eslint-disable-next-line import/first +import validateOptions from '../lib/validateOptions' import { replaceSerializer } from './utils/replaceSerializer' -jest.mock('../lib/getStagedFiles') - const mockCosmiconfigWith = (result) => { cosmiconfig.mockImplementationOnce(() => ({ search: () => Promise.resolve(result), })) } +jest.mock('../lib/getStagedFiles') jest.mock('../lib/gitWorkflow') - -async function withMockedConsole(mockConsole, fn) { - const previousConsole = console - try { - console = mockConsole - await fn() - } finally { - console = previousConsole - } -} +jest.mock('../lib/validateOptions', () => jest.fn().mockImplementation(async () => {})) // TODO: Never run tests in the project's WC because this might change source files git status @@ -41,67 +35,118 @@ describe('lintStaged', () => { it('should use cosmiconfig if no params are passed', async () => { expect.assertions(1) - const config = { - '*': 'mytask', - } + + const config = { '*': 'mytask' } mockCosmiconfigWith({ config }) + await lintStaged(undefined, logger) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + ERROR × Failed to get staged files!" + `) }) it('should return true when passed', async () => { expect.assertions(1) - const config = { - '*': 'node -e "process.exit(0)"', - } + getStagedFiles.mockImplementationOnce(async () => ['sample.java']) - const passed = await lintStaged({ config, quiet: true }, logger) - expect(passed).toEqual(true) + + const config = { '*': 'node -e "process.exit(0)"' } + + await expect(lintStaged({ config, quiet: true }, logger)).resolves.toEqual(true) }) it('should use use the console if no logger is passed', async () => { - expect.assertions(1) + expect.assertions(2) + mockCosmiconfigWith({ config: {} }) + const previousConsole = console const mockedConsole = makeConsoleMock() - try { - await withMockedConsole(mockedConsole, () => lintStaged()) - } catch (ignore) { - expect(mockedConsole.printHistory()).toMatchSnapshot() - } + console = mockedConsole + + await expect(lintStaged()).rejects.toMatchInlineSnapshot( + `[Error: Configuration should not be empty!]` + ) + + expect(mockedConsole.printHistory()).toMatchInlineSnapshot(` + " + ERROR Could not parse lint-staged config. + + Error: Configuration should not be empty! + ERROR + ERROR Please make sure you have created it correctly. + See https://github.com/okonet/lint-staged#configuration." + `) + + console = previousConsole }) it('should output config in debug mode', async () => { expect.assertions(1) - const config = { - '*': 'mytask', - } + + const config = { '*': 'mytask' } mockCosmiconfigWith({ config }) + await lintStaged({ debug: true, quiet: true }, logger) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + LOG Running lint-staged with the following config: + LOG { + '*': 'mytask' + }" + `) }) it('should not output config in normal mode', async () => { expect.assertions(1) - const config = { - '*': 'mytask', - } + + const config = { '*': 'mytask' } mockCosmiconfigWith({ config }) + await lintStaged({ quiet: true }, logger) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(`""`) + }) + + it('should throw when invalid options are provided', async () => { + expect.assertions(2) + + validateOptions.mockImplementationOnce(async () => { + throw InvalidOptionsError + }) + + await expect(lintStaged({ '*': 'mytask' }, logger)).rejects.toMatchInlineSnapshot( + `[Error: Invalid Options]` + ) + + expect(logger.printHistory()).toMatchInlineSnapshot(`""`) }) it('should throw when invalid config is provided', async () => { const config = {} mockCosmiconfigWith({ config }) + await expect(lintStaged({ quiet: true }, logger)).rejects.toMatchInlineSnapshot( `[Error: Configuration should not be empty!]` ) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + ERROR Could not parse lint-staged config. + + Error: Configuration should not be empty! + ERROR + ERROR Please make sure you have created it correctly. + See https://github.com/okonet/lint-staged#configuration." + `) }) it('should load config file when specified', async () => { expect.assertions(1) + await lintStaged( { configPath: path.join(__dirname, '__mocks__', 'my-config.json'), @@ -110,11 +155,19 @@ describe('lintStaged', () => { }, logger ) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + LOG Running lint-staged with the following config: + LOG { + '*': 'mytask' + }" + `) }) it('should parse function linter from js config', async () => { expect.assertions(1) + await lintStaged( { configPath: path.join(__dirname, '__mocks__', 'advanced-config.js'), @@ -123,36 +176,64 @@ describe('lintStaged', () => { }, logger ) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + LOG Running lint-staged with the following config: + LOG { + '*.css': filenames => \`echo \${filenames.join(' ')}\`, + '*.js': filenames => filenames.map(filename => \`echo \${filename}\`) + }" + `) }) it('should use config object', async () => { - const config = { - '*': 'node -e "process.exit(1)"', - } expect.assertions(1) + + const config = { '*': 'node -e "process.exit(1)"' } + await lintStaged({ config, quiet: true }, logger) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(`""`) }) it('should load an npm config package when specified', async () => { expect.assertions(1) + jest.mock('my-lint-staged-config') + await lintStaged({ configPath: 'my-lint-staged-config', quiet: true, debug: true }, logger) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + LOG Running lint-staged with the following config: + LOG { + '*': 'mytask' + }" + `) }) it('should print helpful error message when config file is not found', async () => { expect.assertions(2) + mockCosmiconfigWith(null) + await expect(lintStaged({ quiet: true }, logger)).rejects.toMatchInlineSnapshot( `[Error: Config could not be found]` ) - expect(logger.printHistory()).toMatchSnapshot() + + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + ERROR Config could not be found. + ERROR + ERROR Please make sure you have created it correctly. + See https://github.com/okonet/lint-staged#configuration." + `) }) it('should print helpful error message when explicit config file is not found', async () => { expect.assertions(2) + const nonExistentConfig = 'fake-config-file.yml' // Serialize Windows, Linux and MacOS paths consistently @@ -167,6 +248,14 @@ describe('lintStaged', () => { lintStaged({ configPath: nonExistentConfig, quiet: true }, logger) ).rejects.toThrowError() - expect(logger.printHistory()).toMatchSnapshot() + expect(logger.printHistory()).toMatchInlineSnapshot(` + + ERROR Could not parse lint-staged config. + + Error: ENOENT: no such file or directory, open 'fake-config-file.yml' + ERROR + ERROR Please make sure you have created it correctly. + See https://github.com/okonet/lint-staged#configuration. + `) }) }) diff --git a/test/resolveTaskFn.spec.js b/test/resolveTaskFn.spec.js index 82b077655..3a98a9384 100644 --- a/test/resolveTaskFn.spec.js +++ b/test/resolveTaskFn.spec.js @@ -79,6 +79,23 @@ describe('resolveTaskFn', () => { }) }) + it('should work with path to custom shell', async () => { + expect.assertions(2) + const taskFn = resolveTaskFn({ + ...defaultOpts, + shell: '/bin/bash', + command: 'node --arg=true ./myscript.js', + }) + + await taskFn() + expect(execa).toHaveBeenCalledTimes(1) + expect(execa).lastCalledWith('node --arg=true ./myscript.js test.js', { + preferLocal: true, + reject: false, + shell: '/bin/bash', + }) + }) + it('should pass `gitDir` as `cwd` to `execa()` gitDir !== process.cwd for git commands', async () => { expect.assertions(2) const taskFn = resolveTaskFn({ diff --git a/test/validateOptions.spec.js b/test/validateOptions.spec.js new file mode 100644 index 000000000..a888e4d8e --- /dev/null +++ b/test/validateOptions.spec.js @@ -0,0 +1,65 @@ +import makeConsoleMock from 'consolemock' +import { promises as fs, constants } from 'fs' + +import validateOptions from '../lib/validateOptions' +import { InvalidOptionsError } from '../lib/symbols' + +describe('validateOptions', () => { + const mockAccess = jest.spyOn(fs, 'access') + mockAccess.mockImplementation(async () => {}) + + beforeEach(() => { + mockAccess.mockClear() + }) + + it('should resolve empty and missing config', async () => { + expect.assertions(3) + + const logger = makeConsoleMock() + + await expect(validateOptions({}, logger)).resolves.toBeUndefined() + await expect(validateOptions(undefined, logger)).resolves.toBeUndefined() + + expect(logger.history()).toHaveLength(0) + }) + + it('should resolve with valid string-valued shell option', async () => { + expect.assertions(4) + + const logger = makeConsoleMock() + + await expect(validateOptions({ shell: '/bin/sh' }, logger)).resolves.toBeUndefined() + + expect(mockAccess).toHaveBeenCalledTimes(1) + expect(mockAccess).toHaveBeenCalledWith('/bin/sh', constants.X_OK) + + expect(logger.history()).toHaveLength(0) + }) + + it('should reject with invalid string-valued shell option', async () => { + expect.assertions(5) + + const logger = makeConsoleMock() + + mockAccess.mockImplementationOnce(() => new Promise.reject()) + + await expect(validateOptions({ shell: '/bin/sh' }, logger)).rejects.toThrowError( + InvalidOptionsError + ) + + expect(mockAccess).toHaveBeenCalledTimes(1) + expect(mockAccess).toHaveBeenCalledWith('/bin/sh', constants.X_OK) + + expect(logger.history()).toHaveLength(1) + expect(logger.printHistory()).toMatchInlineSnapshot(` + " + ERROR × Validation Error: + + Invalid value for option shell: /bin/sh + + Promise.reject is not a constructor + + See https://github.com/okonet/lint-staged#command-line-flags" + `) + }) +})