ref(clack-utils): Use child_process spawn instead of exec when installing package (#859)

Lms24 · andreiborza · web-flow · commit c2c62ddf858e · 2025-03-18T11:11:35.000+01:00
Fixes #851 - child_process spawn instead of exec - pipe std streams to fix #851 but ignore stdout - send stacktrace-less error to sentry if install fails --------- Co-authored-by: Andrei Borza <andrei.borza@sentry.io>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3,7 +3,8 @@
 ## Unreleased
 
 - feat(nextjs): Remove react component annotation prompt and insertion ([#858](https://github.com/getsentry/sentry-wizard/pull/858))
-- Prevent addition of multiple `sentry:sourcemaps` commands ([#840](https://github.com/getsentry/sentry-wizard/pull/840))
+- fix: Prevent addition of multiple `sentry:sourcemaps` commands ([#840](https://github.com/getsentry/sentry-wizard/pull/840))
+- ref(clack-utils): Use child_process spawn instead of exec when to install package ([#859](https://github.com/getsentry/sentry-wizard/pull/859))
 
 ## 4.4.0
 
diff --git a/src/utils/clack-utils.ts b/src/utils/clack-utils.ts
@@ -332,6 +332,18 @@ export async function confirmContinueIfPackageVersionNotSupported({
   });
 }
 
+type InstallPackageOptions = {
+  /** The string that is passed to the package manager CLI as identifier to install (e.g. `@sentry/nextjs`, or `@sentry/nextjs@^8`) */
+  packageName: string;
+  alreadyInstalled: boolean;
+  askBeforeUpdating?: boolean;
+  /** Overrides what is shown in the installation logs in place of the `packageName` option. Useful if the `packageName` is ugly (e.g. `@sentry/nextjs@^8`) */
+  packageNameDisplayLabel?: string;
+  packageManager?: PackageManager;
+  /** Add force install flag to command to skip install precondition fails */
+  forceInstall?: boolean;
+};
+
 /**
  * Installs or updates a package with the user's package manager.
  *
@@ -345,17 +357,7 @@ export async function installPackage({
   packageNameDisplayLabel,
   packageManager,
   forceInstall = false,
-}: {
-  /** The string that is passed to the package manager CLI as identifier to install (e.g. `@sentry/nextjs`, or `@sentry/nextjs@^8`) */
-  packageName: string;
-  alreadyInstalled: boolean;
-  askBeforeUpdating?: boolean;
-  /** Overrides what is shown in the installation logs in place of the `packageName` option. Useful if the `packageName` is ugly (e.g. `@sentry/nextjs@^8`) */
-  packageNameDisplayLabel?: string;
-  packageManager?: PackageManager;
-  /** Add force install flag to command to skip install precondition fails */
-  forceInstall?: boolean;
-}): Promise<{ packageManager?: PackageManager }> {
+}: InstallPackageOptions): Promise<{ packageManager?: PackageManager }> {
   return traceStep('install-package', async () => {
     if (alreadyInstalled && askBeforeUpdating) {
       const shouldUpdatePackage = await abortIfCancelled(
@@ -383,31 +385,81 @@ export async function installPackage({
 
     try {
       await new Promise<void>((resolve, reject) => {
-        childProcess.exec(
-          `${pkgManager.installCommand} ${packageName} ${pkgManager.flags} ${
-            forceInstall ? pkgManager.forceInstallFlag : ''
-          }`,
-          (err, stdout, stderr) => {
-            if (err) {
-              // Write a log file so we can better troubleshoot issues
-              fs.writeFileSync(
-                join(
-                  process.cwd(),
-                  `sentry-wizard-installation-error-${Date.now()}.log`,
-                ),
-                JSON.stringify({
-                  stdout,
-                  stderr,
-                }),
-                { encoding: 'utf8' },
-              );
+        const installArgs = [
+          pkgManager.installCommand,
+          packageName,
+          ...(pkgManager.flags ? pkgManager.flags.split(' ') : []),
+          ...(forceInstall ? [pkgManager.forceInstallFlag] : []),
+        ];
+
+        const stringifiedInstallCmd = `${pkgManager.name} ${installArgs.join(
+          ' ',
+        )}`;
+
+        function handleErrorAndReject(
+          code: number | null,
+          cause: Error | string,
+          type: 'spawn_error' | 'process_error',
+        ) {
+          // Write a log file so we can better troubleshoot issues
+          fs.writeFileSync(
+            join(
+              process.cwd(),
+              `sentry-wizard-installation-error-${Date.now()}.log`,
+            ),
+            JSON.stringify(stderr, null, 2),
+            { encoding: 'utf8' },
+          );
+
+          Sentry.captureException('Package Installation Error', {
+            tags: {
+              'install-command': stringifiedInstallCmd,
+              'package-manager': pkgManager.name,
+              'package-name': packageName,
+              'error-type': type,
+            },
+          });
+
+          reject(
+            new Error(
+              `Installation command ${chalk.cyan(
+                stringifiedInstallCmd,
+              )} exited with code ${code}.`,
+              {
+                cause,
+              },
+            ),
+          );
+        }
 
-              reject(err);
-            } else {
-              resolve();
-            }
+        const installProcess = childProcess.spawn(
+          pkgManager.name,
+          installArgs,
+          {
+            shell: true,
+            // Ignoring `stdout` to prevent certain node + yarn v4 (observed on ubuntu + snap)
+            // combinations from crashing here. See #851
+            stdio: ['pipe', 'ignore', 'pipe'],
           },
         );
+
+        let stderr = '';
+
+        installProcess.stderr.on('data', (data) => {
+          stderr += data.toString();
+        });
+
+        installProcess.on('error', (err) => {
+          handleErrorAndReject(null, err, 'spawn_error');
+        });
+
+        installProcess.on('close', (code) => {
+          if (code !== 0) {
+            handleErrorAndReject(code, stderr, 'process_error');
+          } else {
+            resolve();
+          }
+        });
       });
     } catch (e) {
       sdkInstallSpinner.stop('Installation failed.');
diff --git a/src/utils/package-manager.ts b/src/utils/package-manager.ts
@@ -1,6 +1,5 @@
-/* eslint-disable @typescript-eslint/typedef */
-import * as fs from 'fs';
-import * as path from 'path';
+import * as fs from 'node:fs';
+import * as path from 'node:path';
 
 import * as Sentry from '@sentry/node';
 import { traceStep } from '../telemetry';
@@ -22,7 +21,7 @@ export interface PackageManager {
 export const BUN: PackageManager = {
   name: 'bun',
   label: 'Bun',
-  installCommand: 'bun add',
+  installCommand: 'add',
   buildCommand: 'bun run build',
   runScriptCommand: 'bun run',
   flags: '',
@@ -47,7 +46,7 @@ export const BUN: PackageManager = {
 export const YARN_V1: PackageManager = {
   name: 'yarn',
   label: 'Yarn V1',
-  installCommand: 'yarn add',
+  installCommand: 'add',
   buildCommand: 'yarn build',
   runScriptCommand: 'yarn',
   flags: '--ignore-workspace-root-check',
@@ -79,7 +78,7 @@ export const YARN_V1: PackageManager = {
 export const YARN_V2: PackageManager = {
   name: 'yarn',
   label: 'Yarn V2/3/4',
-  installCommand: 'yarn add',
+  installCommand: 'add',
   buildCommand: 'yarn build',
   runScriptCommand: 'yarn',
   flags: '',
@@ -110,7 +109,7 @@ export const YARN_V2: PackageManager = {
 export const PNPM: PackageManager = {
   name: 'pnpm',
   label: 'PNPM',
-  installCommand: 'pnpm add',
+  installCommand: 'add',
   buildCommand: 'pnpm build',
   runScriptCommand: 'pnpm',
   flags: '--ignore-workspace-root-check',
@@ -136,7 +135,7 @@ export const PNPM: PackageManager = {
 export const NPM: PackageManager = {
   name: 'npm',
   label: 'NPM',
-  installCommand: 'npm add',
+  installCommand: 'install',
   buildCommand: 'npm run build',
   runScriptCommand: 'npm run',
   flags: '',
diff --git a/test/utils/clack-utils.test.ts b/test/utils/clack-utils.test.ts
@@ -157,11 +157,25 @@ describe('installPackage', () => {
     jest.clearAllMocks();
   });
 
+  const spawnSpy = jest
+    .spyOn(ChildProcess, 'spawn')
+    // @ts-expect-error - ignoring complete typing
+    .mockImplementation(() => ({
+      // @ts-expect-error - not passing the full object but directly resolving
+      // to simulate a successful install
+      on: jest.fn((evt: 'close', cb: (args) => void) => {
+        if (evt === 'close') {
+          cb(0);
+        }
+      }),
+      stderr: { on: jest.fn() },
+    }));
+
   it('force-installs a package if the forceInstall flag is set', async () => {
     const packageManagerMock: PackageManager = {
       name: 'npm',
       label: 'NPM',
-      installCommand: 'npm install',
+      installCommand: 'install',
       buildCommand: 'npm run build',
       runScriptCommand: 'npm run',
       flags: '',
@@ -170,28 +184,19 @@ describe('installPackage', () => {
       addOverride: jest.fn(),
     };
 
-    const execSpy = jest
-      .spyOn(ChildProcess, 'exec')
-      // @ts-expect-error - don't care about the return value
-      .mockImplementationOnce((cmd, cb) => {
-        if (cb) {
-          // @ts-expect-error - don't care about the options value
-          cb(null, '', '');
-        }
-      });
-
     await installPackage({
       alreadyInstalled: false,
-      packageName: '@sentry/sveltekit',
-      packageNameDisplayLabel: '@sentry/sveltekit',
+      packageName: '@some/package',
+      packageNameDisplayLabel: '@some/package',
       forceInstall: true,
       askBeforeUpdating: false,
       packageManager: packageManagerMock,
     });
 
-    expect(execSpy).toHaveBeenCalledWith(
-      'npm install @sentry/sveltekit  --force',
-      expect.any(Function),
+    expect(spawnSpy).toHaveBeenCalledWith(
+      'npm',
+      ['install', '@some/package', '--force'],
+      { shell: true, stdio: ['pipe', 'ignore', 'pipe'] },
     );
   });
 
@@ -201,7 +206,7 @@ describe('installPackage', () => {
       const packageManagerMock: PackageManager = {
         name: 'npm',
         label: 'NPM',
-        installCommand: 'npm install',
+        installCommand: 'install',
         buildCommand: 'npm run build',
         runScriptCommand: 'npm run',
         flags: '',
@@ -210,16 +215,6 @@ describe('installPackage', () => {
         addOverride: jest.fn(),
       };
 
-      const execSpy = jest
-        .spyOn(ChildProcess, 'exec')
-        // @ts-expect-error - don't care about the return value
-        .mockImplementationOnce((cmd, cb) => {
-          if (cb) {
-            // @ts-expect-error - don't care about the options value
-            cb(null, '', '');
-          }
-        });
-
       await installPackage({
         alreadyInstalled: false,
         packageName: '@sentry/sveltekit',
@@ -229,12 +224,43 @@ describe('installPackage', () => {
         packageManager: packageManagerMock,
       });
 
-      expect(execSpy).toHaveBeenCalledWith(
-        'npm install @sentry/sveltekit  ',
-        expect.any(Function),
+      expect(spawnSpy).toHaveBeenCalledWith(
+        'npm',
+        ['install', '@sentry/sveltekit'],
+        { shell: true, stdio: ['pipe', 'ignore', 'pipe'] },
       );
     },
   );
+
+  it('adds install flags if defined', async () => {
+    const packageManagerMock: PackageManager = {
+      name: 'npm',
+      label: 'NPM',
+      installCommand: 'install',
+      buildCommand: 'npm run build',
+      runScriptCommand: 'npm run',
+      flags: '--ignore-workspace-root-check',
+      forceInstallFlag: '--force',
+      detect: jest.fn(),
+      addOverride: jest.fn(),
+    };
+
+    await installPackage({
+      alreadyInstalled: false,
+      packageName: '@some/package',
+      packageNameDisplayLabel: '@some/package',
+      forceInstall: true,
+      askBeforeUpdating: false,
+      packageManager: packageManagerMock,
+    });
+
+    expect(spawnSpy).toHaveBeenCalledWith(
+      'npm',
+
+      ['install', '@some/package', '--ignore-workspace-root-check', '--force'],
+      { shell: true, stdio: ['pipe', 'ignore', 'pipe'] },
+    );
+  });
 });
 
 describe('askForWizardLogin', () => {
