Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: improved sfError #1046

Merged
merged 7 commits into from
Apr 8, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 46 additions & 23 deletions src/sfError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,20 @@
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

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

export type SfErrorOptions<T extends ErrorDataProperties = ErrorDataProperties> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type for the new .create method param obj

message: string;
exitCode?: number;
name?: string;
data?: T;
cause?: Error;
context?: string;
actions?: string[];
};

type ErrorDataProperties = AnyJson;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* A generalized sfdx error which also contains an action. The action is used in the
* CLI to help guide users past the error.
Expand All @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave NamedError out of this

public readonly name: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

brought from NamedError

/**
* Action messages. Hints to the users regarding what can be done to fix related issues.
*/
Expand Down Expand Up @@ -59,13 +71,15 @@ export class SfError<T = unknown> extends NamedError {
*/
public constructor(
message: string,
name?: string,
name = 'SfError',
actions?: string[],
exitCodeOrCause?: number | Error,
cause?: Error
) {
cause = exitCodeOrCause instanceof Error ? exitCodeOrCause : cause;
super(name ?? 'SfError', message || name, cause);
const derivedCause = exitCodeOrCause instanceof Error ? exitCodeOrCause : cause;
super(message);
this.name = name;
this.cause = derivedCause;
this.actions = actions;
if (typeof exitCodeOrCause === 'number') {
this.exitCode = exitCodeOrCause;
Expand All @@ -74,6 +88,10 @@ export class SfError<T = unknown> extends NamedError {
}
}

public get fullStack(): string | undefined {
cristiand391 marked this conversation as resolved.
Show resolved Hide resolved
return recursiveStack(this).join('\nCaused by: ');
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
public get code(): any {
return this.#code ?? this.name;
Expand All @@ -83,26 +101,39 @@ export class SfError<T = unknown> extends NamedError {
this.#code = code;
}

/** like the constructor, but takes an typed object and let you also set context and data properties */
public static create<T extends ErrorDataProperties = ErrorDataProperties>(inputs: SfErrorOptions<T>): SfError<T> {
const error = new SfError<T>(inputs.message, inputs.name, inputs.actions, inputs.exitCode, inputs.cause);
error.data = inputs.data;
error.context = inputs.context;
return error;
}
/**
* 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> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supports unknown.

if (isString(err)) {
return new SfError(err);
return new SfError<T>(err);
}

if (err instanceof SfError) {
return err;
return err as SfError<T>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only matters if you want to type the .data prop

}

const sfError = new SfError(err.message, err.name, undefined, err);
const sfError =
err instanceof Error
? // 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 }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why TypeError and not just Error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// If the original error has a code, use that instead of name.
if (hasString(err, 'code')) {
sfError.code = err.code;
}

return sfError;
}

Expand Down Expand Up @@ -130,24 +161,16 @@ export class SfError<T = unknown> extends NamedError {
* Convert an {@link SfError} state to an object. Returns a plain object representing the state of this error.
*/
public toObject(): JsonMap {
const obj: JsonMap = {
return {
name: this.name,
message: this.message ?? this.name,
exitCode: this.exitCode,
actions: this.actions,
...(this.context ? { context: this.context } : {}),
...(this.data ? { data: this.data } : {}),
};

if (this.context) {
obj.context = this.context;
}

if (this.data) {
// DANGER: data was previously typed as `unknown` and this assertion was here on the toObject.
// TODO in next major release: put proper type constraint on SfError.data to something that can serialize
// while we're making breaking changes, provide a more definite type for toObject
obj.data = this.data as AnyJson;
}

return obj;
}
}

const recursiveStack = (err: Error): string[] =>
(err.cause && err.cause instanceof Error ? [err.stack, ...recursiveStack(err.cause)] : [err.stack]).filter(isString);
6 changes: 5 additions & 1 deletion test/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@
"noEmit": true,
"skipLibCheck": true,
"resolveJsonModule": true,
"esModuleInterop": true
"esModuleInterop": true,
"lib": ["ES2022"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

necessary to get the native error.cause

Copy link
Contributor Author

@mshanemc mshanemc Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"module": "Node16",
"moduleResolution": "Node16",
"target": "ES2022"
}
}
197 changes: 196 additions & 1 deletion test/unit/sfErrorTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* 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 { expect, assert } from 'chai';
import { Messages } from '../../src/messages';
import { SfError } from '../../src/sfError';

Expand All @@ -31,8 +31,71 @@ describe('SfError', () => {
const err = new SfError(msg, 'myErrorName');
expect(err.name).to.equal('myErrorName');
});

it('sets actions', () => {
const msg = 'this is a test message';
const actions = ['Do this action', 'Do that action'];
const err = new SfError(msg, 'myErrorName', actions);
expect(err.actions).to.equal(actions);
});

it('cause as 4th property', () => {
const msg = 'this is a test message';
const cause = new Error('cause');
const err = new SfError(msg, 'myErrorName', undefined, cause);
expect(err.cause).to.equal(cause);
});

it('cause as 5th property + exitCode', () => {
const msg = 'this is a test message';
const cause = new Error('cause');
const err = new SfError(msg, 'myErrorName', undefined, 2, cause);
expect(err.cause).to.equal(cause);
expect(err.exitCode).to.equal(2);
});

it('exitCode is 1 when undefined is provided', () => {
const msg = 'this is a test message';
const cause = new Error('cause');
const err = new SfError(msg, 'myErrorName', undefined, undefined, cause);
expect(err.cause).to.equal(cause);
expect(err.exitCode).to.equal(1);
});

it('exitCode is 1 when no arg is provided', () => {
const msg = 'this is a test message';
const err = new SfError(msg, 'myErrorName');
expect(err.cause).to.equal(undefined);
expect(err.exitCode).to.equal(1);
});
});

describe('fullStack', () => {
it('returned `name:message` when no cause', () => {
const err = new SfError('test');
expect(err.fullStack).to.include('SfError: test');
expect(err.fullStack).to.include('sfErrorTest.ts');
expect(err.fullStack).to.not.include('Caused by:');
});
it('1 cause', () => {
const nestedError = new Error('nested');
const err = new SfError('test', undefined, undefined, nestedError);
expect(err.fullStack).to.include('SfError: test');
expect(err.fullStack).to.include('sfErrorTest.ts');
expect(err.fullStack).to.include('nested');
expect(err.fullStack?.match(/Caused by:/g)).to.have.lengthOf(1);
});
it('recurse through stacked causes', () => {
const nestedError = new Error('nested');
const nestedError2 = new Error('nested2', { cause: nestedError });
const err = new SfError('test', undefined, undefined, nestedError2);
expect(err.fullStack).to.include('SfError: test');
expect(err.fullStack).to.include('sfErrorTest.ts');
expect(err.fullStack).to.include('nested');
expect(err.fullStack).to.include('nested2');
expect(err.fullStack?.match(/Caused by:/g)).to.have.lengthOf(2);
});
});
describe('wrap', () => {
it('should return a wrapped error', () => {
const myErrorMsg = 'yikes! What did you do?';
Expand Down Expand Up @@ -70,6 +133,55 @@ describe('SfError', () => {
expect(mySfError).to.be.an.instanceOf(SfError);
expect(mySfError).to.equal(existingSfError);
});

describe('handling "other" stuff that is not Error', () => {
it('undefined', () => {
const wrapMe = undefined;
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
expect(mySfError.message === 'An unexpected error occurred');
expect(mySfError.name === 'TypeError');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.message === 'An unexpected error occurred');
expect(mySfError.cause.cause).to.equal(wrapMe);
});
it('a number', () => {
const wrapMe = 2;
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.cause).to.equal(wrapMe);
});
it('an object', () => {
const wrapMe = { a: 2 };
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.cause).to.equal(wrapMe);
});
it('an object that has a code', () => {
const wrapMe = { a: 2, code: 'foo' };
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.cause).to.equal(wrapMe);
expect(mySfError.code).to.equal('foo');
});
it('an array', () => {
const wrapMe = [1, 5, 6];
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.cause).to.equal(wrapMe);
});
it('a class', () => {
const wrapMe = new (class Test {})();
const mySfError = SfError.wrap(wrapMe);
expect(mySfError).to.be.an.instanceOf(SfError);
assert(mySfError.cause instanceof TypeError);
expect(mySfError.cause.cause).to.equal(wrapMe);
});
});
});

