From 3501c16d19add4bf0e56ba17e2b7fd1061b6571a Mon Sep 17 00:00:00 2001 From: Mark Doliner Date: Sun, 31 May 2020 22:11:54 -0400 Subject: [PATCH] Add Error to prototype chain of exceptions. This is one possible approach for making exceptions derive from Error (https://github.com/creditkarma/thrift-typescript/issues/178) and be distinguishable from each other (https://github.com/creditkarma/thrift-typescript/issues/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. --- .../render/thrift-server/exception/index.ts | 2 +- src/main/render/thrift-server/struct/class.ts | 61 ++++++++++++++++++- src/main/render/thrift-server/struct/index.ts | 3 +- 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/src/main/render/thrift-server/exception/index.ts b/src/main/render/thrift-server/exception/index.ts index 5275a492..8570000a 100644 --- a/src/main/render/thrift-server/exception/index.ts +++ b/src/main/render/thrift-server/exception/index.ts @@ -10,5 +10,5 @@ export function renderException( node: ExceptionDefinition, state: IRenderState, ): Array { - return renderStruct(node, state) + return renderStruct(node, state, true) } diff --git a/src/main/render/thrift-server/struct/class.ts b/src/main/render/thrift-server/struct/class.ts index b96e4796..eeebd27b 100644 --- a/src/main/render/thrift-server/struct/class.ts +++ b/src/main/render/thrift-server/struct/class.ts @@ -38,6 +38,7 @@ export function renderClass( node: InterfaceWithFields, state: IRenderState, isExported: boolean, + extendError: boolean = false, ): ts.ClassDeclaration { const fields: Array = [ ...createFieldsForStruct(node, state), @@ -61,6 +62,10 @@ export function renderClass( fields.splice(-2, 0, nameField) } + const optionallyAddErrorToPrototypeChain: Array = extendError + ? createAddErrorToPrototypeChainStatement() + : Array() + /** * 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. @@ -88,7 +93,11 @@ export function renderClass( // Build the constructor body const ctor: ts.ConstructorDeclaration = createClassConstructor( [argsParameter], - [createSuperCall(), ...fieldAssignments], + [ + createSuperCall(), + ...optionallyAddErrorToPrototypeChain, + ...fieldAssignments, + ], ) // export class { ... } @@ -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 { + 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'), + ), + ], + ), + ), + ] +} diff --git a/src/main/render/thrift-server/struct/index.ts b/src/main/render/thrift-server/struct/index.ts index 8ce46603..81c992b8 100644 --- a/src/main/render/thrift-server/struct/index.ts +++ b/src/main/render/thrift-server/struct/index.ts @@ -13,10 +13,11 @@ import { renderClass } from './class' export function renderStruct( node: InterfaceWithFields, state: IRenderState, + extendError: boolean = false, ): Array { return [ ...renderInterface(node, state, true), renderToolkit(node, state, true), - renderClass(node, state, true), + renderClass(node, state, true, extendError), ] }