feat: better error handling in SfCommand.catch

Author: shetzel

src/SfCommandError.ts

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2023, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { SfError, StructuredMessage } from '@salesforce/core';
+import { AnyJson } from '@salesforce/ts-types';
+import { computeErrorCode } from './errorHandling.js';
+import { removeEmpty } from './util.js';
+
+// These types are 90% the same as SfErrorOptions (but they aren't exported to extend)
+type ErrorDataProperties = AnyJson;
+export type SfCommandErrorOptions<T extends ErrorDataProperties = ErrorDataProperties> = {
+  message: string;
+  exitCode: number;
+  code: string;
+  name?: string;
+  commandName: string;
+  data?: T;
+  cause?: unknown;
+  context?: string;
+  actions?: string[];
+  result?: unknown;
+  warnings?: Array<StructuredMessage | string>;
+};
+
+type SfCommandErrorJson = {
+  name: string;
+  message: string;
+  exitCode: number;
+  commandName: string;
+  context: string;
+  code: string;
+  status: string;
+  stack: string;
+  actions?: string;
+  data?: ErrorDataProperties;
+  cause?: string;
+  warnings?: Array<StructuredMessage | string>;
+  result?: unknown;
+};
+
+export class SfCommandError extends SfError {
+  public status: number;
+  public commandName: string;
+  public warnings?: Array<StructuredMessage | string>;
+  public result?: unknown;
+  public skipOclifErrorHandling: boolean;
+  public oclif: { exit: number };
+
+  /**
+   * SfCommandError is meant to wrap errors from `SfCommand.catch()` for a common
+   * error format to be logged, sent to telemetry, and re-thrown. Do not create
+   * instances from the constructor.  Call the static method, `SfCommandError.from()`
+   * and use the returned `SfCommandError`.
+   */
+  private constructor(input: SfCommandErrorOptions) {
+    super(input.message, input.name, input.actions, input.exitCode, input.cause);
+    this.data = input.data;
+    this.status = input.exitCode;
+    this.warnings = input.warnings;
+    this.skipOclifErrorHandling = true;
+    this.commandName = input.commandName;
+    this.code = input.code;
+    this.result = input.result;
+    this.oclif = { exit: input.exitCode };
+    this.context = input.context ?? input.commandName;
+  }
+
+  public static from(
+    err: Error | SfError | SfCommandError,
+    commandName: string,
+    warnings?: Array<StructuredMessage | string>
+  ): SfCommandError {
+    // SfError.wrap() does most of what we want so start with that.
+    const sfError = SfError.wrap(err);
+    const exitCode = computeErrorCode(sfError);
+    sfError.code = 'code' in err ? err.code : exitCode.toString(10);
+    return new this({
+      message: sfError.message,
+      name: err.name ?? 'Error',
+      actions: 'actions' in err ? err.actions : undefined,
+      exitCode,
+      code: sfError.code,
+      cause: sfError.cause,
+      commandName: 'commandName' in err ? err.commandName : commandName,
+      data: 'data' in err ? err.data : undefined,
+      result: 'result' in err ? err.result : undefined,
+      context: 'context' in err ? err.context : commandName,
+      warnings,
+    });
+  }
+
+  public toJson(): SfCommandErrorJson {
+    return {
+      ...removeEmpty({
+        // toObject() returns name, message, exitCode, actions, context, data
+        ...this.toObject(),
+        stack: this.stack,
+        cause: this.cause,
+        warnings: this.warnings,
+        code: this.code,
+        status: this.status,
+        commandName: this.commandName,
+        result: this.result,
+      }),
+    } as SfCommandErrorJson;
+  }
+}

src/errorFormatting.ts

@@ -5,10 +5,11 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 
-import { Mode, Messages, envVars } from '@salesforce/core';
+import { inspect } from 'node:util';
 import type { Ansis } from 'ansis';
+import { Mode, Messages, envVars } from '@salesforce/core';
 import { StandardColors } from './ux/standardColors.js';
-import { SfCommandError } from './types.js';
+import { SfCommandError } from './SfCommandError.js';
 
 Messages.importMessagesDirectoryFromMetaUrl(import.meta.url);
 const messages = Messages.loadMessages('@salesforce/sf-plugins-core', 'messages');
@@ -45,7 +46,7 @@ export const formatError = (error: SfCommandError): string =>
     `${formatErrorPrefix(error)} ${error.message}`,
     ...formatActions(error.actions ?? []),
     error.stack && envVars.getString('SF_ENV') === Mode.DEVELOPMENT
-      ? StandardColors.info(`\n*** Internal Diagnostic ***\n\n${error.stack}\n******\n`)
+      ? StandardColors.info(`\n*** Internal Diagnostic ***\n\n${inspect(error)}\n******\n`)
       : [],
   ].join('\n');
 

src/errorHandling.ts

@@ -7,8 +7,7 @@
 
 import { SfError } from '@salesforce/core';
 import { CLIError } from '@oclif/core/errors';