describe('generic for data', () => {
Expand Down Expand Up @@ -131,4 +243,87 @@ describe('SfError', () => {
});
});
});

describe('create', () => {
it('message only sets the default error name', () => {
const message = 'its a trap!';
const error = SfError.create({ message });
expect(error.message).to.equal(message);
expect(error.name).to.equal('SfError');
});
it('sets name', () => {
const message = 'its a trap!';
const name = 'BadError';
const error = SfError.create({ message, name });
expect(error.message).to.equal(message);
expect(error.name).to.equal(name);
});
it('sets cause', () => {
const cause = new Error('cause');
const error = SfError.create({ message: 'its a trap!', cause });
expect(error.cause).to.equal(cause);
});
it('sets exit code', () => {
const message = 'its a trap!';
const exitCode = 100;
const error = SfError.create({ message, exitCode });
expect(error.message).to.equal(message);
expect(error.exitCode).to.equal(exitCode);
});
it('sets actions', () => {
const message = 'its a trap!';
const actions = ['do the opposite'];
const error = SfError.create({ message, actions });
expect(error.message).to.equal(message);
expect(error.actions).to.equal(actions);
});
it('sets data', () => {
const message = 'its a trap!';
const data = { foo: 'pity the foo' };
const error = SfError.create({ message, data });
expect(error.message).to.equal(message);
expect(error.data).to.equal(data);
});
it('sets data (typed)', () => {
const message = 'its a trap!';
const data = { foo: 'pity the foo' };
const error = SfError.create<{ foo: string }>({ message, data });
expect(error.message).to.equal(message);
expect(error.data).to.equal(data);
});
it('sets context', () => {
const message = 'its a trap!';
const context = 'TestContext1';
const error = SfError.create({ message, context });
expect(error.message).to.equal(message);
expect(error.context).to.equal(context);
});
it('all the things', () => {
const message = 'its a trap!';
const name = 'BadError';
const actions = ['do the opposite'];
const cause = new Error('cause');
const exitCode = 100;
const context = 'TestContext1';
const data = { foo: 'pity the foo' };

const error = SfError.create({
message,
name,
actions,
cause,
exitCode,
context,
data,
});

expect(error.message).to.equal(message);
expect(error.name).to.equal(name);
expect(error.actions).to.equal(actions);
expect(error.cause).to.equal(cause);
expect(error.exitCode).to.equal(exitCode);
expect(error.context).to.equal(context);
expect(error.data).to.equal(data);
});
});
});
4 changes: 4 additions & 0 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
"compilerOptions": {
"outDir": "./lib",
"resolveJsonModule": true,
"lib": ["ES2022"],
"module": "Node16",
"moduleResolution": "Node16",
"target": "ES2022",
"rootDir": "./src",
"plugins": [{ "transform": "./src/messageTransformer.ts" }],
"esModuleInterop": true
Expand Down
Loading