Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Discover config files in home dir, working dir #1161

Merged
merged 2 commits into from
Dec 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 60 additions & 8 deletions src/config.js
Original file line number Diff line number Diff line change
@@ -1,34 +1,41 @@
/* @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,
|};

export function applyConfigToArgv({
argv,
argvFromCLI,
configObject,
options,
configFileName,
}: ApplyConfigToArgvParams): Object {
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)}"`);
}

Expand All @@ -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});
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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<Array<string>> {
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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an issue (just a proposed "nice to have" for a follow up):

we could also support config options loaded from a section in the package.json file (e.g. a "web-ext" section, similarly to how other nodejs tools support config loading from the package.json file, as an example babel and jest both support it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Filed: #1166

];

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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we opt for the Maybe type Promise<?string>, we have to change if (f) { ... } into if (typeof(f) === 'string') {...}.

Also how about using configs.filter and returning its result (return configs.filter((f) => typeof(f) === 'string');) so that we don't need to accumulate them in the new existentConfig array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take another look but I think filter() was failing because Flow doesn't seem to understand that the returned array will not contain undefined values (or something like that).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the best workaround I found:

  const files = (
    (configs.filter((f) => typeof f === 'string'): Array<any>): Array<string>
  );
  return files;

That's too confusing though. I think it's probably best to leave it as is.

The Flow bug: facebook/flow#1414

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I agree, better to leave it as it is (which is also probably much more readable to a contributor that is not so used to the flow syntaxes).

if (typeof f === 'string') {
existingConfigs.push(f);
}
});
return existingConfigs;
}
63 changes: 53 additions & 10 deletions src/program.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/* @flow */
import os from 'os';
import path from 'path';
import {readFileSync} from 'fs';

Expand All @@ -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';
Expand All @@ -34,6 +36,7 @@ type ExecuteOptions = {
logStream?: typeof defaultLogStream,
getVersion?: VersionGetterFn,
applyConfigToArgv?: typeof defaultApplyConfigToArgv,
discoverConfigFiles?: typeof defaultConfigDiscovery,
loadJSConfigFile?: typeof defaultLoadJSConfigFile,
shouldExitProgram?: boolean,
globalEnv?: string,
Expand Down Expand Up @@ -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<void> {

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
37 changes: 37 additions & 0 deletions src/util/file-exists.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/* @flow */
import {fs} from 'mz';

import {isErrorWithCode} from '../errors';

type FileExistsOptions = {|
fileIsReadable: (filePath: string) => Promise<boolean>,
|};

/*
* 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<boolean> {
try {
await fileIsReadable(path);
const stat = await fs.stat(path);
return stat.isFile();
} catch (error) {
if (isErrorWithCode(['EACCES', 'ENOENT'], error)) {
return false;
}
throw error;
}
}
7 changes: 3 additions & 4 deletions tests/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,8 @@ export function fixturePath(...pathParts: Array<string>): string {
*
* Usage:
*
* Promise.resolve()
* .then(makeSureItFails())
* .catch((error) => {
* Promise.reject(new Error('some error'))
* .then(makeSureItFails(), (error) => {
* // Safely make assertions about the error...
* });
*/
Expand Down Expand Up @@ -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';
}
}
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/test-util/test.artifacts.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
}
));
Expand Down
60 changes: 60 additions & 0 deletions tests/unit/test-util/test.file-exists.js
Original file line number Diff line number Diff line change
@@ -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');
});
});
});
Loading