refactor(core): avoid forEach and increase test coverage for forge-co…

…nfig
electron · Jan 13, 2020 · 6b79717 · 6b79717
1 parent 4883037
commit 6b79717
Show file tree

Hide file tree

Showing 7 changed files with 117 additions and 21 deletions.
diff --git a/packages/api/core/src/util/forge-config.ts b/packages/api/core/src/util/forge-config.ts
@@ -20,14 +20,13 @@ const proxify = <T extends object>(
     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<T>(newObject, {
     get(target, name, receiver) {
@@ -82,14 +81,28 @@ export function fromBuildIdentifier<T>(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))
       || fs.pathExists(path.resolve(dir, `${forgeConfig}.js`))
     );
 }
 
+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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1 @@
+{}