-import { SfCommandError } from './types.js';
-import { removeEmpty } from './util.js';
+import { SfCommandError } from './SfCommandError.js';
 
 /**
  *
@@ -66,28 +65,6 @@ export const errorIsTypeError = (error: Error | SfError): boolean =>
   Boolean(error.stack?.includes('TypeError')) ||
   ('cause' in error && error.cause instanceof Error && errorIsTypeError(error.cause));
 
-export const errorToSfCommandError = (
-  codeFromError: number,
-  error: Error | SfError | SfCommandError,
-  commandName: string
-): SfCommandError => ({
-  ...removeEmpty({
-    code: codeFromError,
-    actions: 'actions' in error ? error.actions : null,
-    context: ('context' in error ? error.context : commandName) ?? commandName,
-    commandName: ('commandName' in error ? error.commandName : commandName) ?? commandName,
-    data: 'data' in error ? error.data : null,
-    result: 'result' in error ? error.result : null,
-  }),
-  ...{
-    message: error.message,
-    name: error.name ?? 'Error',
-    status: codeFromError,
-    stack: error.stack,
-    exitCode: codeFromError,
-  },
-});
-
 /** custom typeGuard for handling the fact the SfCommand doesn't know about oclif error structure */
 const isOclifError = <T extends Error | SfError | SfCommandError>(e: T): e is T & CLIError =>
   'oclif' in e ? true : false;

src/sfCommand.ts

@@ -20,11 +20,10 @@ import type { AnyJson } from '@salesforce/ts-types';
 import { Progress } from './ux/progress.js';
 import { Spinner } from './ux/spinner.js';
 import { Ux } from './ux/ux.js';
-import { SfCommandError } from './types.js';
+import { SfCommandError } from './SfCommandError.js';
 import { formatActions, formatError } from './errorFormatting.js';
 import { StandardColors } from './ux/standardColors.js';
 import { confirm, secretPrompt, PromptInputs } from './ux/prompts.js';
-import { computeErrorCode, errorToSfCommandError } from './errorHandling.js';
 
 Messages.importMessagesDirectoryFromMetaUrl(import.meta.url);
 const messages = Messages.loadMessages('@salesforce/sf-plugins-core', 'messages');
