-
Notifications
You must be signed in to change notification settings - Fork 343
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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)}"`); | ||
} | ||
|
||
|
@@ -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<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), | ||
]; | ||
|
||
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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we opt for the Maybe type Also how about using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll take another look but I think There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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; | ||
} | ||
} |
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'); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Filed: #1166