Merge pull request #23538 from storybookjs/valentin/proper-error-hand…

…ling-and-logging CLI: Gracefully shutdown and cleanup execa child processes
storybookjs · Jul 21, 2023 · 779b4f6 · 779b4f6
2 parents 7a5394f + 3e6487d
commit 779b4f6
Show file tree

Hide file tree

Showing 8 changed files with 112 additions and 118 deletions.
diff --git a/code/lib/cli/src/generators/baseGenerator.ts b/code/lib/cli/src/generators/baseGenerator.ts
@@ -17,7 +17,6 @@ import {
   extractEslintInfo,
   suggestESLintPlugin,
 } from '../automigrate/helpers/eslintPlugin';
-import { HandledError } from '../HandledError';
 
 const logger = console;
 
@@ -185,34 +184,9 @@ export async function baseGenerator(
   options: FrameworkOptions = defaultOptions,
   framework?: SupportedFrameworks
 ) {
-  // This is added so that we can handle the scenario where the user presses Ctrl+C and report a canceled event.
-  // Given that there are subprocesses running as part of this function, we need to handle the signal ourselves otherwise it might run into race conditions.
-  // TODO: This should be revisited once we have a better way to handle this.
-  let isNodeProcessExiting = false;
-  const setNodeProcessExiting = () => {
-    isNodeProcessExiting = true;
-  };
-  process.on('SIGINT', setNodeProcessExiting);
-
   const isStorybookInMonorepository = packageManager.isStorybookInMonorepo();
   const shouldApplyRequireWrapperOnPackageNames = isStorybookInMonorepository || pnp;
 
-  const stopIfExiting = async <T>(callback: () => Promise<T>) => {
-    if (isNodeProcessExiting) {
-      throw new HandledError('Canceled by the user');
-    }
-
-    try {
-      return await callback();
-    } catch (error) {
-      if (isNodeProcessExiting) {
-        throw new HandledError('Canceled by the user');
-      }
-
-      throw error;
-    }
-  };
-
   const {
     extraAddons: extraAddonPackages,
     extraPackages,
@@ -308,9 +282,7 @@ export async function baseGenerator(
     indent: 2,
     text: `Getting the correct version of ${packages.length} packages`,
   }).start();
-  const versionedPackages = await stopIfExiting(async () =>
-    packageManager.getVersionedPackages(packages)
-  );
+  const versionedPackages = await packageManager.getVersionedPackages(packages);
   versionedPackagesSpinner.succeed();
 
   const depsToInstall = [...versionedPackages];
@@ -369,9 +341,7 @@ export async function baseGenerator(
       indent: 2,
       text: 'Installing Storybook dependencies',
     }).start();
-    await stopIfExiting(async () =>
-      packageManager.addDependencies({ ...npmOptions, packageJson }, depsToInstall)
-    );
+    await packageManager.addDependencies({ ...npmOptions, packageJson }, depsToInstall);
     addDependenciesSpinner.succeed();
   }
 
@@ -429,24 +399,18 @@ export async function baseGenerator(
   });
 
   if (addScripts) {
-    await stopIfExiting(async () =>
-      packageManager.addStorybookCommandInScripts({
-        port: 6006,
-      })
-    );
+    await packageManager.addStorybookCommandInScripts({
+      port: 6006,
+    });
   }
 
   if (addComponents) {
     const templateLocation = hasFrameworkTemplates(framework) ? framework : rendererId;
-    await stopIfExiting(async () =>
-      copyTemplateFiles({
-        renderer: templateLocation,
-        packageManager,
-        language,
-        destination: componentsDestinationPath,
-      })
-    );
+    await copyTemplateFiles({
+      renderer: templateLocation,
+      packageManager,
+      language,
+      destination: componentsDestinationPath,
+    });
   }
-
-  process.off('SIGINT', setNodeProcessExiting);
 }
diff --git a/code/lib/cli/src/initiate.ts b/code/lib/cli/src/initiate.ts
@@ -239,7 +239,18 @@ const projectTypeInquirer = async (
   process.exit(0);
 };
 
