feat: basic cli-argument validation (#72)

* refactor: explicit check return code of commands

* refactor: specify cli arguments explicitly

This syntax is necessary in order to specify parsing functions in a future commit

* deps: bump commander

* refactor: remove unused cli option

As far as I can tell this "registry" option is not used. The login function seems to use the global registry option.

* feat: basic cli option/argument validation

* refactor: remove unecessary try-catch

* fix: confusing wording on error message

package.json

@@ -35,6 +35,7 @@
   "devDependencies": {
     "@babel/core": "^7.18.6",
     "@babel/eslint-parser": "^7.17.0",
+    "@commander-js/extra-typings": "^9.5.0",
     "@semantic-release/changelog": "^6.0.1",
     "@semantic-release/git": "^10.0.1",
     "@types/cli-table": "^0.3.2",
@@ -72,7 +73,7 @@
     "another-npm-registry-client": "^8.7.0",
     "chalk": "^4.1.2",
     "cli-table": "^0.3.11",
-    "commander": "^9.3.0",
+    "commander": "^9.5.0",
     "fs-extra": "^10.1.0",
     "is-wsl": "^2.2.0",
     "libnpmsearch": "^5.0.3",

src/cli-parsing.ts

@@ -0,0 +1,40 @@
+import { InvalidArgumentError } from "@commander-js/extra-typings";
+
+/**
+ * @throws {InvalidArgumentError}
+ */
+type CliValueParser<TOut> = (input: string, previous?: TOut) => TOut;
+
+export function mustSatisfy<TOut extends string>(
+  typeAssertion: (input: string) => input is TOut,
+  makeErrorMessage: (input: string) => string
+): CliValueParser<TOut> {
+  return (input) => {
+    if (!typeAssertion(input))
+      throw new InvalidArgumentError(makeErrorMessage(input));
+    return input;
+  };
+}
+
+export function mustBeParceable<TOut>(
+  parse: (input: string) => TOut,
+  makeErrorMessage: (input: string, error: unknown) => string
+): CliValueParser<TOut> {
+  return (input) => {
+    try {
+      return parse(input);
+    } catch (error) {
+      throw new InvalidArgumentError(makeErrorMessage(input, error));
+    }
+  };
+}
+
+export function eachValue<TOut>(
+  parser: CliValueParser<TOut>
+): CliValueParser<TOut[]> {
+  return (input, previous) => {
+    const parsed = parser(input);
+    if (previous === undefined) return [parsed];
+    else return [...previous, parsed];
+  };
+}

src/cli.ts

@@ -1,4 +1,4 @@
-import { program } from "commander";
+import { createCommand } from "@commander-js/extra-typings";
 import pkginfo from "pkginfo";
 import updateNotifier from "update-notifier";
 import { add } from "./cmd-add";
@@ -7,30 +7,69 @@ import { search } from "./cmd-search";
 import { view } from "./cmd-view";
 import { deps } from "./cmd-deps";
 import { login } from "./cmd-login";
-
 import log from "./logger";
 
 // update-notifier
 import pkg from "../package.json";
-import { assertIsError } from "./utils/error-type-guards";
+import { eachValue, mustBeParceable, mustSatisfy } from "./cli-parsing";
+import { isPackageReference } from "./types/package-reference";
+import { isDomainName } from "./types/domain-name";
+import { coerceRegistryUrl } from "./types/registry-url";
+import { CmdOptions } from "./types/options";
+
+const mustBePackageReference = mustSatisfy(
+  isPackageReference,
+  (input) => `"${input}" is not a valid package-reference`
+);
+
+const mustBeDomainName = mustSatisfy(
+  isDomainName,
+  (input) => `"${input}" is not a valid package name`
+);
+
+const mustBeRegistryUrl = mustBeParceable(
+  coerceRegistryUrl,
+  (input) => `"${input}" is not a valid registry-url`
+);
 
 pkginfo(module);
 const notifier = updateNotifier({ pkg });
 notifier.notify();
 
-program
+const program = createCommand()
   .version(module.exports.version)
   .option("-c, --chdir <path>", "change the working directory")
-  .option("-r, --registry <url>", "specify registry url")
+  .option("-r, --registry <url>", "specify registry url", mustBeRegistryUrl)
   .option("-v, --verbose", "output extra debugging")
   .option("--cn", "use the China region registry")
   .option("--system-user", "auth for Windows system user")
   .option("--wsl", "auth for Windows when using WSL")
   .option("--no-upstream", "don't use upstream unity registry")
   .option("--no-color", "disable color");
 
+/**
+ * Creates a CmdOptions object by adding global options to the given
+ * specific options
+ * @param specificOptions The specific options
+ */
+function makeCmdOptions<T extends Record<string, unknown>>(
+  specificOptions: T
+): CmdOptions<T> {
+  return { ...specificOptions, _global: program.opts() };
+}
+
 program
-  .command("add <pkg> [otherPkgs...]")
+  .command("add")
+  .argument(
+    "<pkg>",
+    "Reference to the package that should be added",
+    mustBePackageReference
+  )
+  .argument(
+    "[otherPkgs...]",
+    "References to additional packages that should be added",
+    eachValue(mustBePackageReference)
+  )
   .aliases(["install", "i"])
   .option("-t, --test", "add package as testable")
   .option(
@@ -43,45 +82,50 @@ openupm add <pkg> [otherPkgs...]
 openupm add <pkg>@<version> [otherPkgs...]`
   )
   .action(async function (pkg, otherPkgs, options) {
-    options._global = program.opts();
     const pkgs = [pkg].concat(otherPkgs);
-    const retCode = await add(pkgs, options);
-    if (retCode) process.exit(retCode);
+    const retCode = await add(pkgs, makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 program
-  .command("remove <pkg> [otherPkgs...]")
+  .command("remove")
+  .argument("<pkg>", "Name of the package to remove", mustBeDomainName)
+  .argument(
+    "[otherPkgs...]",
+    "Names of additional packages to remove",
+    eachValue(mustBeDomainName)
+  )
   .aliases(["rm", "uninstall"])
   .description("remove package from manifest json")
   .action(async function (pkg, otherPkgs, options) {
-    options._global = program.opts();
     const pkgs = [pkg].concat(otherPkgs);
-    const retCode = await remove(pkgs, options);
-    if (retCode) process.exit(retCode);
+    const retCode = await remove(pkgs, makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 program
-  .command("search <keyword>")
+  .command("search")
+  .argument("<keyword>", "The keyword to search")
   .aliases(["s", "se", "find"])
   .description("Search package by keyword")
   .action(async function (keyword, options) {
-    options._global = program.opts();
-    const retCode = await search(keyword, options);
-    if (retCode) process.exit(retCode);
+    const retCode = await search(keyword, makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 program
-  .command("view <pkg>")
+  .command("view")
+  .argument("<pkg>", "Reference to a package", mustBePackageReference)
   .aliases(["v", "info", "show"])
   .description("view package information")
   .action(async function (pkg, options) {
-    options._global = program.opts();
-    const retCode = await view(pkg, options);
-    if (retCode) process.exit(retCode);
+    const retCode = await view(pkg, makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 program
-  .command("deps <pkg>")
+  .command("deps")
+  .argument("<pkg>", "Reference to a package", mustBePackageReference)
   .alias("dep")
   .option("-d, --deep", "view package dependencies recursively")
   .description(
@@ -90,9 +134,8 @@ openupm deps <pkg>
 openupm deps <pkg>@<version>`
   )
   .action(async function (pkg, options) {
-    options._global = program.opts();
-    const retCode = await deps(pkg, options);
-    if (retCode) process.exit(retCode);
+    const retCode = await deps(pkg, makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 program
@@ -101,23 +144,15 @@ program
   .option("-u, --username <username>", "username")
   .option("-p, --password <password>", "password")
   .option("-e, --email <email>", "email address")
-  .option("-r, --registry <url>", "registry url")
   .option("--basic-auth", "use basic authentication instead of token")
   .option(
     "--always-auth",
     "always auth for tarball hosted on a different domain"
   )
   .description("authenticate with a scoped registry")
   .action(async function (options) {
-    options._global = program.opts();
-    try {
-      const retCode = await login(options);
-      if (retCode) process.exit(retCode);
-    } catch (err) {
-      assertIsError(err);
-      log.error("", err.message);
-      process.exit(1);
-    }
+    const retCode = await login(makeCmdOptions(options));
+    if (retCode !== 0) process.exit(retCode);
   });
 
 // prompt for invalid command