From 649c1c28dae535a47890d8f66da90314e3fffe4c Mon Sep 17 00:00:00 2001 From: Kumar McMillan Date: Thu, 21 Dec 2017 16:13:12 -0600 Subject: [PATCH] feat: Discover config files in home dir, working dir (#1161) --- src/config.js | 68 +++++++- src/program.js | 63 +++++-- src/util/file-exists.js | 37 ++++ tests/unit/helpers.js | 7 +- tests/unit/test-util/test.artifacts.js | 2 +- tests/unit/test-util/test.file-exists.js | 60 +++++++ tests/unit/test.config.js | 150 +++++++++++++++- tests/unit/test.program.js | 212 +++++++++++++++++++++-- 8 files changed, 557 insertions(+), 42 deletions(-) create mode 100644 src/util/file-exists.js create mode 100644 tests/unit/test-util/test.file-exists.js diff --git a/src/config.js b/src/config.js index a615bea3cc..ca5f572d7b 100644 --- a/src/config.js +++ b/src/config.js @@ -1,17 +1,23 @@ /* @flow */ +import os from 'os'; import path from 'path'; import requireUncached from 'require-uncached'; import camelCase from 'camelcase'; import decamelize from 'decamelize'; +import fileExists from './util/file-exists'; import {createLogger} from './util/logger'; import {UsageError, WebExtError} from './errors'; const log = createLogger(__filename); type ApplyConfigToArgvParams = {| + // This is the argv object which will get updated by each + // config applied. argv: Object, + // This is the argv that only has CLI values applied to it. + argvFromCLI: Object, configObject: Object, options: Object, configFileName: string, @@ -19,6 +25,7 @@ type ApplyConfigToArgvParams = {| export function applyConfigToArgv({ argv, + argvFromCLI, configObject, options, configFileName, @@ -26,9 +33,9 @@ export function applyConfigToArgv({ let newArgv = {...argv}; for (const option in configObject) { - if (camelCase(option) !== option) { - throw new UsageError(`The config option "${option}" must be ` + + throw new UsageError( + `The config option "${option}" must be ` + `specified in camel case: "${camelCase(option)}"`); } @@ -37,6 +44,7 @@ export function applyConfigToArgv({ // Descend into the nested configuration for a sub-command. newArgv = applyConfigToArgv({ argv: newArgv, + argvFromCLI, configObject: configObject[option], options: options[option], configFileName}); @@ -74,15 +82,19 @@ export function applyConfigToArgv({ } } - // we assume the value was set on the CLI if the default value is - // not the same as that on the argv object as there is a very rare chance - // of this happening + // This is our best effort (without patching yargs) to detect + // if a value was set on the CLI instead of in the config. + // It looks for a default value and if the argv value is + // different, it assumes that the value was configured on the CLI. - const wasValueSetOnCLI = typeof(argv[option]) !== 'undefined' && - (argv[option] !== defaultValue); + const wasValueSetOnCLI = + typeof argvFromCLI[option] !== 'undefined' && + argvFromCLI[option] !== defaultValue; if (wasValueSetOnCLI) { - log.debug(`Favoring CLI: ${option}=${argv[option]} over ` + + log.debug( + `Favoring CLI: ${option}=${argvFromCLI[option]} over ` + `configuration: ${option}=${configObject[option]}`); + newArgv[option] = argvFromCLI[option]; continue; } @@ -118,3 +130,43 @@ export function loadJSConfigFile(filePath: string): Object { } return configObject; } + +type DiscoverConfigFilesParams = {| + getHomeDir: () => string, +|}; + +export async function discoverConfigFiles( + {getHomeDir = os.homedir}: DiscoverConfigFilesParams = {} +): Promise> { + const magicConfigName = 'web-ext-config.js'; + + // Config files will be loaded in this order. + const possibleConfigs = [ + // Look for a magic hidden config (preceded by dot) in home dir. + path.join(getHomeDir(), `.${magicConfigName}`), + // Look for a magic config in the current working directory. + path.join(process.cwd(), magicConfigName), + ]; + + const configs = await Promise.all(possibleConfigs.map( + async (fileName) => { + const resolvedFileName = path.resolve(fileName); + if (await fileExists(resolvedFileName)) { + return resolvedFileName; + } else { + log.debug( + `Discovered config "${resolvedFileName}" does not ` + + 'exist or is not readable'); + return undefined; + } + } + )); + + const existingConfigs = []; + configs.forEach((f) => { + if (typeof f === 'string') { + existingConfigs.push(f); + } + }); + return existingConfigs; +} diff --git a/src/program.js b/src/program.js index f895c8ea56..3cddefbbf1 100644 --- a/src/program.js +++ b/src/program.js @@ -1,4 +1,5 @@ /* @flow */ +import os from 'os'; import path from 'path'; import {readFileSync} from 'fs'; @@ -12,6 +13,7 @@ import {createLogger, consoleStream as defaultLogStream} from './util/logger'; import {coerceCLICustomPreference} from './firefox/preferences'; import {checkForUpdates as defaultUpdateChecker} from './util/updates'; import { + discoverConfigFiles as defaultConfigDiscovery, loadJSConfigFile as defaultLoadJSConfigFile, applyConfigToArgv as defaultApplyConfigToArgv, } from './config'; @@ -34,6 +36,7 @@ type ExecuteOptions = { logStream?: typeof defaultLogStream, getVersion?: VersionGetterFn, applyConfigToArgv?: typeof defaultApplyConfigToArgv, + discoverConfigFiles?: typeof defaultConfigDiscovery, loadJSConfigFile?: typeof defaultLoadJSConfigFile, shouldExitProgram?: boolean, globalEnv?: string, @@ -121,11 +124,15 @@ export class Program { async execute( absolutePackageDir: string, { - checkForUpdates = defaultUpdateChecker, systemProcess = process, - logStream = defaultLogStream, getVersion = defaultVersionGetter, + checkForUpdates = defaultUpdateChecker, + systemProcess = process, + logStream = defaultLogStream, + getVersion = defaultVersionGetter, applyConfigToArgv = defaultApplyConfigToArgv, + discoverConfigFiles = defaultConfigDiscovery, loadJSConfigFile = defaultLoadJSConfigFile, - shouldExitProgram = true, globalEnv = WEBEXT_BUILD_ENV, + shouldExitProgram = true, + globalEnv = WEBEXT_BUILD_ENV, }: ExecuteOptions = {} ): Promise { @@ -155,19 +162,46 @@ export class Program { }); } - let argvFromConfig = {...argv}; + let adjustedArgv = {...argv}; + const configFiles = []; + + if (argv.configDiscovery) { + log.debug( + 'Discovering config files. ' + + 'Set --no-config-discovery to disable'); + const discoveredConfigs = await discoverConfigFiles(); + configFiles.push(...discoveredConfigs); + } else { + log.debug('Not discovering config files'); + } + if (argv.config) { - const configFileName = path.resolve(argv.config); + configFiles.push(path.resolve(argv.config)); + } + + if (configFiles.length) { + const niceFileList = configFiles + .map((f) => f.replace(process.cwd(), '.')) + .map((f) => f.replace(os.homedir(), '~')) + .join(', '); + log.info( + 'Applying config file' + + `${configFiles.length !== 1 ? 's' : ''}: ` + + `${niceFileList}`); + } + + configFiles.forEach((configFileName) => { const configObject = loadJSConfigFile(configFileName); - argvFromConfig = applyConfigToArgv({ - argv, + adjustedArgv = applyConfigToArgv({ + argv: adjustedArgv, + argvFromCLI: argv, configFileName, configObject, options: this.options, }); - } + }); - await runCommand(argvFromConfig, {shouldExitProgram}); + await runCommand(adjustedArgv, {shouldExitProgram}); } catch (error) { if (!(error instanceof UsageError) || argv.verbose) { @@ -289,11 +323,20 @@ Example: $0 --help run. }, 'config': { alias: 'c', - describe: 'Path to the config file', + describe: 'Path to a CommonJS config file to set ' + + 'option defaults', default: undefined, + demand: false, requiresArg: true, type: 'string', }, + 'config-discovery': { + describe: 'Discover config files in home directory and ' + + 'working directory. Disable with --no-config-discovery.', + demand: false, + default: true, + type: 'boolean', + }, }); program diff --git a/src/util/file-exists.js b/src/util/file-exists.js new file mode 100644 index 0000000000..eaf441ddfc --- /dev/null +++ b/src/util/file-exists.js @@ -0,0 +1,37 @@ +/* @flow */ +import {fs} from 'mz'; + +import {isErrorWithCode} from '../errors'; + +type FileExistsOptions = {| + fileIsReadable: (filePath: string) => Promise, +|}; + +/* + * Resolves true if the path is a readable file. + * + * Usage: + * + * const exists = await fileExists(filePath); + * if (exists) { + * // ... + * } + * + * */ +export default async function fileExists( + path: string, + { + fileIsReadable = (f) => fs.access(f, fs.constants.R_OK), + }: FileExistsOptions = {} +): Promise { + try { + await fileIsReadable(path); + const stat = await fs.stat(path); + return stat.isFile(); + } catch (error) { + if (isErrorWithCode(['EACCES', 'ENOENT'], error)) { + return false; + } + throw error; + } +} diff --git a/tests/unit/helpers.js b/tests/unit/helpers.js index 9cbb27aadb..73e6cf62f4 100644 --- a/tests/unit/helpers.js +++ b/tests/unit/helpers.js @@ -110,9 +110,8 @@ export function fixturePath(...pathParts: Array): string { * * Usage: * - * Promise.resolve() - * .then(makeSureItFails()) - * .catch((error) => { + * Promise.reject(new Error('some error')) + * .then(makeSureItFails(), (error) => { * // Safely make assertions about the error... * }); */ @@ -272,7 +271,7 @@ export class TCPConnectError extends ExtendableError { export class ErrorWithCode extends Error { code: string; constructor(code: ?string, message: ?string) { - super(message || 'pretend this is a system error'); + super(`${code || ''}: ${message || 'pretend this is a system error'}`); this.code = code || 'SOME_CODE'; } } diff --git a/tests/unit/test-util/test.artifacts.js b/tests/unit/test-util/test.artifacts.js index f25fdf9601..9dbccd574d 100644 --- a/tests/unit/test-util/test.artifacts.js +++ b/tests/unit/test-util/test.artifacts.js @@ -142,7 +142,7 @@ describe('prepareArtifactsDir', () => { return prepareArtifactsDir(tmpPath, {asyncMkdirp: fakeAsyncMkdirp}) .then(makeSureItFails(), (error) => { sinon.assert.called(fakeAsyncMkdirp); - assert.equal(error.message, 'an error'); + assert.equal(error.message, 'ENOSPC: an error'); }); } )); diff --git a/tests/unit/test-util/test.file-exists.js b/tests/unit/test-util/test.file-exists.js new file mode 100644 index 0000000000..c485b0df42 --- /dev/null +++ b/tests/unit/test-util/test.file-exists.js @@ -0,0 +1,60 @@ +/* @flow */ +import path from 'path'; + +import {assert} from 'chai'; +import {describe, it} from 'mocha'; +import {fs} from 'mz'; + +import fileExists from '../../../src/util/file-exists'; +import {withTempDir} from '../../../src/util/temp-dir'; +import {makeSureItFails, ErrorWithCode} from '../helpers'; + + +describe('util/file-exists', () => { + it('returns true for existing files', () => { + return withTempDir( + async (tmpDir) => { + const someFile = path.join(tmpDir.path(), 'file.txt'); + await fs.writeFile(someFile, ''); + + assert.equal(await fileExists(someFile), true); + }); + }); + + it('returns false for non-existent files', () => { + return withTempDir( + async (tmpDir) => { + // This file does not exist. + const someFile = path.join(tmpDir.path(), 'file.txt'); + + assert.equal(await fileExists(someFile), false); + }); + }); + + it('returns false for directories', () => { + return withTempDir( + async (tmpDir) => { + assert.equal(await fileExists(tmpDir.path()), false); + }); + }); + + it('returns false for unreadable files', async () => { + const exists = await fileExists('pretend/unreadable/file', { + fileIsReadable: async () => { + throw new ErrorWithCode('EACCES', 'permission denied'); + }, + }); + assert.equal(exists, false); + }); + + it('throws unexpected errors', async () => { + const exists = fileExists('pretend/file', { + fileIsReadable: async () => { + throw new ErrorWithCode('EBUSY', 'device is busy'); + }, + }); + await exists.then(makeSureItFails(), (error) => { + assert.equal(error.message, 'EBUSY: device is busy'); + }); + }); +}); diff --git a/tests/unit/test.config.js b/tests/unit/test.config.js index bab9872b37..7101e6091c 100644 --- a/tests/unit/test.config.js +++ b/tests/unit/test.config.js @@ -9,6 +9,7 @@ import {fs} from 'mz'; import {Program} from '../../src/program'; import { applyConfigToArgv, + discoverConfigFiles, loadJSConfigFile, } from '../../src/config'; import {withTempDir} from '../../src/util/temp-dir'; @@ -39,8 +40,11 @@ function makeArgv({ if (commandOpt) { program.command(command, commandDesc, commandExecutor, commandOpt); } + + const argv = program.yargs.exitProcess(false).argv; return { - argv: program.yargs.exitProcess(false).argv, + argv, + argvFromCLI: argv, options: program.options, }; } @@ -245,6 +249,34 @@ describe('config', () => { assert.strictEqual(newArgv.overwriteFiles, true); }); + it('can load multiple configs for global options', () => { + const params = makeArgv({ + userCmd: ['fakecommand'], + globalOpt: { + 'file-path': { + demand: false, + type: 'string', + }, + }, + }); + + // Make sure the second global option overrides the first. + const firstConfigObject = { + filePath: 'first/path', + }; + const secondConfigObject = { + filePath: 'second/path', + }; + + let argv = applyConf({ + ...params, configObject: firstConfigObject, + }); + argv = applyConf({ + ...params, argv, configObject: secondConfigObject, + }); + assert.strictEqual(argv.filePath, secondConfigObject.filePath); + }); + it('uses CLI option over undefined configured option and default', () => { const cmdLineSrcDir = '/user/specified/source/dir/'; const params = makeArgv({ @@ -474,6 +506,41 @@ describe('config', () => { assert.strictEqual(newArgv.apiKey, cmdApiKey); }); + it('can load multiple configs for sub-command options', () => { + const params = makeArgv({ + userCmd: ['sign'], + command: 'sign', + commandOpt: { + 'file-path': { + demand: false, + type: 'string', + }, + }, + }); + + // Make sure the second sub-command option overrides the first. + const firstConfigObject = { + sign: { + filePath: 'first/path', + }, + }; + const secondConfigObject = { + sign: { + filePath: 'second/path', + }, + }; + + let argv = applyConf({ + ...params, configObject: firstConfigObject, + }); + argv = applyConf({ + ...params, argv, configObject: secondConfigObject, + }); + assert.strictEqual( + argv.filePath, secondConfigObject.sign.filePath + ); + }); + it('preserves default value if not in config', () => { const params = makeArgv({ userCmd: ['sign'], @@ -803,9 +870,88 @@ describe('config', () => { return withTempDir( (tmpDir) => { const configFilePath = path.join(tmpDir.path(), 'config.js'); - fs.writeFileSync(configFilePath, '{};'); + fs.writeFileSync(configFilePath, 'module.exports = {};'); loadJSConfigFile(configFilePath); }); }); }); + + describe('discoverConfigFiles', () => { + function _discoverConfigFiles(params = {}) { + return discoverConfigFiles({ + // By default, do not look in the real home directory. + getHomeDir: () => '/not-a-directory', + ...params, + }); + } + + it('finds a config in your home directory', () => { + return withTempDir( + async (tmpDir) => { + const homeDirConfig = path.join( + tmpDir.path(), '.web-ext-config.js' + ); + await fs.writeFile(homeDirConfig, 'module.exports = {}'); + assert.deepEqual( + // Stub out getHomeDir() so that it returns tmpDir.path() + // as if that was a user's home directory. + await _discoverConfigFiles({ + getHomeDir: () => tmpDir.path(), + }), + [path.resolve(homeDirConfig)] + ); + }); + }); + + it('finds a config in your working directory', () => { + return withTempDir( + async (tmpDir) => { + const lastDir = process.cwd(); + process.chdir(tmpDir.path()); + try { + const expectedConfig = path.resolve( + path.join(process.cwd(), 'web-ext-config.js') + ); + await fs.writeFile(expectedConfig, 'module.exports = {}'); + + assert.deepEqual( + await _discoverConfigFiles(), + [expectedConfig] + ); + } finally { + process.chdir(lastDir); + } + }); + }); + + it('discovers all config files', () => { + return withTempDir( + async (tmpDir) => { + const lastDir = process.cwd(); + process.chdir(tmpDir.path()); + try { + const fakeHomeDir = path.join(tmpDir.path(), 'home-dir'); + await fs.mkdir(fakeHomeDir); + const globalConfig = path.resolve( + path.join(fakeHomeDir, '.web-ext-config.js') + ); + await fs.writeFile(globalConfig, 'module.exports = {}'); + + const projectConfig = path.resolve( + path.join(process.cwd(), 'web-ext-config.js') + ); + await fs.writeFile(projectConfig, 'module.exports = {}'); + + assert.deepEqual( + await _discoverConfigFiles({ + getHomeDir: () => fakeHomeDir, + }), + [globalConfig, projectConfig] + ); + } finally { + process.chdir(lastDir); + } + }); + }); + }); }); diff --git a/tests/unit/test.program.js b/tests/unit/test.program.js index ff3a7eec57..d7db25ca29 100644 --- a/tests/unit/test.program.js +++ b/tests/unit/test.program.js @@ -7,6 +7,7 @@ import {fs} from 'mz'; import sinon, {spy} from 'sinon'; import {assert} from 'chai'; +import {applyConfigToArgv} from '../../src/config'; import {defaultVersionGetter, main, Program} from '../../src/program'; import commands from '../../src/cmd'; import { @@ -317,14 +318,41 @@ describe('program.Program', () => { describe('program.main', () => { - function execProgram(argv, {projectRoot = '', ...mainOptions}: Object = {}) { - const runOptions = { - getVersion: () => 'not-a-real-version', - checkForUpdates: spy(), - shouldExitProgram: false, - systemProcess: createFakeProcess(), + function execProgram( + argv, + {projectRoot = '', runOptions, ...mainOptions}: Object = {} + ) { + return main( + projectRoot, + { + argv, + runOptions: { + discoverConfigFiles: async () => [], + getVersion: () => 'not-a-real-version', + checkForUpdates: spy(), + shouldExitProgram: false, + systemProcess: createFakeProcess(), + ...runOptions, + }, + ...mainOptions, + } + ); + } + + type MakeConfigLoaderParams = {| + configObjects: { [fileName: string]: Object }, + |}; + + function makeConfigLoader( + {configObjects}: MakeConfigLoaderParams + ) { + return (fileName) => { + const conf = configObjects[fileName]; + if (!conf) { + throw new Error(`Config file was not mapped: ${fileName}`); + } + return conf; }; - return main(projectRoot, {argv, runOptions, ...mainOptions}); } it('executes a command handler', () => { @@ -466,7 +494,7 @@ describe('program.main', () => { }); }); - it('applies options from the specified config file', () => { + it('applies options from the specified config file', async () => { const fakeCommands = fake(commands, { lint: () => Promise.resolve(), }); @@ -480,21 +508,171 @@ describe('program.main', () => { return configObject; }); - return execProgram( + await execProgram( ['lint', '--config', 'path/to/web-ext-config.js'], { commands: fakeCommands, runOptions: { loadJSConfigFile: fakeLoadJSConfigFile, }, - }) - .then(() => { - const options = fakeCommands.lint.firstCall.args[0]; - // This makes sure that the config object was applied - // to the lint command options. - assert.equal( - options.selfHosted, configObject.lint.selfHosted); - }); + } + ); + + const options = fakeCommands.lint.firstCall.args[0]; + // This makes sure that the config object was applied + // to the lint command options. + assert.equal( + options.selfHosted, configObject.lint.selfHosted); + }); + + it('discovers config files', async () => { + const fakeCommands = fake(commands, { + lint: () => Promise.resolve(), + }); + const configObject = { + lint: { + selfHosted: true, + }, + }; + // Instead of loading/parsing a real file, just return an object. + const fakeLoadJSConfigFile = sinon.spy(() => { + return configObject; + }); + + const discoveredFile = 'fake/config.js'; + await execProgram( + ['lint'], + { + commands: fakeCommands, + runOptions: { + discoverConfigFiles: async () => [discoveredFile], + loadJSConfigFile: fakeLoadJSConfigFile, + }, + } + ); + + const options = fakeCommands.lint.firstCall.args[0]; + // This makes sure that the config object was applied + // to the lint command options. + assert.equal( + options.selfHosted, configObject.lint.selfHosted); + + sinon.assert.calledWith(fakeLoadJSConfigFile, discoveredFile); + }); + + it('lets you disable config discovery', async () => { + const fakeCommands = fake(commands, { + lint: () => Promise.resolve(), + }); + + const discoverConfigFiles = sinon.spy(() => Promise.resolve([])); + await execProgram( + ['--no-config-discovery', 'lint'], + { + commands: fakeCommands, + runOptions: { + discoverConfigFiles, + }, + } + ); + + sinon.assert.notCalled(discoverConfigFiles); + }); + + it('applies config files in order', async () => { + const fakeCommands = fake(commands, { + lint: () => Promise.resolve(), + }); + + const globalConfig = 'home/dir/.web-ext-config.js'; + const projectConfig = 'project/dir/web-ext-config.js'; + const customConfig = path.resolve('custom/web-ext-config.js'); + + const loadJSConfigFile = makeConfigLoader({ + configObjects: { + [globalConfig]: { + noInput: true, + }, + [projectConfig]: { + verbose: true, + }, + [customConfig]: { + lint: { + selfHosted: true, + }, + }, + }, + }); + const fakeApplyConfigToArgv = sinon.spy(applyConfigToArgv); + + await execProgram( + ['lint', '--config', customConfig], + { + commands: fakeCommands, + runOptions: { + applyConfigToArgv: fakeApplyConfigToArgv, + discoverConfigFiles: async () => [ + globalConfig, projectConfig, + ], + loadJSConfigFile, + }, + } + ); + + // Check that the config files were all applied to argv. + const options = fakeCommands.lint.firstCall.args[0]; + assert.equal(options.noInput, true); + assert.equal(options.verbose, true); + assert.equal(options.selfHosted, true); + + // Make sure the config files were loaded in the right order. + assert.include(fakeApplyConfigToArgv.firstCall.args[0], { + configFileName: globalConfig, + }); + assert.include(fakeApplyConfigToArgv.secondCall.args[0], { + configFileName: projectConfig, + }); + assert.include(fakeApplyConfigToArgv.thirdCall.args[0], { + configFileName: customConfig, + }); + }); + + it('overwrites old config values', async () => { + const fakeCommands = fake(commands, { + lint: () => Promise.resolve(), + }); + + const globalConfig = path.resolve('home/dir/.web-ext-config.js'); + const customConfig = path.resolve('custom/web-ext-config.js'); + + const finalSourceDir = path.resolve('final/source-dir'); + const loadJSConfigFile = makeConfigLoader({ + configObjects: { + // This config is loaded first. + [globalConfig]: { + sourceDir: 'first/source-dir', + }, + // This config is loaded next which overwrites the old value. + [customConfig]: { + sourceDir: finalSourceDir, + }, + }, + }); + + await execProgram( + ['lint', '--config', customConfig], + { + commands: fakeCommands, + runOptions: { + discoverConfigFiles: async () => [globalConfig], + loadJSConfigFile, + }, + } + ); + + const options = fakeCommands.lint.firstCall.args[0]; + // This should equal the final configured value. + assert.equal(options.sourceDir, finalSourceDir); }); });