feat: improved sfError #1046

mshanemc · 2024-04-04T16:12:53Z

What does this PR do?

bump tsconfig target/lib/etc for es2022 so we get native error.cause
sfError extends Error instead of kit/NamedError
- adds the name property
- adds the fullStack method...which now operates on stack of error.cause by recursing through them to build its "caused by"
- add a new .create method that's less painful than the 5-prop overloaded constructor and which supports the context and data properties
- wrap method takes an unknown so you can wrap things in a catch block without having to worry about their type (previously only supported string | Error | SfError). Will wrap any "other" things in an error to preserve its info in the new error's cause
- the constructor (arg5) and the new .create also allow cause to be unknown and will narrow it to an error (and otherwise throw) for that same convenience in catch blocks.
- code property always returns a string [it's either code (which can only be a string) or name (also required, string)] and now the types say that

What issues does this PR fix or reference?

@W-14507125@

mshanemc · 2024-04-04T16:27:49Z

src/sfError.ts

 import { AnyJson, hasString, isString, JsonMap } from '@salesforce/ts-types';

+export type SfErrorOptions<T extends ErrorDataProperties = ErrorDataProperties> = {


type for the new .create method param obj

mshanemc · 2024-04-04T16:28:30Z

src/sfError.ts

+  actions?: string[];
+};
+
+type ErrorDataProperties = AnyJson;


constrain the data Prop...see the "DANGER" note on the original's line 145

mshanemc · 2024-04-04T16:29:12Z

src/sfError.ts

@@ -24,7 +35,8 @@ import { AnyJson, hasString, isString, JsonMap } from '@salesforce/ts-types';
 * throw new SfError(message.getMessage('myError'), 'MyErrorName');
 * ```
 */
-export class SfError<T = unknown> extends NamedError {
+export class SfError<T extends ErrorDataProperties = ErrorDataProperties> extends Error {


leave NamedError out of this

mshanemc · 2024-04-04T16:29:28Z

src/sfError.ts

@@ -24,7 +35,8 @@ import { AnyJson, hasString, isString, JsonMap } from '@salesforce/ts-types';
 * throw new SfError(message.getMessage('myError'), 'MyErrorName');
 * ```
 */
-export class SfError<T = unknown> extends NamedError {
+export class SfError<T extends ErrorDataProperties = ErrorDataProperties> extends Error {
+  public readonly name: string;


brought from NamedError

src/sfError.ts

mshanemc · 2024-04-04T16:33:09Z

src/sfError.ts

  /**
   * Convert an Error to an SfError.
   *
   * @param err The error to convert.
   */
-  public static wrap(err: Error | string): SfError {
+  public static wrap<T extends ErrorDataProperties = ErrorDataProperties>(err: unknown): SfError<T> {


supports unknown.

Safer from runtime errors from not checking types in a catch like ex:

sfdx-core/src/org/org.ts

Line 1599 in 092af1b

const err = SfError.wrap(e as string | Error);

Simpler than checking types in a catch block like

sfdx-core/src/org/scratchOrgInfoApi.ts

Lines 379 to 380 in 092af1b

if (error instanceof Error) {

const sfError = SfError.wrap(error);

mshanemc · 2024-04-04T16:33:42Z

src/sfError.ts

    }

    if (err instanceof SfError) {
-      return err;
+      return err as SfError<T>;


only matters if you want to type the .data prop

mshanemc · 2024-04-04T16:34:29Z

test/tsconfig.json

@@ -5,6 +5,10 @@
    "noEmit": true,
    "skipLibCheck": true,
    "resolveJsonModule": true,
-    "esModuleInterop": true
+    "esModuleInterop": true,
+    "lib": ["ES2022"],


necessary to get the native error.cause

we already set engines to node18+ but now we type/compile for it. Dev-scripts non-esm are still on es2021.

WillieRuemmele

it all seems like a lot of usability improvements

WillieRuemmele · 2024-04-04T17:13:06Z

test/unit/sfErrorTest.ts

+        const mySfError = SfError.wrap(wrapMe);
+        expect(mySfError).to.be.an.instanceOf(SfError);
+        expect(mySfError.message === 'An unexpected error occurred');
+        expect(mySfError.name === 'TypeError');


Does it really make sense than an SfError.wrap(undefined).name==='TypeError'?

I think so. You're passing something that is of an unexpected type and we can't do much with it.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError

A TypeError may be thrown when:
an operand or argument passed to a function is incompatible with the type expected by that operator or function; or

What would you prefer?

shetzel · 2024-04-04T17:26:34Z

src/sfError.ts

+        ? // a basic error with message and name.  We make it the cause to preserve any other properties
+          new SfError<T>(err.message, err.name, undefined, err)
+        : // ok, something was throws that wasn't error or string.  Convert it to an Error that preserves the information as the cause and wrap that.
+          SfError.wrap<T>(new TypeError('An unexpected error occurred', { cause: err }));


Why TypeError and not just Error?

see above comment about TypeError. Sounds like it's confusing to 2 people now.

I'd argue it's fairly idiomatic in TS (typically, type-checking an any/unknown)
see https://github.com/search?q=throw+new+TypeError+language%3ATypeScript&type=code&l=TypeScript

To me, a TypeError means that the author of the code made a mistake and someone needs to fix a client side bug. Usually this would mean the CLI team. W.r.t "plugin health" it would be categorized as a bug for the CLI team (if called from the sf CLI). If this is intended, then it's fine. If the meaning of the error is anything else, we should just use Error. Although, since it's immediately being wrapped by SfError the command framework probably wouldn't interpret this as a TypeError and would categorize it differently. In the end it probably doesn't matter much.

cristiand391 · 2024-04-05T14:59:06Z

src/sfError.ts

-    super(name ?? 'SfError', message || name, cause);
+    if (typeof cause !== 'undefined' && !(cause instanceof Error)) {
+      throw new TypeError(`The cause, if provided, must be an instance of Error. Received: ${typeof cause}`);
+    }


related to the WI about correctly bubbling up the erro from iso-git > STL > SDR:

STL throws an SfError with data in it, SDR will wrap it and rethrow (new SfError...) but you have to remember to call error.setData(originalError) or the data is lost.
Could we check here in the constructor if cause instanceof SfError and save possible cause.data in the new error?

a "parent" error could call setData or mutating the data prop and losing the cause data.

but, it's a good idea.

Hmm....there's also the typing for data to be concerned about.

Ex:

if (resolvedCause instanceof SfError && resolvedCause.data) { this.data = resolvedCause.data; <== this is all squiggly }

because the SfError can be constructed with a but the cause has a different and unknowable T. Asserting that it is the same can cause (no pun intended) problems upstream.

but maybe this is the least bad option.

this.data = resolvedCause.data as T;

mshanemc added 3 commits April 4, 2024 10:50

feat: sfError

c8d9c1c

test: create

91cc5c2

chore: tsconfig changes to get es2022 error.cause (node 18+)

39bd401

mshanemc commented Apr 4, 2024

View reviewed changes

src/sfError.ts Outdated Show resolved Hide resolved

mshanemc commented Apr 4, 2024

View reviewed changes

WillieRuemmele reviewed Apr 4, 2024

View reviewed changes

shetzel reviewed Apr 4, 2024

View reviewed changes

shetzel approved these changes Apr 4, 2024

View reviewed changes

feat: let cause be unknown for constructor, create (like wrap)

aed3f3c

cristiand391 reviewed Apr 5, 2024

View reviewed changes

mshanemc added 3 commits April 5, 2024 12:34

chore: no TypeError

cdfa928

chore: wrap an Error (so cause can be the original error)

08b0c74

chore: tsconfig for postcompile (doc)

bcdaf7f

mshanemc merged commit 9b4ac85 into sm/core-7-deprecations Apr 8, 2024
66 checks passed

mshanemc deleted the sm/sfError branch April 8, 2024 19:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improved sfError #1046

feat: improved sfError #1046

mshanemc commented Apr 4, 2024 •

edited

Loading

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024

mshanemc Apr 4, 2024 •

edited

Loading

WillieRuemmele left a comment

WillieRuemmele Apr 4, 2024

mshanemc Apr 5, 2024

shetzel Apr 4, 2024

mshanemc Apr 5, 2024

shetzel Apr 5, 2024

cristiand391 Apr 5, 2024

mshanemc Apr 5, 2024

mshanemc Apr 5, 2024

mshanemc Apr 5, 2024

		import { AnyJson, hasString, isString, JsonMap } from '@salesforce/ts-types';

		export type SfErrorOptions<T extends ErrorDataProperties = ErrorDataProperties> = {

	if (error instanceof Error) {
	const sfError = SfError.wrap(error);

feat: improved sfError #1046

feat: improved sfError #1046

Conversation

mshanemc commented Apr 4, 2024 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc Apr 4, 2024 • edited Loading

Choose a reason for hiding this comment

WillieRuemmele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc commented Apr 4, 2024 •

edited

Loading

mshanemc Apr 4, 2024 •

edited

Loading