-async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<void> {
+async function doInitiate(
+  options: CommandOptions,
+  pkg: PackageJson
+): Promise<
+  | {
+      shouldRunDev: true;
+      projectType: ProjectType;
+      packageManager: JsPackageManager;
+      storybookCommand: string;
+    }
+  | { shouldRunDev: false }
+> {
   let { packageManager: pkgMgr } = options;
   if (options.useNpm) {
     useNpmWarning();
@@ -317,7 +328,7 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
   }
 
   if (!options.disableTelemetry) {
-    telemetry('init', { projectType });
+    await telemetry('init', { projectType });
   }
 
   if (projectType === ProjectType.REACT_NATIVE) {
@@ -330,14 +341,17 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
     logger.log('\n For more in information, see the github readme:\n');
     logger.log(chalk.cyan('https://github.com/storybookjs/react-native'));
     logger.log();
-  } else {
-    const storybookCommand =
-      projectType === ProjectType.ANGULAR
-        ? `ng run ${installResult.projectName}:storybook`
-        : packageManager.getRunStorybookCommand();
-    logger.log(
-      boxen(
-        dedent`
+
+    return { shouldRunDev: false };
+  }
+
+  const storybookCommand =
+    projectType === ProjectType.ANGULAR
+      ? `ng run ${installResult.projectName}:storybook`
+      : packageManager.getRunStorybookCommand();
+  logger.log(
+    boxen(
+      dedent`
           Storybook was successfully installed in your project! 🎉
           To run Storybook manually, run ${chalk.yellow(
             chalk.bold(storybookCommand)
@@ -346,65 +360,64 @@ async function doInitiate(options: CommandOptions, pkg: PackageJson): Promise<vo
           Wanna know more about Storybook? Check out ${chalk.cyan('https://storybook.js.org/')}
           Having trouble or want to chat? Join us at ${chalk.cyan('https://discord.gg/storybook/')}
         `,
-        { borderStyle: 'round', padding: 1, borderColor: '#F1618C' }
-      )
-    );
-
-    const shouldRunDev = process.env.CI !== 'true' && process.env.IN_STORYBOOK_SANDBOX !== 'true';
-    if (shouldRunDev) {
-      logger.log('\nRunning Storybook');
-
-      try {
-        const isReactWebProject =
-          projectType === ProjectType.REACT_SCRIPTS ||
-          projectType === ProjectType.REACT ||
-          projectType === ProjectType.WEBPACK_REACT ||
-          projectType === ProjectType.REACT_PROJECT ||
-          projectType === ProjectType.NEXTJS;
-
-        const flags = [];
-
-        // npm needs extra -- to pass flags to the command
-        if (packageManager.type === 'npm') {
-          flags.push('--');
-        }
-
-        if (isReactWebProject) {
-          flags.push('--initial-path=/onboarding');
-        }
-
-        flags.push('--quiet');
-
-        // instead of calling 'dev' automatically, we spawn a subprocess so that it gets
-        // executed directly in the user's project directory. This avoid potential issues
-        // with packages running in npxs' node_modules
-        packageManager.runPackageCommandSync(
-          storybookCommand.replace(/^yarn /, ''),
-          flags,
-          undefined,
-          'inherit'
-        );
-      } catch (e) {
-        const isCtrlC =
-          e.message.includes('Command failed with exit code 129') &&
-          e.message.includes('CTRL+C') &&
-          e.message.includes('SIGINT');
-        if (!isCtrlC) {
-          // only throw if it's not ctrl + c
-          throw e;
-        }
-      }
-    }
-  }
+      { borderStyle: 'round', padding: 1, borderColor: '#F1618C' }
+    )
+  );
+
+  return {
+    shouldRunDev: process.env.CI !== 'true' && process.env.IN_STORYBOOK_SANDBOX !== 'true',
+    projectType,
+    packageManager,
+    storybookCommand,
+  };
 }
 
 export async function initiate(options: CommandOptions, pkg: PackageJson): Promise<void> {
-  await withTelemetry(
+  const initiateResult = await withTelemetry(
     'init',
     {
       cliOptions: options,
       printError: (err) => !err.handled && logger.error(err),
     },
     () => doInitiate(options, pkg)
   );
+
+  if (initiateResult.shouldRunDev) {
+    const { projectType, packageManager, storybookCommand } = initiateResult;
+    logger.log('\nRunning Storybook');
+
+    try {
+      const isReactWebProject =
+        projectType === ProjectType.REACT_SCRIPTS ||
+        projectType === ProjectType.REACT ||
+        projectType === ProjectType.WEBPACK_REACT ||
+        projectType === ProjectType.REACT_PROJECT ||
+        projectType === ProjectType.NEXTJS;
+
+      const flags = [];
+
+      // npm needs extra -- to pass flags to the command
+      if (packageManager.type === 'npm') {
+        flags.push('--');
+      }
+
+      if (isReactWebProject) {
+        flags.push('--initial-path=/onboarding');
+      }
+
+      flags.push('--quiet');
+
+      // instead of calling 'dev' automatically, we spawn a subprocess so that it gets
+      // executed directly in the user's project directory. This avoid potential issues
+      // with packages running in npxs' node_modules
+      packageManager.runPackageCommandSync(
+        storybookCommand.replace(/^yarn /, ''),
+        flags,
+        undefined,
+        'inherit'
+      );
+    } catch (e) {
+      // Do nothing here, as the command above will spawn a `storybook dev` process which does the error handling already. Else, the error will get bubbled up and sent to crash reports twice
+    }
+  }
 }
diff --git a/code/lib/cli/src/js-package-manager/JsPackageManager.ts b/code/lib/cli/src/js-package-manager/JsPackageManager.ts
@@ -496,6 +496,7 @@ export abstract class JsPackageManager {
         stdio: stdio ?? 'pipe',
         encoding: 'utf-8',
         shell: true,
+        cleanup: true,
         env,
         ...execaOptions,
       });
@@ -529,6 +530,7 @@ export abstract class JsPackageManager {
         stdio: stdio ?? 'pipe',
         encoding: 'utf-8',
         shell: true,
+        cleanup: true,
         env,
         ...execaOptions,
       });

diff --git a/code/lib/cli/src/repro-generators/scripts.ts b/code/lib/cli/src/repro-generators/scripts.ts
@@ -105,7 +105,10 @@ const addLocalPackageResolutions = async ({ cwd }: Options) => {
   const packageJsonPath = path.join(cwd, 'package.json');
   const packageJson = await readJSON(packageJsonPath);
   const workspaceDir = path.join(__dirname, '..', '..', '..', '..', '..');
-  const { stdout } = await command('yarn workspaces list --json', { cwd: workspaceDir });
+  const { stdout } = await command('yarn workspaces list --json', {
+    cwd: workspaceDir,
+    cleanup: true,
+  });
 
   const workspaces = JSON.parse(`[${stdout.split('\n').join(',')}]`);
 

diff --git a/code/lib/core-server/src/withTelemetry.ts b/code/lib/core-server/src/withTelemetry.ts
@@ -109,15 +109,20 @@ export async function withTelemetry<T>(
   options: TelemetryOptions,
   run: () => Promise<T>
 ): Promise<T> {
+  let canceled = false;
+
+  async function cancelTelemetry() {
+    canceled = true;
+    if (!options.cliOptions.disableTelemetry) {
+      await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true });
+    }
+
+    process.exit(0);
+  }
+
   if (eventType === 'init') {
     // We catch Ctrl+C user interactions to be able to detect a cancel event
-    process.on('SIGINT', async () => {
-      if (!options.cliOptions.disableTelemetry) {
-        await telemetry('canceled', { eventType }, { stripMetadata: true, immediate: true });
-      }
-
-      process.exit(0);
-    });
+    process.on('SIGINT', cancelTelemetry);
   }
 
   if (!options.cliOptions.disableTelemetry)
@@ -126,7 +131,7 @@ export async function withTelemetry<T>(
   try {
     return await run();
   } catch (error) {
-    if (error?.message === 'Canceled by the user') {
+    if (canceled) {
       return undefined;
     }
 
@@ -135,5 +140,7 @@ export async function withTelemetry<T>(
     await sendTelemetryError(error, eventType, options);
 
     throw error;
+  } finally {
+    process.off('SIGINIT', cancelTelemetry);
   }
 }
diff --git a/scripts/build-package.js b/scripts/build-package.js
@@ -132,6 +132,7 @@ async function run() {
         cwd,
         buffer: false,
         shell: true,
+        cleanup: true,
         env: {
           NODE_ENV: 'production',
         },

diff --git a/scripts/check-package.js b/scripts/check-package.js
@@ -103,6 +103,7 @@ async function run() {
       cwd,
       buffer: false,
       shell: true,
+      cleanup: true,
       env: {
         NODE_ENV: 'production',
       },

diff --git a/scripts/utils/exec.ts b/scripts/utils/exec.ts
@@ -26,7 +26,10 @@ export const execaCommand = async (
   const execa = await getExeca();
   // We await here because execaCommand returns a promise, but that's not what the user expects
   // eslint-disable-next-line @typescript-eslint/return-await
-  return await execa.execaCommand(command, options);
+  return await execa.execaCommand(command, {
+    cleanup: true,
+    ...options,
+  });
 };
 
 export const exec = async (