@@ -201,7 +200,7 @@ export abstract class SfCommand<T> extends Command {
 
     const colorizedArgs = [
       `${StandardColors.warning(messages.getMessage('warning.prefix'))} ${message}`,
-      ...formatActions(typeof input === 'string' ? [] : input.actions ?? [], { actionColor: StandardColors.info }),
+      ...formatActions(typeof input === 'string' ? [] : input.actions ?? []),
     ];
 
     this.logToStderr(colorizedArgs.join(os.EOL));
@@ -216,10 +215,9 @@ export abstract class SfCommand<T> extends Command {
   public info(input: SfCommand.Info): void {
     const message = typeof input === 'string' ? input : input.message;
     this.log(
-      [
-        `${StandardColors.info(message)}`,
-        ...formatActions(typeof input === 'string' ? [] : input.actions ?? [], { actionColor: StandardColors.info }),
-      ].join(os.EOL)
+      [`${StandardColors.info(message)}`, ...formatActions(typeof input === 'string' ? [] : input.actions ?? [])].join(
+        os.EOL
+      )
     );
   }
 
@@ -368,66 +366,27 @@ export abstract class SfCommand<T> extends Command {
     };
   }
 
-  /**
-   * Wrap the command error into the standardized JSON structure.
-   */
-  protected toErrorJson(error: SfCommand.Error): SfCommand.Error {
-    return {
-      ...error,
-      warnings: this.warnings,
-    };
-  }
-
   // eslint-disable-next-line @typescript-eslint/require-await
   protected async catch(error: Error | SfError | SfCommand.Error): Promise<never> {
     // stop any spinners to prevent it from unintentionally swallowing output.
     // If there is an active spinner, it'll say "Error" instead of "Done"
     this.spinner.stop(StandardColors.error('Error'));
-    // transform an unknown error into one that conforms to the interface
 
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
-    // const codeFromError = (error.oclif?.exit as number | undefined) ?? (error.exitCode as number | undefined) ?? 1;
-    const codeFromError = computeErrorCode(error);
-    process.exitCode = codeFromError;
-
-    const sfCommandError = errorToSfCommandError(codeFromError, error, this.statics.name);
+    // transform an unknown error into a SfCommandError
+    const sfCommandError = SfCommandError.from(error, this.statics.name, this.warnings);
+    process.exitCode = sfCommandError.exitCode;
 
     if (this.jsonEnabled()) {
-      this.logJson(this.toErrorJson(sfCommandError));
+      this.logJson(sfCommandError.toJson());
     } else {
       this.logToStderr(formatError(sfCommandError));
     }
 
-    // Create SfError that can be thrown
-    const err = new SfError(
-      error.message,
-      error.name ?? 'Error',
-      // @ts-expect-error because actions is not on Error
-      (error.actions as string[]) ?? [],
-      process.exitCode
-    );
-    if (sfCommandError.data) {
-      err.data = sfCommandError.data as AnyJson;
-    }
-    err.context = sfCommandError.context;
-    err.stack = sfCommandError.stack;
-    // @ts-expect-error because code is not on SfError
-    err.code = codeFromError;
-    // @ts-expect-error because status is not on SfError
-    err.status = sfCommandError.status;
-
-    // @ts-expect-error because skipOclifErrorHandling is not on SfError
-    err.skipOclifErrorHandling = true;
-
-    // Add oclif exit code to the error so that oclif can use the exit code when exiting.
-    // @ts-expect-error because oclif is not on SfError
-    err.oclif = { exit: process.exitCode };
-
     // Emit an event for plugin-telemetry prerun hook to pick up.
     // @ts-expect-error because TS is strict about the events that can be emitted on process.
-    process.emit('sfCommandError', err, this.id);
+    process.emit('sfCommandError', sfCommandError, this.id);
 
-    throw err;
+    throw sfCommandError;
   }
 
   public abstract run(): Promise<T>;

src/types.ts

@@ -0,0 +1,82 @@
+/*
+ * Copyright (c) 2023, salesforce.com, inc.
+ * All rights reserved.
+ * Licensed under the BSD 3-Clause license.
+ * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
+ */
+import { expect } from 'chai';
+import { Mode, SfError } from '@salesforce/core';
+import { formatError } from '../../src/errorFormatting.js';
+import { SfCommandError } from '../../src/SfCommandError.js';
+
+describe('errorFormatting.formatError()', () => {
+  afterEach(() => {
+    delete process.env.SF_ENV;
+  });
+
+  it('should have correct output for non-development mode, no actions', () => {
+    const err = SfCommandError.from(new Error('this did not work'), 'thecommand');
+    const errorOutput = formatError(err);
+    expect(errorOutput).to.contain('Error (1)');
+    expect(errorOutput).to.contain('this did not work');
+  });
+
+  it('should have correct output for non-development mode, with actions', () => {
+    const sfError = new SfError('this did not work', 'BadError');
+    const err = SfCommandError.from(sfError, 'thecommand');
+    err.actions = ['action1', 'action2'];
+    const errorOutput = formatError(err);
+    expect(errorOutput).to.contain('Error (BadError)');
+    expect(errorOutput).to.contain('this did not work');
+    expect(errorOutput).to.contain('Try this:');
+    expect(errorOutput).to.contain('action1');
+    expect(errorOutput).to.contain('action2');
+  });
+
+  it('should have correct output for development mode, basic error', () => {
+    process.env.SF_ENV = Mode.DEVELOPMENT;
+    const err = SfCommandError.from(new SfError('this did not work'), 'thecommand');
+    const errorOutput = formatError(err);
+    expect(errorOutput).to.contain('Error (SfError)');
+    expect(errorOutput).to.contain('this did not work');
+    expect(errorOutput).to.contain('*** Internal Diagnostic ***');
+    expect(errorOutput).to.contain('at Function.from');
+    expect(errorOutput).to.contain('actions: undefined');
+    expect(errorOutput).to.contain('exitCode: 1');
+    expect(errorOutput).to.contain("context: 'thecommand'");
+    expect(errorOutput).to.contain('data: undefined');
+    expect(errorOutput).to.contain('cause: undefined');
+    expect(errorOutput).to.contain('status: 1');
+    expect(errorOutput).to.contain("commandName: 'thecommand'");
+    expect(errorOutput).to.contain('warnings: undefined');
+    expect(errorOutput).to.contain('result: undefined');
+  });
+
+  it('should have correct output for development mode, full error', () => {
+    process.env.SF_ENV = Mode.DEVELOPMENT;
+    const sfError = SfError.create({
+      name: 'WOMP_WOMP',
+      message: 'this did not work',
+      actions: ['action1', 'action2'],
+      cause: new Error('this is the cause'),
+      exitCode: 9,
+      context: 'somecommand',
+      data: { foo: 'bar' },
+    });
+    const err = SfCommandError.from(sfError, 'thecommand');
+    const errorOutput = formatError(err);
+    expect(errorOutput).to.contain('Error (WOMP_WOMP)');
+    expect(errorOutput).to.contain('this did not work');
+    expect(errorOutput).to.contain('*** Internal Diagnostic ***');
+    expect(errorOutput).to.contain('at Function.from');
+    expect(errorOutput).to.contain("actions: [ 'action1', 'action2' ]");
+    expect(errorOutput).to.contain('exitCode: 9');
+    expect(errorOutput).to.contain("context: 'somecommand'");
+    expect(errorOutput).to.contain("data: { foo: 'bar' }");
+    expect(errorOutput).to.contain('cause: Error: this is the cause');
+    expect(errorOutput).to.contain('status: 9');
+    expect(errorOutput).to.contain("commandName: 'thecommand'");
+    expect(errorOutput).to.contain('warnings: undefined');
+    expect(errorOutput).to.contain('result: undefined');
+  });
+});