Skip to content

Commit

Permalink
Add Error to prototype chain of exceptions.
Browse files Browse the repository at this point in the history
This is one possible approach for making exceptions derive from Error (creditkarma#178) and be distinguishable from each other (creditkarma#181).

It adds this line to the constructor of exception classes:
```
Object.setPrototypeOf(Object.getPrototypeOf(this), Error.prototype);
```

It's sort of the half-way solution. It's an attempt to solve the above problems with minimal disruption. See the in-code comment for details.

That being said, I'm not a fan of this solution. It does make `instanceof` work correctly, which is great. But `name` still exists and there are problems if its type isn't `string`. Also `name` gets set to "Error" instead of the name of our exception class. We could fix that by setting `this.name` in our constructor but the whole point of doing it this way was to avoid polluting our exception classes with fields from Error. Worth mentioning: I have no idea what sets `name` to "Error".

Also I didn't look at the render/apache/ code at all. It's possible a similar change should be made there--I have no idea.
  • Loading branch information
markdoliner-doma committed Jun 1, 2020
1 parent e8931ff commit 3501c16
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/main/render/thrift-server/exception/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ export function renderException(
node: ExceptionDefinition,
state: IRenderState,
): Array<ts.Statement> {
return renderStruct(node, state)
return renderStruct(node, state, true)
}
61 changes: 60 additions & 1 deletion src/main/render/thrift-server/struct/class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ export function renderClass(
node: InterfaceWithFields,
state: IRenderState,
isExported: boolean,
extendError: boolean = false,
): ts.ClassDeclaration {
const fields: Array<ts.PropertyDeclaration> = [
...createFieldsForStruct(node, state),
Expand All @@ -61,6 +62,10 @@ export function renderClass(
fields.splice(-2, 0, nameField)
}

const optionallyAddErrorToPrototypeChain: Array<ts.Statement> = extendError
? createAddErrorToPrototypeChainStatement()
: Array<ts.Statement>()

/**
* After creating the properties on our class for the struct fields we must create
* a constructor that knows how to assign these values based on a passed args.
Expand Down Expand Up @@ -88,7 +93,11 @@ export function renderClass(
// Build the constructor body
const ctor: ts.ConstructorDeclaration = createClassConstructor(
[argsParameter],
[createSuperCall(), ...fieldAssignments],
[
createSuperCall(),
...optionallyAddErrorToPrototypeChain,
...fieldAssignments,
],
)

// export class <node.name> { ... }
Expand Down Expand Up @@ -366,3 +375,53 @@ function createArgsTypeForStruct(
undefined,
)
}

/**
* Add Error to the class's prototype chain so that the class looks like
* it extends Error. It's convenient for Thrift exceptions to look like
* Errors because some tools/libraries work better that way. For example
* Jest's `toThrow()` expectation.
*
* We avoid ACTUALLY extending Error because we don't want to call
* super() and as of 2020-06-01 the TypeScript compiler doesn't allow
* extending a class and not calling super(). It gives this error:
* TS2377: Constructors for derived classes must contain a 'super' call.
*
* And we don't want to call super() because Error's constructor sets a
* few fields (message, name, etc). We don't want those fields to be set
* because they likely won't be defined in the Thrift definition, or if
* they ARE defined int he Thrift definition they will conflict with
* their meaning in the Error class. It could certainly be argued that
* this is the wrong decision and that we SHOULD call Error's
* constructor to cause the standard error fields to be set. It would
* mean that users of this compiler can't have fields named "message,"
* "name," etc in their Thrift exceptions. And maybe that's ok?
*/
function createAddErrorToPrototypeChainStatement(): Array<ts.Statement> {
return [
// Object.setPrototypeOf(Object.getPrototypeOf(this), Error.prototype)
ts.createStatement(
ts.createCall(
ts.createPropertyAccess(
ts.createIdentifier('Object'),
ts.createIdentifier('setPrototypeOf'),
),
undefined,
[
ts.createCall(
ts.createPropertyAccess(
ts.createIdentifier('Object'),
ts.createIdentifier('getPrototypeOf'),
),
undefined,
[COMMON_IDENTIFIERS.this],
),
ts.createPropertyAccess(
COMMON_IDENTIFIERS.Error,
ts.createIdentifier('prototype'),
),
],
),
),
]
}
3 changes: 2 additions & 1 deletion src/main/render/thrift-server/struct/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ import { renderClass } from './class'
export function renderStruct(
node: InterfaceWithFields,
state: IRenderState,
extendError: boolean = false,
): Array<ts.Statement> {
return [
...renderInterface(node, state, true),
renderToolkit(node, state, true),
renderClass(node, state, true),
renderClass(node, state, true, extendError),
]
}

0 comments on commit 3501c16

Please sign in to comment.