Fix applyConfig behavior (#85)

#84 left `applyConfig` in a broken state. This PR fixes the behavior of `applyConfig` such that the config will transform `argv`, but only by setting properties that are valid arguments for the current command and that _have not_ been specified by the user on the command line.

In addition, this PR makes the following changes:
- Remove duplicate call to `setSnapGlobals` in `yargs` middleware
  - The duplicate call was added in #84 because `applyConfig` would overwrite arguments given on the command line, but this is no longer the case.  
- Stop parsing config options from `package.json`
  - We used the `web3Wallet` property in `package.json` to infer arguments to the CLI
  - This implicit, default behavior could be surprising. Let's just use explicit configs instead.
- Use `normalize` config in builders for path arguments to make `yargs` normalize paths
  - `sanitizeInputs` now only converts `./` to `.`, which is not done by the `yargs` normalizer (which is just `NodeJS.path.normalize(str)`
- Change deprecated `required` property in builders to `demandOption`
- Remove some outdated aliases for the `bundle` option

.eslintrc.js

@@ -47,6 +47,16 @@ module.exports = {
         'node/shebang': 'off',
       },
     },
+    {
+      files: [
+        // Exports a yargs middleware function, which must be synchronous.
+        'src/utils/snap-config.ts',
+        'test/utils/snap-config.test.js',
+      ],
+      rules: {
+        'node/no-sync': 'off',
+      },
+    },
     {
       files: [
         'examples/**/*.js',

package.json

@@ -69,7 +69,8 @@
     "ses": "^0.11.0",
     "strip-comments": "^2.0.1",
     "terser": "^4.3.1",
-    "yargs": "^16.2.0"
+    "yargs": "^16.2.0",
+    "yargs-parser": "^20.2.2"
   },
   "engines": {
     "node": ">=14.15.1"

src/builders.ts

@@ -5,109 +5,113 @@ const builders: Builders = {
     alias: 's',
     describe: 'Source file',
     type: 'string',
-    required: true,
+    demandOption: true,
+    normalize: true,
     default: 'index.js',
   },
 
   dist: {
     alias: 'd',
     describe: 'Output directory',
     type: 'string',
-    required: true,
+    demandOption: true,
+    normalize: true,
     default: 'dist',
   },
 
   bundle: {
-    alias: ['plugin', 'p', 'b'],
+    alias: 'b',
     describe: 'Snap bundle file',
     type: 'string',
-    required: true,
+    demandOption: true,
+    normalize: true,
     default: 'dist/bundle.js',
   },
 
   root: {
     alias: 'r',
     describe: 'Server root directory',
     type: 'string',
-    required: true,
+    demandOption: true,
+    normalize: true,
     default: '.',
   },
 
   port: {
     alias: 'p',
     describe: 'Local server port for testing',
     type: 'number',
-    required: true,
+    demandOption: true,
     default: 8081,
   },
 
   sourceMaps: {
     describe: 'Whether builds include sourcemaps',
     type: 'boolean',
-    required: false,
+    demandOption: false,
     default: false,
   },
 
   stripComments: {
     alias: 'strip',
     describe: 'Whether to remove code comments from the build output',
     type: 'boolean',
-    required: false,
+    demandOption: false,
     default: false,
   },
 
   outfileName: {
     alias: 'n',
     describe: 'Output file name',
     type: 'string',
-    required: false,
+    demandOption: false,
     default: 'bundle.js',
   },
 
   manifest: {
     alias: 'm',
     describe: 'Validate project package.json as a Snap manifest',
     type: 'boolean',
-    required: false,
+    demandOption: false,
     default: true,
   },
 
   populate: {
     describe: 'Update Snap manifest properties of package.json',
     type: 'boolean',
-    required: false,
+    demandOption: false,
     default: true,
   },
 
   eval: {
     alias: 'e',
     describe: 'Attempt to evaluate Snap bundle in SES',
     type: 'boolean',
-    required: false,
+    demandOption: false,
     default: true,
   },
 
   verboseErrors: {
-    alias: ['v', 'verboseErrors'],
+    alias: 'v',
     type: 'boolean',
     describe: 'Display original errors',
-    required: false,
+    demandOption: false,
     default: false,
   },
 
   suppressWarnings: {
-    alias: ['sw', 'suppressWarnings'],
+    alias: 'sw',
     type: 'boolean',
     describe: 'Suppress warnings',
-    required: false,
+    demandOption: false,
     default: false,
   },
 
   environment: {
     alias: 'env',
     describe: 'The execution environment of the plugin.',
     type: 'string',
-    required: false,
+    demandOption: false,
     default: 'worker',
     choices: ['worker'],
   },

src/cli.ts

@@ -1,4 +1,6 @@
-import yargs from 'yargs/yargs';
+import yargs, { Arguments } from 'yargs';
+import yargsType from 'yargs/yargs';
+
 import { SnapsCliGlobals } from './types/package';
 import { applyConfig, sanitizeInputs, setSnapGlobals } from './utils';
 import builders from './builders';
@@ -12,8 +14,9 @@ declare global {
 }
 
 export function cli(argv: string[], commands: any): void {
+  const rawArgv = argv.slice(2);
   // eslint-disable-next-line @typescript-eslint/no-unused-expressions
-  yargs(argv.slice(2))
+  yargs(rawArgv)
 
     .usage('Usage: $0 <command> [options]')
 
@@ -32,17 +35,14 @@ export function cli(argv: string[], commands: any): void {
 
     .strict()
 
-    .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);
+    // Typecast: The @types/yargs type for .middleware is incorrect.
+    // yargs middleware functions receive the yargs instance as a second parameter.
+    // ref: https://yargs.js.org/docs/#api-reference-middlewarecallbacks-applybeforevalidation
+    .middleware(((yargsArgv: Arguments, yargsInstance: typeof yargsType) => {
+      applyConfig(rawArgv, yargsArgv, yargsInstance);
       setSnapGlobals(yargsArgv);
       sanitizeInputs(yargsArgv);
-    })
+    }) as any, true)
 
     .fail((msg: string, err: Error, _yargs) => {
       console.error(msg || err.message);

src/utils/misc.ts

@@ -20,10 +20,7 @@ export const CONFIG_PATHS = [
  *
  * @param {Argument} argv - arguments as an object generated by yargs
  */
-export function setSnapGlobals(argv: Arguments<{
-  verboseErrors: unknown;
-  suppressWarnings: unknown;
-}>) {
+export function setSnapGlobals(argv: Arguments) {
   if (['w', 'watch'].includes(argv._[0] as string)) {
     global.snaps.isWatching = true;
   } else {
@@ -38,17 +35,17 @@ export function setSnapGlobals(argv: Arguments<{
 }
 
 /**
-   * Sanitizes inputs. Currently normalizes paths.
+   * Sanitizes inputs. Currently normalizes "./" paths to ".".
+   * Yargs handles other path normalization as specified in builders.
    *
    * @param {Argument} argv - arguments as an object generated by yargs
    */
 export function sanitizeInputs(argv: Arguments) {
   Object.keys(argv).forEach((key) => {
     if (typeof argv[key] === 'string') {
+      // Node's path.normalize() does not do this
       if (argv[key] === './') {
         argv[key] = '.';
-      } else if ((argv[key] as string).startsWith('./')) {
-        argv[key] = (argv[key] as string).substring(2);
       }
     }
   });

src/utils/snap-config.ts

@@ -1,78 +1,94 @@
-import { promises as fs } from 'fs';
+import fs from 'fs';
 import { Arguments } from 'yargs';
+import yargs from 'yargs/yargs';
+import yargsParse from 'yargs-parser';
 import builders from '../builders';
 import { logError } from './misc';
 import { CONFIG_PATHS } from '.';
 
-const INVALID_CONFIG_FILE = 'Invalid config file.';
+// Note that the below function is necessary because yarg's .config() function
+// leaves much to be desired.
+//
+// In particular, it will set all properties included in the config file
+// regardless of the command, which fails during validation.
 
 /**
- * Attempts to read the config file and apply the config to
- * globals.
+ * Attempts to read configuration options for package.json and the config file,
+ * and apply them to argv if they weren't already set.
+ *
+ * Arguments are only set per the snap-cli config file if they were not specified
+ * on the command line.
  */
-export async function applyConfig(argv: Arguments): Promise<void> {
-  let pkg: any;
+export function applyConfig(
+  processArgv: string[],
+  yargsArgv: Arguments,
+  yargsInstance: typeof yargs,
+): void {
+  // Instances of yargs has a number of undocumented functions, including
+  // getOptions. This function returns an object with properties "key" and
+  // "alias", which specify the options associated with the current command and
+  // their aliases, respectively.
+  //
+  // We leverage this to ensure that the config is only applied to args that are
+  // valid for the current command, and that weren't specified by the user on
+  // the command line.
+  //
+  // If we set args that aren't valid for the current command, yargs will error
+  // during validation.
+  const {
+    alias: aliases,
+    key: options,
+  } = (yargsInstance as any).getOptions() as {
+    alias: Record<string, string[]>;
+    key: Record<string, unknown>;
+  };
 
-  // first, attempt to read and apply config from package.json
-  try {
-    pkg = JSON.parse(await fs.readFile('package.json', 'utf8'));
+  const parsedProcessArgv = yargsParse(processArgv, {
+    alias: aliases,
+  }) as Record<string, unknown>;
+  delete parsedProcessArgv._; // irrelevant yargs parser artifact
 
-    if (pkg.main) {
-      argv.src = pkg.main;
-    }
+  const commandOptions = new Set(Object.keys(options));
 
-    if (pkg.web3Wallet) {
-      const { bundle } = pkg.web3Wallet;
-      if (bundle?.local) {
-        const { local: bundlePath } = bundle;
-        argv.bundle = bundlePath;
-        let dist: string;
-        if (bundlePath.indexOf('/') === -1) {
-          dist = '.';
-        } else {
-          dist = bundlePath.substr(0, bundlePath.indexOf('/') + 1);
-        }
-        argv.dist = dist;
-      }
-    }
-  } catch (err) {
-    if (err.code !== 'ENOENT') {
-      logError(
-        'Error: package.json exists but could not be parsed.',
-        err,
-      );
-      process.exit(1);
-    }
-  }
+  const shouldSetArg = (key: string): boolean => {
+    return commandOptions.has(key) &&
+      !Object.prototype.hasOwnProperty.call(parsedProcessArgv, key);
+  };
 
-  // second, attempt to read and apply config from config file,
-  // which will always be preferred if it exists
+  // Now, we attempt to read and apply config from the config file, if any.
   let cfg: Record<string, unknown> = {};
   let usedConfigPath: string | null = null;
   for (const configPath of CONFIG_PATHS) {
     try {
-      cfg = JSON.parse(await fs.readFile(configPath, 'utf8'));
+      cfg = JSON.parse(fs.readFileSync(configPath, 'utf8'));
       usedConfigPath = configPath;
       break;
     } catch (err) {
-      if (err.code !== 'ENOENT') {
-        logError(
-          `Error: "${configPath}" exists but could not be parsed`,
-          err,
-        );
-        process.exit(1);
+      if (err.code === 'ENOENT') {
+        // If there's no config file, we're done here.
+        return;
       }
+
+      logError(
+        `Error: "${configPath}" exists but could not be parsed. Ensure your config file is valid JSON and try again.`,
+        err,
+      );
+      process.exit(1);
+
     }
   }
 
   if (cfg && typeof cfg === 'object' && !Array.isArray(cfg)) {
     for (const key of Object.keys(cfg)) {
       if (Object.hasOwnProperty.call(builders, key)) {
-        argv[key] = cfg[key];
+        if (shouldSetArg(key)) {
+          yargsArgv[key] = cfg[key];
+        }
       } else {
         logError(
-          `Error: Encountered unrecognized config file property "${key}" in config file "${usedConfigPath as string}".`,
-          new Error(INVALID_CONFIG_FILE),
+          `Error: Encountered unrecognized config property "${key}" in config file "${
+            usedConfigPath as string
+          }". Remove the property and try again.`,
         );
         process.exit(1);
       }
@@ -81,8 +97,9 @@ export async function applyConfig(argv: Arguments): Promise<void> {
     const cfgType = cfg === null ? 'null' : typeof cfg;
 
     logError(
-      `Error: The config file must consist of a top-level JSON object. Received "${cfgType}" from "${usedConfigPath as string}".`,
-      new Error(INVALID_CONFIG_FILE),
+      `Error: The config file must consist of a top-level JSON object. Received "${cfgType}" from "${
+        usedConfigPath as string
+      }". Fix your config file and try again.`,
     );
     process.exit(1);
   }

test/utils/misc.test.js

@@ -83,7 +83,7 @@ describe('misc', () => {
     sw: false,
     'suppress-warnings': false,
     src: '.',
-    s: 'index.js',
+    s: './index.js', // now handled by yargs itself
     dist: 'dist',
     d: 'dist',
     outfileName: 'bundle.js',