Make applyConfig transform argv instead of builders (#84)

Per the `yargs` docs, [`yargs.middleware(...)`](https://yargs.js.org/docs/#api-reference-middlewarecallbacks-applybeforevalidation) is intended to be used for `argv` transformations that should be run before a command is applied.

`applyConfig` used to transform the object exported by `builders`. This is a bad idea. Instead, it now transforms `argv` in the `yargs` middleware, just like `sanitizeInputs` was already doing.

Additionally, `assignGlobals` was renamed to `setSnapGlobals`.

src/builders.ts

@@ -1,6 +1,6 @@
-import { Builder } from './types/package';
+import { Builders } from './types/package';
 
-const builders: Builder = {
+const builders: Builders = {
   src: {
     alias: 's',
     describe: 'Source file',

src/cli.ts

@@ -1,6 +1,7 @@
 import yargs from 'yargs/yargs';
 import { SnapsCliGlobals } from './types/package';
-import { assignGlobals, sanitizeInputs } from './utils';
+import { applyConfig, sanitizeInputs, setSnapGlobals } from './utils';
+import builders from './builders';
 
 declare global {
   // eslint-disable-next-line @typescript-eslint/no-namespace
@@ -25,26 +26,21 @@ export function cli(argv: string[], commands: any): void {
 
     .command(commands)
 
-    .option('verboseErrors', {
-      alias: ['v', 'verboseErrors'],
-      type: 'boolean',
-      describe: 'Display original errors',
-      required: false,
-      default: false,
-    })
+    .option('verboseErrors', builders.verboseErrors)
 
-    .option('suppressWarnings', {
-      alias: ['sw', 'suppressWarnings'],
-      type: 'boolean',
-      describe: 'Suppress warnings',
-      required: false,
-      default: false,
-    })
+    .option('suppressWarnings', builders.suppressWarnings)
 
     .strict()
 
-    .middleware((yargsArgv) => {
-      assignGlobals(yargsArgv);
+    .middleware(async (yargsArgv) => {
+      // Some globals affect error logging, and applyConfig may error.
+      // Therefore, we set globals from the yargs argv object before applying
+      // defaults from the config, and then set the globals again after, to
+      // ensure that inline options are preferred over any config files.
+      // This is ugly, but cheap.
+      setSnapGlobals(yargsArgv);
+      await applyConfig(yargsArgv);
+      setSnapGlobals(yargsArgv);
       sanitizeInputs(yargsArgv);
     })
 

src/main.ts

@@ -1,6 +1,5 @@
 #!/usr/bin/env node
 import { cli } from './cli';
-import { applyConfig } from './utils';
 import commands from './cmds';
 
 global.snaps = {
@@ -12,7 +11,6 @@ global.snaps = {
 main();
 
 // application
-async function main(): Promise<void> {
-  await applyConfig();
+function main(): void {
   cli(process.argv, commands);
 }

src/types/package.d.ts

@@ -19,20 +19,19 @@ export interface NodePackageManifest {
   web3Wallet?: ManifestWalletProperty;
 }
 
-export interface Builder {
-  [key: string]: Options;
-  src: Options;
-  dist: Options;
-  bundle: Options;
-  root: Options;
-  port: Options;
-  sourceMaps: Options;
-  stripComments: Options;
-  outfileName: Options;
-  manifest: Options;
-  populate: Options;
-  eval: Options;
-  verboseErrors: Options;
-  suppressWarnings: Options;
-  environment: Options;
+export interface Builders {
+  readonly src: Readonly<Options>;
+  readonly dist: Readonly<Options>;
+  readonly bundle: Readonly<Options>;
+  readonly root: Readonly<Options>;
+  readonly port: Readonly<Options>;
+  readonly sourceMaps: Readonly<Options>;
+  readonly stripComments: Readonly<Options>;
+  readonly outfileName: Readonly<Options>;
+  readonly manifest: Readonly<Options>;
+  readonly populate: Readonly<Options>;
+  readonly eval: Readonly<Options>;
+  readonly verboseErrors: Readonly<Options>;
+  readonly suppressWarnings: Readonly<Options>;
+  readonly environment: Readonly<Options>;
 }

src/utils/misc.ts

@@ -1,4 +1,4 @@
-import yargs from 'yargs';
+import { Arguments } from 'yargs';
 
 export const permRequestKeys = [
   '@context',
@@ -20,10 +20,9 @@ export const CONFIG_PATHS = [
  *
  * @param {Argument} argv - arguments as an object generated by yargs
  */
-export function assignGlobals(argv: yargs.Arguments<{
-  verboseErrors: boolean;
-} & {
-  suppressWarnings: boolean;
+export function setSnapGlobals(argv: Arguments<{
+  verboseErrors: unknown;
+  suppressWarnings: unknown;
 }>) {
   if (['w', 'watch'].includes(argv._[0] as string)) {
     global.snaps.isWatching = true;
@@ -43,11 +42,7 @@ export function assignGlobals(argv: yargs.Arguments<{
    *
    * @param {Argument} argv - arguments as an object generated by yargs
    */
-export function sanitizeInputs(argv: yargs.Arguments<{
-  verboseErrors: boolean;
-} & {
-  suppressWarnings: boolean;
-}>) {
+export function sanitizeInputs(argv: Arguments) {
   Object.keys(argv).forEach((key) => {
     if (typeof argv[key] === 'string') {
       if (argv[key] === './') {

src/utils/snap-config.ts

@@ -1,4 +1,5 @@
 import { promises as fs } from 'fs';
+import { Arguments } from 'yargs';
 import builders from '../builders';
 import { logError } from './misc';
 import { CONFIG_PATHS } from '.';
@@ -9,29 +10,29 @@ const INVALID_CONFIG_FILE = 'Invalid config file.';
  * Attempts to read the config file and apply the config to
  * globals.
  */
-export async function applyConfig(): Promise<void> {
+export async function applyConfig(argv: Arguments): Promise<void> {
   let pkg: any;
 
   // first, attempt to read and apply config from package.json
   try {
     pkg = JSON.parse(await fs.readFile('package.json', 'utf8'));
 
     if (pkg.main) {
-      builders.src.default = pkg.main;
+      argv.src = pkg.main;
     }
 
     if (pkg.web3Wallet) {
       const { bundle } = pkg.web3Wallet;
       if (bundle?.local) {
         const { local: bundlePath } = bundle;
-        builders.bundle.default = bundlePath;
+        argv.bundle = bundlePath;
         let dist: string;
         if (bundlePath.indexOf('/') === -1) {
           dist = '.';
         } else {
           dist = bundlePath.substr(0, bundlePath.indexOf('/') + 1);
         }
-        builders.dist.default = dist;
+        argv.dist = dist;
       }
     }
   } catch (err) {
@@ -67,7 +68,7 @@ export async function applyConfig(): Promise<void> {
   if (cfg && typeof cfg === 'object' && !Array.isArray(cfg)) {
     for (const key of Object.keys(cfg)) {
       if (Object.hasOwnProperty.call(builders, key)) {
-        builders[key].default = cfg[key];
+        argv[key] = cfg[key];
       } else {
         logError(
           `Error: Encountered unrecognized config file property "${key}" in config file "${usedConfigPath as string}".`,

test/utils/misc.test.js

@@ -1,4 +1,4 @@
-const { trimPathString, logError, logWarning, sanitizeInputs, assignGlobals } = require('../../dist/src/utils/misc');
+const { trimPathString, logError, logWarning, sanitizeInputs, setSnapGlobals } = require('../../dist/src/utils/misc');
 
 describe('misc', () => {
   global.snaps = {
@@ -115,16 +115,16 @@ describe('misc', () => {
     delete global.snaps;
   });
 
-  describe('assignGlobals', () => {
+  describe('setSnapGlobals', () => {
     it('sets global variables correctly', () => {
-      assignGlobals(exampleArgv);
+      setSnapGlobals(exampleArgv);
       expect(global.snaps.isWatching).toStrictEqual(true);
       expect(global.snaps.verboseErrors).toStrictEqual(true);
       expect(global.snaps.suppressWarnings).toStrictEqual(true);
     });
 
     it('doesnt set global variables incorrectly', () => {
-      assignGlobals(defaultArgv);
+      setSnapGlobals(defaultArgv);
       expect(global.snaps.isWatching).toStrictEqual(false);
       expect(global.snaps.verboseErrors).toStrictEqual(false);
       expect(global.snaps.suppressWarnings).toStrictEqual(false);

test/utils/snap-config.test.js

@@ -3,11 +3,6 @@ const { default: builders } = require('../../dist/src/builders');
 const { applyConfig } = require('../../dist/src/utils/snap-config');
 const misc = require('../../dist/src/utils/misc');
 
-const originalBuilders = Object.keys(builders).reduce((snapshot, key) => {
-  snapshot[key] = builders[key];
-  return builders[key];
-}, {});
-
 const getDefaultWeb3Wallet = () => {
   return {
     'bundle': {
@@ -37,6 +32,22 @@ const getDefaultSnapConfig = () => {
   };
 };
 
+const getArgv = ({
+  bundle = builders.bundle.default,
+  dist = builders.dist.default,
+  outfileName = builders.outfileName.default,
+  port = builders.port.default,
+  src = builders.src.default,
+} = {}) => {
+  return {
+    bundle,
+    dist,
+    outfileName,
+    port,
+    src,
+  };
+};
+
 describe('snap-config', () => {
   let fsMock;
 
@@ -59,9 +70,6 @@ describe('snap-config', () => {
   });
 
   afterEach(() => {
-    Object.keys(originalBuilders).forEach((key) => {
-      builders[key] = originalBuilders[key];
-    });
     fsMock.mockRestore();
     fsMock = null;
     delete global.snaps;
@@ -75,10 +83,12 @@ describe('snap-config', () => {
           return {};
         });
 
-      await applyConfig();
-      expect(builders.src.default).toStrictEqual('index.js');
-      expect(builders.bundle.default).toStrictEqual('dist/foo.js');
-      expect(builders.dist.default).toStrictEqual('dist/');
+      const argv = getArgv();
+      await applyConfig(argv);
+
+      expect(argv.src).toStrictEqual(builders.src.default);
+      expect(argv.bundle).toStrictEqual('dist/foo.js');
+      expect(argv.dist).toStrictEqual('dist/');
     });
 
     it('sets web3wallet with no local property correctly', async () => {
@@ -98,10 +108,12 @@ describe('snap-config', () => {
           return {};
         });
 
-      await applyConfig();
-      expect(builders.src.default).toStrictEqual(main);
-      expect(builders.bundle.default).toStrictEqual(builders.bundle.default);
-      expect(builders.dist.default).toStrictEqual(builders.dist.default);
+      const argv = getArgv();
+      await applyConfig(argv);
+
+      expect(argv.src).toStrictEqual(main);
+      expect(argv.bundle).toStrictEqual(builders.bundle.default);
+      expect(argv.dist).toStrictEqual(builders.dist.default);
     });
 
     it('sets dist field correctly in edge case', async () => {
@@ -122,10 +134,12 @@ describe('snap-config', () => {
           return {};
         });
 
-      await applyConfig();
-      expect(builders.src.default).toStrictEqual(main);
-      expect(builders.bundle.default).toStrictEqual(web3Wallet.bundle.local);
-      expect(builders.dist.default).toStrictEqual('.');
+      const argv = getArgv();
+      await applyConfig(argv);
+
+      expect(argv.src).toStrictEqual(main);
+      expect(argv.bundle).toStrictEqual(web3Wallet.bundle.local);
+      expect(argv.dist).toStrictEqual('.');
     });
 
     it('package.json read error is handled correctly', async () => {
@@ -137,7 +151,7 @@ describe('snap-config', () => {
           throw new Error('foo');
         });
 
-      await applyConfig();
+      await applyConfig(getArgv());
       expect(mockLogError).toHaveBeenCalled();
       expect(process.exit).toHaveBeenCalledWith(1);
     });
@@ -149,9 +163,11 @@ describe('snap-config', () => {
         .mockImplementationOnce(async () => getPackageJson())
         .mockImplementationOnce(async () => getDefaultSnapConfig());
 
-      await applyConfig();
-      expect(builders.port.default).toStrictEqual(8084);
-      expect(builders.outfileName.default).toStrictEqual('test.js');
+      const argv = getArgv();
+      await applyConfig(argv);
+
+      expect(argv.port).toStrictEqual(8084);
+      expect(argv.outfileName).toStrictEqual('test.js');
     });
 
     it('config file read error is handled correctly', async () => {
@@ -164,7 +180,7 @@ describe('snap-config', () => {
           throw new Error('foo');
         });
 
-      await applyConfig();
+      await applyConfig(getArgv());
       expect(mockLogError).toHaveBeenCalled();
       expect(process.exit).toHaveBeenCalledWith(1);
     });
@@ -177,7 +193,7 @@ describe('snap-config', () => {
         .mockImplementationOnce(async () => getPackageJson())
         .mockImplementationOnce(async () => 'foo');
 
-      await applyConfig();
+      await applyConfig(getArgv());
       expect(mockLogError).toHaveBeenCalled();
       expect(process.exit).toHaveBeenCalledWith(1);
     });
@@ -192,37 +208,9 @@ describe('snap-config', () => {
           return { foo: 'bar' };
         });
 
-      await applyConfig();
+      await applyConfig(getArgv());
       expect(mockLogError).toHaveBeenCalled();
       expect(process.exit).toHaveBeenCalledWith(1);
     });
   });
-
-  describe('applyConfig', () => {
-    it('sets default values correctly', async () => {
-      fsMock = jest.spyOn(fs, 'readFile')
-        .mockImplementationOnce(async () => getPackageJson())
-        .mockImplementationOnce(async () => {
-          return {};
-        });
-
-      await applyConfig();
-      expect(builders.src.default).toStrictEqual('index.js');
-      expect(builders.bundle.default).toStrictEqual('dist/foo.js');
-      expect(builders.dist.default).toStrictEqual('dist/');
-    });
-
-    it('config file overrides package.json fields', async () => {
-      const customConfig = {
-        'src': 'custom.js',
-      };
-
-      fsMock = jest.spyOn(fs, 'readFile')
-        .mockImplementationOnce(async () => getPackageJson())
-        .mockImplementationOnce(async () => customConfig);
-
-      await applyConfig();
-      expect(builders.src.default).toStrictEqual('custom.js');
-    });
-  });
 });