From 932a3830396f4b7e6d9cc23cde9e4b6846475602 Mon Sep 17 00:00:00 2001 From: Mark Lee Date: Mon, 13 Jan 2020 13:16:45 -0800 Subject: [PATCH] refactor(core): avoid forEach and increase test coverage for forge-config --- packages/api/core/src/util/forge-config.ts | 37 ++++---- .../api/core/test/fast/forge-config_spec.ts | 84 ++++++++++++++++++- .../fixture/bad_external_forge_config/bad.js | 1 + .../bad_external_forge_config/package.json | 5 ++ .../fixture/bad_forge_config/package.json | 5 ++ .../core/test/fixture/dummy_js_conf/foo.js | 5 ++ .../test/fixture/no_forge_config/package.json | 1 + 7 files changed, 117 insertions(+), 21 deletions(-) create mode 100644 packages/api/core/test/fixture/bad_external_forge_config/bad.js create mode 100644 packages/api/core/test/fixture/bad_external_forge_config/package.json create mode 100644 packages/api/core/test/fixture/bad_forge_config/package.json create mode 100644 packages/api/core/test/fixture/dummy_js_conf/foo.js create mode 100644 packages/api/core/test/fixture/no_forge_config/package.json diff --git a/packages/api/core/src/util/forge-config.ts b/packages/api/core/src/util/forge-config.ts index b54e59bb76..10fac7429d 100644 --- a/packages/api/core/src/util/forge-config.ts +++ b/packages/api/core/src/util/forge-config.ts @@ -20,14 +20,13 @@ const proxify = ( newObject = [] as any; } - Object.keys(object).forEach((key) => { - const val = (object as any)[key]; + for (const [key, val] of Object.entries(object)) { if (typeof val === 'object' && key !== 'pluginInterface' && !(val instanceof RegExp)) { (newObject as any)[key] = proxify(buildIdentifier, (object as any)[key], `${envPrefix}_${underscoreCase(key)}`); } else { (newObject as any)[key] = (object as any)[key]; } - }); + } return new Proxy(newObject, { get(target, name, receiver) { @@ -82,7 +81,7 @@ export function fromBuildIdentifier(map: { [key: string]: T | undefined }) { }; } -async function forgeConfigIsValidFilePath(dir: string, forgeConfig: string | ForgeConfig) { +export async function forgeConfigIsValidFilePath(dir: string, forgeConfig: string | ForgeConfig) { return typeof forgeConfig === 'string' && ( await fs.pathExists(path.resolve(dir, forgeConfig)) @@ -90,6 +89,20 @@ async function forgeConfigIsValidFilePath(dir: string, forgeConfig: string | For ); } +export function renderConfigTemplate(dir: string, templateObj: any, obj: any) { + for (const [key, value] of Object.entries(obj)) { + if (typeof value === 'object' && value !== null) { + renderConfigTemplate(dir, templateObj, value); + } else if (typeof value === 'string') { + obj[key] = _template(value)(templateObj); + if (obj[key].startsWith('require:')) { + // eslint-disable-next-line global-require, import/no-dynamic-require + obj[key] = require(path.resolve(dir, obj[key].substr(8))); + } + } + } +} + export default async (dir: string) => { const packageJSON = await readRawPackageJson(dir); let forgeConfig: ForgeConfig | string | null = (packageJSON.config && packageJSON.config.forge) @@ -126,21 +139,7 @@ export default async (dir: string) => { }; const templateObj = { ...packageJSON, year: (new Date()).getFullYear() }; - const template = (obj: any) => { - Object.keys(obj).forEach((objKey) => { - if (typeof obj[objKey] === 'object' && obj !== null) { - template(obj[objKey]); - } else if (typeof obj[objKey] === 'string') { - obj[objKey] = _template(obj[objKey])(templateObj); - if (obj[objKey].startsWith('require:')) { - // eslint-disable-next-line global-require, import/no-dynamic-require - obj[objKey] = require(path.resolve(dir, obj[objKey].substr(8))); - } - } - }); - }; - - template(forgeConfig); + renderConfigTemplate(dir, templateObj, forgeConfig); forgeConfig.pluginInterface = new PluginInterface(dir, forgeConfig); diff --git a/packages/api/core/test/fast/forge-config_spec.ts b/packages/api/core/test/fast/forge-config_spec.ts index 0b2d9f5675..f12e21ac78 100644 --- a/packages/api/core/test/fast/forge-config_spec.ts +++ b/packages/api/core/test/fast/forge-config_spec.ts @@ -1,7 +1,7 @@ import { expect } from 'chai'; import path from 'path'; -import findConfig from '../../src/util/forge-config'; +import findConfig, { forgeConfigIsValidFilePath, renderConfigTemplate, setInitialForgeConfig } from '../../src/util/forge-config'; const defaults = { packagerConfig: {}, @@ -12,7 +12,21 @@ const defaults = { }; describe('forge-config', () => { - it('should resolve the object in package.json with defaults if one exists', async () => { + it('should fail if the config is not an object or requirable path', async () => { + await expect(findConfig(path.resolve(__dirname, '../fixture/bad_forge_config'))).to.eventually.be.rejectedWith('Expected packageJSON.config.forge to be an object or point to a requirable JS file'); + }); + + it('should fail if the external config is not parseable', async () => { + await expect(findConfig(path.resolve(__dirname, '../fixture/bad_external_forge_config'))).to.eventually.be.rejectedWith(/bad.js: Unexpected token/); + }); + + it('should be set to the defaults if no Forge config is specified in package.json', async () => { + const config = await findConfig(path.resolve(__dirname, '../fixture/no_forge_config')); + delete config.pluginInterface; + expect(config).to.deep.equal(defaults); + }); + + it('should resolve the object in package.json with defaults if one exists', async () => { const config = await findConfig(path.resolve(__dirname, '../fixture/dummy_app')); delete config.pluginInterface; expect(config).to.be.deep.equal({ @@ -126,4 +140,70 @@ describe('forge-config', () => { expect(conf.regexp.test('foo')).to.equal(true, 'regexp should match foo'); expect(conf.regexp.test('bar')).to.equal(false, 'regexp should not match bar'); }); + + describe('forgeConfigIsValidFilePath', () => { + it('succeeds for a file extension-less path', async () => { + await expect(forgeConfigIsValidFilePath(path.resolve(__dirname, '../fixture/dummy_js_conf/'), 'forge.different.config')).to.eventually.equal(true); + }); + + it('fails when a file is nonexistent', async () => { + await expect(forgeConfigIsValidFilePath(path.resolve(__dirname, '../fixture/dummy_js_conf/'), 'forge.nonexistent.config')).to.eventually.equal(false); + }); + }); + + describe('renderConfigTemplate', () => { + it('should import a JS file when a string starts with "require:"', () => { + const dir = path.resolve(__dirname, '../fixture/dummy_js_conf'); + const config = { + foo: 'require:foo', + }; + renderConfigTemplate(dir, {}, config); + expect(config.foo).to.deep.equal({ + bar: { + baz: 'quux', + }, + }); + }); + }); + + describe('setInitialForgeConfig', () => { + it('should normalize hyphens in maker names to underscores', () => { + const packageJSON = { + name: 'foo-bar', + config: { + forge: { + makers: [ + { + name: '@electron-forge/maker-test', + config: { + name: 'will be overwritten', + }, + }, + ], + }, + }, + }; + setInitialForgeConfig(packageJSON); + expect(packageJSON.config.forge.makers[0].config.name).to.equal('foo_bar'); + }); + + it('should handle when package.json name is not set', () => { + const packageJSON = { + config: { + forge: { + makers: [ + { + name: '@electron-forge/maker-test', + config: { + name: 'will be overwritten', + }, + }, + ], + }, + }, + }; + setInitialForgeConfig(packageJSON); + expect(packageJSON.config.forge.makers[0].config.name).to.equal(''); + }); + }); }); diff --git a/packages/api/core/test/fixture/bad_external_forge_config/bad.js b/packages/api/core/test/fixture/bad_external_forge_config/bad.js new file mode 100644 index 0000000000..5c34318c21 --- /dev/null +++ b/packages/api/core/test/fixture/bad_external_forge_config/bad.js @@ -0,0 +1 @@ +} diff --git a/packages/api/core/test/fixture/bad_external_forge_config/package.json b/packages/api/core/test/fixture/bad_external_forge_config/package.json new file mode 100644 index 0000000000..b01e1097a0 --- /dev/null +++ b/packages/api/core/test/fixture/bad_external_forge_config/package.json @@ -0,0 +1,5 @@ +{ + "config": { + "forge": "bad.js" + } +} diff --git a/packages/api/core/test/fixture/bad_forge_config/package.json b/packages/api/core/test/fixture/bad_forge_config/package.json new file mode 100644 index 0000000000..9889515e17 --- /dev/null +++ b/packages/api/core/test/fixture/bad_forge_config/package.json @@ -0,0 +1,5 @@ +{ + "config": { + "forge": 1 + } +} diff --git a/packages/api/core/test/fixture/dummy_js_conf/foo.js b/packages/api/core/test/fixture/dummy_js_conf/foo.js new file mode 100644 index 0000000000..cb08f71efe --- /dev/null +++ b/packages/api/core/test/fixture/dummy_js_conf/foo.js @@ -0,0 +1,5 @@ +module.exports = { + bar: { + baz: 'quux', + }, +} diff --git a/packages/api/core/test/fixture/no_forge_config/package.json b/packages/api/core/test/fixture/no_forge_config/package.json new file mode 100644 index 0000000000..0967ef424b --- /dev/null +++ b/packages/api/core/test/fixture/no_forge_config/package.json @@ -0,0 +1 @@ +{}