Merge pull request #304 from spotify/rugvip/cli-helpers

cli: add helpers for running child processes and error handling in commands

packages/cli/src/commands/plugin/build.ts

@@ -14,12 +14,11 @@
  * limitations under the License.
  */
 
-import chalk from 'chalk';
 import { Command } from 'commander';
-import { spawnSync } from 'child_process';
 import fs from 'fs-extra';
 import recursive from 'recursive-readdir';
 import path from 'path';
+import { run } from '../../helpers/run';
 
 export default async (cmd: Command) => {
   const args = [
@@ -35,17 +34,8 @@ export default async (cmd: Command) => {
     args.push('--watch');
   }
 
-  try {
-    await copyStaticAssets();
-    const result = spawnSync('tsc', args, { stdio: 'inherit', shell: true });
-    if (result.error) {
-      throw result.error;
-    }
-    process.exit(result.status ?? 0);
-  } catch (error) {
-    process.stderr.write(`${chalk.red(error.message)}\n`);
-    process.exit(1);
-  }
+  await copyStaticAssets();
+  await run('tsc', args);
 };
 
 const copyStaticAssets = async () => {

packages/cli/src/commands/plugin/lint.ts

@@ -14,9 +14,8 @@
  * limitations under the License.
  */
 
-import chalk from 'chalk';
 import { Command } from 'commander';
-import { spawnSync } from 'child_process';
+import { run } from '../../helpers/run';
 
 export default async (cmd: Command) => {
   const args = ['lint'];
@@ -24,14 +23,5 @@ export default async (cmd: Command) => {
     args.push('--fix');
   }
 
-  try {
-    const result = spawnSync('web-scripts', args, { stdio: 'inherit', shell: true });
-    if (result.error) {
-      throw result.error;
-    }
-    process.exit(result.status ?? 0);
-  } catch (error) {
-    process.stderr.write(`${chalk.red(error.message)}\n`);
-    process.exit(1);
-  }
+  await run('web-scripts', args);
 };

packages/cli/src/commands/plugin/serve/index.ts

@@ -14,14 +14,8 @@
  * limitations under the License.
  */
 
-import chalk from 'chalk';
 import { startDevServer } from './server';
 
 export default async () => {
-  try {
-    await startDevServer();
-  } catch (error) {
-    process.stderr.write(`${chalk.red(error.message)}\n`);
-    process.exit(1);
-  }
+  await startDevServer();
 };

packages/cli/src/commands/plugin/testCommand.ts

@@ -14,9 +14,8 @@
  * limitations under the License.
  */
 
-import chalk from 'chalk';
 import { Command } from 'commander';
-import { spawnSync } from 'child_process';
+import { run } from '../../helpers/run';
 
 export default async (cmd: Command) => {
   const args = ['test'];
@@ -28,14 +27,5 @@ export default async (cmd: Command) => {
     args.push('--coverage');
   }
 
-  try {
-    const result = spawnSync('web-scripts', args, { stdio: 'inherit', shell: true });
-    if (result.error) {
-      throw result.error;
-    }
-    process.exit(result.status ?? 0);
-  } catch (error) {
-    process.stderr.write(`${chalk.red(error.message)}\n`);
-    process.exit(1);
-  }
+  await run('web-scripts', args);
 };

packages/cli/src/commands/watch-deps/child.ts

@@ -34,4 +34,5 @@ export function startChild(args: string[]) {
   child.stderr!.on('data', data => {
     log.err(data.toString('utf8'));
   });
+  return child;
 }

packages/cli/src/commands/watch-deps/index.ts

@@ -22,6 +22,7 @@ import { getPackageDeps } from './packages';
 import { startWatcher, startPackageWatcher } from './watcher';
 import { startCompiler } from './compiler';
 import { startChild } from './child';
+import { waitForExit } from '../../helpers/run';
 
 const PACKAGE_BLACKLIST = [
   // We never want to watch for changes in the cli, but all packages will depend on it.
@@ -67,6 +68,6 @@ export default async (_command: any, args: string[]) => {
   });
 
   if (args?.length) {
-    startChild(args);
+    await waitForExit(startChild(args));
   }
 };

packages/cli/src/helpers/errors.ts

@@ -0,0 +1,46 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import chalk from 'chalk';
+
+export class CustomError extends Error {
+  get name(): string {
+    return this.constructor.name;
+  }
+}
+
+export class ExitCodeError extends CustomError {
+  readonly code: number;
+
+  constructor(code: number, command?: string) {
+    if (command) {
+      super(`Command '${command}' exited with code ${code}`);
+    } else {
+      super(`Child exited with code ${code}`);
+    }
+    this.code = code;
+  }
+}
+
+export function exitWithError(error: Error): never {
+  if (error instanceof ExitCodeError) {
+    process.stderr.write(`${chalk.red(error.message)}\n`);
+    process.exit(error.code);
+  } else {
+    process.stderr.write(`${chalk.red(`${error}`)}\n`);
+    process.exit(1);
+  }
+}

packages/cli/src/helpers/run.ts

@@ -0,0 +1,62 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { SpawnSyncOptions, spawn, ChildProcess } from 'child_process';
+import { ExitCodeError } from './errors';
+
+// Runs a child command, returning a promise that is only resolved if the child exits with code 0.
+export async function run(
+  name: string,
+  args: string[] = [],
+  options: SpawnSyncOptions = {},
+) {
+  const env: NodeJS.ProcessEnv = {
+    ...process.env,
+    FORCE_COLOR: 'true',
+    ...(options.env ?? {}),
+  };
+
+  const child = spawn(name, args, {
+    stdio: 'inherit',
+    shell: true,
+    ...options,
+    env,
+  });
+
+  await waitForExit(child);
+}
+
+export async function waitForExit(
+  child: ChildProcess & { exitCode?: number },
+): Promise<void> {
+  if (typeof child.exitCode === 'number') {
+    if (child.exitCode) {
+      throw new ExitCodeError(child.exitCode, name);
+    }
+    return;
+  }
+
+  await new Promise((resolve, reject) => {
+    child.once('error', error => reject(error));
+    child.once('exit', code => {
+      if (code) {
+        reject(new ExitCodeError(code, name));
+      } else {
+        resolve();
+      }
+    });
+  });
+}

packages/cli/src/index.ts

@@ -23,10 +23,7 @@ import pluginBuild from './commands/plugin/build';
 import pluginLint from './commands/plugin/lint';
 import pluginServe from './commands/plugin/serve';
 import pluginTest from './commands/plugin/testCommand';
-
-process.on('unhandledRejection', err => {
-  throw err;
-});
+import { exitWithError } from './helpers/errors';
 
 const main = (argv: string[]) => {
   const packageJson = JSON.parse(fs.readFileSync('package.json', 'utf-8'));
@@ -36,36 +33,36 @@ const main = (argv: string[]) => {
   program
     .command('create-plugin')
     .description('Creates a new plugin in the current repository')
-    .action(createPluginCommand);
+    .action(actionHandler(createPluginCommand));
 
   program
     .command('plugin:build')
     .option('--watch', 'Enable watch mode')
     .description('Build a plugin')
-    .action(pluginBuild);
+    .action(actionHandler(pluginBuild));
 
   program
     .command('plugin:lint')
     .option('--fix', 'Attempt to automatically fix violations')
     .description('Lint a plugin')
-    .action(pluginLint);
+    .action(actionHandler(pluginLint));
 
   program
     .command('plugin:serve')
     .description('Serves the dev/ folder of a plugin')
-    .action(pluginServe);
+    .action(actionHandler(pluginServe));
 
   program
     .command('plugin:test')
     .option('--watch', 'Enable watch mode')
     .option('--coverage', 'Report test coverage')
     .description('Run all tests for a plugin')
-    .action(pluginTest);
+    .action(actionHandler(pluginTest));
 
   program
     .command('watch-deps')
     .description('Watch all dependencies while running another command')
-    .action(watch);
+    .action(actionHandler(watch));
 
   program.on('command:*', () => {
     console.log();
@@ -84,5 +81,27 @@ const main = (argv: string[]) => {
   program.parse(argv);
 };
 
+// Wraps an action function so that it always exits and handles errors
+function actionHandler<T extends readonly any[]>(
+  actionFunc: (...args: T) => Promise<any>,
+): (...args: T) => Promise<never> {
+  return async (...args: T) => {
+    try {
+      await actionFunc(...args);
+      process.exit(0);
+    } catch (error) {
+      exitWithError(error);
+    }
+  };
+}
+
+process.on('unhandledRejection', rejection => {
+  if (rejection instanceof Error) {
+    exitWithError(rejection);
+  } else {
+    exitWithError(new Error(`Unknown rejection: '${rejection}'`));
+  }
+});
+
 main(process.argv);
 // main([process.argv[0], process.argv[1], '--version']);

scripts/cli-e2e-test.js

@@ -62,6 +62,9 @@ function print(msg) {
 }
 
 async function waitForExit(child) {
+  if (child.exitCode !== null) {
+    throw new Error(`Child already exited with code ${child.exitCode}`);
+  }
   await new Promise((resolve, reject) =>
     child.once('exit', code => {
       if (code) {