feat(js): Add live: 'rejectOnError' execution mode to execute method (#2605)

The Sentry CLI JS interface's `execute` method takes a `live` parameter
as a second argument:
- if `false`, stdio is returned and the promise returned from `execute`
rejects/resolves based on the CLI binary's exit code.
- if `true`, CLI stdio is piped to the process calling `execute`, but
the returned promise always resolves regardless of the exit code (raised
in #2062)

This feature adds a third value (`'rejectOnError'`) to `live`, which still
pipes stdio but also rejects if the CLI binary's exit code was non-zero.
Arguably, this is what `true` should do but to avoid a breaking change
for now, let's introduce this as a new value instead.

Author: Lms24

js/__tests__/helper.test.js

@@ -22,6 +22,30 @@ describe('SentryCli helper', () => {
     expect(helper.getPath()).toMatch(pattern);
   });
 
+  describe('execute', () => {
+    test('execute with live=false returns stdout', async () => {
+      const output = await helper.execute(['--version'], false);
+      expect(output.trim()).toBe('sentry-cli DEV');
+    });
+
+    test('execute with live=true resolves without output', async () => {
+      // TODO (v3): This should resolve with a string, not undefined/void
+      const result = await helper.execute(['--version'], true);
+      expect(result).toBeUndefined();
+    });
+
+    test('execute with live=rejectOnError resolves on success', async () => {
+      const result = await helper.execute(['--version'], 'rejectOnError');
+      expect(result).toBe('success (live mode)');
+    });
+
+    test('execute with live=rejectOnError rejects on failure', async () => {
+      await expect(helper.execute(['fail'], 'rejectOnError')).rejects.toThrow(
+        'Command fail failed with exit code 1'
+      );
+    });
+  });
+
   describe('`prepare` command', () => {
     test('call prepare command add default ignore', () => {
       const command = ['releases', 'files', 'release', 'upload-sourcemaps', '/dev/null'];

js/helper.js

@@ -281,7 +281,11 @@ function getPath() {
  * expect(output.trim()).toBe('sentry-cli x.y.z');
  *
  * @param {string[]} args Command line arguments passed to `sentry-cli`.
- * @param {boolean} live We inherit stdio to display `sentry-cli` output directly.
+ * @param {boolean | 'rejectOnError'} live can be set to:
+ *  - `true` to inherit stdio to display `sentry-cli` output directly.
+ *  - `false` to not inherit stdio and return the output as a string.
+ *  - `'rejectOnError'` to inherit stdio and reject the promise if the command
+ *    exits with a non-zero exit code.
  * @param {boolean} silent Disable stdout for silents build (CI/Webpack Stats, ...)
  * @param {string} [configFile] Relative or absolute path to the configuration file.
  * @param {Object} [config] More configuration to pass to the CLI
@@ -322,16 +326,27 @@ async function execute(args, live, silent, configFile, config = {}) {
     ]);
     args = [...headers, ...args];
   }
+
   return new Promise((resolve, reject) => {
-    if (live === true) {
+    if (live === true || live === 'rejectOnError') {
       const output = silent ? 'ignore' : 'inherit';
       const pid = childProcess.spawn(getPath(), args, {
         env,
         // stdin, stdout, stderr
         stdio: ['ignore', output, output],
       });
-      pid.on('exit', () => {
-        // @ts-expect-error - this is a TODO (v3) to fix and resolve a string here
+      pid.on('exit', (exitCode) => {
+        if (live === 'rejectOnError') {
+          if (exitCode === 0) {
+            resolve('success (live mode)');
+          }
+          reject(new Error(`Command ${args.join(' ')} failed with exit code ${exitCode}`));
+        }
+        // According to the type definition, resolving with void is not allowed.
+        // However, for backwards compatibility, we resolve void here to
+        // avoid a behaviour-breaking change.
+        // TODO (v3): Clean this up and always resolve a string (or change the type definition)
+        // @ts-expect-error - see comment above
         resolve();
       });
     } else {

js/releases/__tests__/index.test.js

@@ -159,6 +159,25 @@ describe('SentryCli releases', () => {
           { silent: false }
         );
       });
+
+      test.each([true, false, 'rejectOnError'])('handles live mode %s', async (live) => {
+        await cli.releases.uploadSourceMaps('my-version', { include: ['path'], live });
+        expect(mockExecute).toHaveBeenCalledWith(
+          [
+            'releases',
+            'files',
+            'my-version',
+            'upload-sourcemaps',
+            'path',
+            '--ignore',
+            'node_modules',
+          ],
+          live,
+          false,
+          undefined,
+          { silent: false }
+        );
+      });
     });
   });
 });

js/releases/index.js

@@ -199,7 +199,10 @@ class Releases {
 
       return uploadPaths.map((path) =>
         // `execute()` is async and thus we're returning a promise here
-        this.execute(helper.prepareCommand([...args, path], SOURCEMAPS_SCHEMA, newOptions), true)
+        this.execute(
+          helper.prepareCommand([...args, path], SOURCEMAPS_SCHEMA, newOptions),
+          options.live != null ? options.live : true
+        )
       );
     });
 
@@ -255,7 +258,11 @@ class Releases {
   /**
    * See {helper.execute} docs.
    * @param {string[]} args Command line arguments passed to `sentry-cli`.
-   * @param {boolean} live We inherit stdio to display `sentry-cli` output directly.
+   * @param {boolean | 'rejectOnError'} live can be set to:
+   *  - `true` to inherit stdio to display `sentry-cli` output directly.
+   *  - `false` to not inherit stdio and return the output as a string.
+   *  - `'rejectOnError'` to inherit stdio and reject the promise if the command
+   *    exits with a non-zero exit code.
    * @returns {Promise.<string>} A promise that resolves to the standard output.
    */
   async execute(args, live) {