From 62a300420fb28ec4f16be2fc11729d092f0e1c7e Mon Sep 17 00:00:00 2001 From: Mark Doliner Date: Sun, 31 May 2020 23:11:17 -0400 Subject: [PATCH] Change exceptions to extend Error. This changes exception classes generated for `--target thrift-server` so that they extend `ErrorThriftLike` instead of `ThriftLike`. There's a corresponding `thrift-server` change to add the `ErrorThriftLike` class. As mentioned in [the GitHub issue](https://github.com/creditkarma/thrift-typescript/issues/178), this is useful because it causes Thrift exceptions to have stack traces and because some frameworks expect exceptions to derive from Error. The GitHub issue mentions graphql-js. Jest's `toThrow()` function would also benefit from this. Here's an example: ``` await expect(thriftClient.someFunction()).rejects.toThrow() ``` `toThrow` doesn't identify Thrift exceptions as exceptions because they don't derive from Error and so it will return false in such cases. Part of the blame could fall on Jest, because perhaps `toThrow` should return true for any rejected promise regardless of the type of object being throw, but I think some of the blame still falls on Thrift. A downside of extending Error is that there's a naming collision when developers define fields named `name`, `message`, `stack`, etc in their Thrift exceptions (because these are defined by the super class). I think this is an acceptable tradeoff. FYI `tsc` complains if someone defines a field that differs from the super class. For example, if someone has a Thrift exception with `1: required i32 message` (because message must be a string) or `1: optional string message` (because message is required). I tried to avoid the naming collisions by manually adding Error to the exception class's prototype chain. It might avoid the tsc compile errors when there's a naming collision but it doesn't really eliminate the problem of same-named fields being used for two different things. And so I think manually changing the prototype chain is a bad solution. Possible remaining work: - The naming collision limitation should be mentioned in README.md. - `tsc` complains for naming collisions, and that's fine. Maybe the Thrift compiler should check for and report on collisions? Would probably be a friendly user experience. One challenge is [different JS implementations define different properties on their Error class](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties). - I didn't change the code generated for `--target apache`. It looks like those classes also don't derive from Error. They probably should. --- src/main/render/thrift-server/exception/index.ts | 2 +- src/main/render/thrift-server/identifiers.ts | 1 + src/main/render/thrift-server/struct/class.ts | 7 ++++++- src/main/render/thrift-server/struct/index.ts | 3 ++- src/main/render/thrift-server/struct/utils.ts | 9 +++++++++ 5 files changed, 19 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/identifiers.ts b/src/main/render/thrift-server/identifiers.ts index 8c6d1847..2b3b54cd 100644 --- a/src/main/render/thrift-server/identifiers.ts +++ b/src/main/render/thrift-server/identifiers.ts @@ -33,6 +33,7 @@ export const THRIFT_IDENTIFIERS = { 'thrift.InputBufferUnderrunError', ), StructLike: ts.createIdentifier('thrift.StructLike'), + ErrorStructLike: ts.createIdentifier('thrift.ErrorStructLike'), } export const THRIFT_TYPES = { diff --git a/src/main/render/thrift-server/struct/class.ts b/src/main/render/thrift-server/struct/class.ts index b96e4796..fe08421b 100644 --- a/src/main/render/thrift-server/struct/class.ts +++ b/src/main/render/thrift-server/struct/class.ts @@ -27,6 +27,7 @@ import { classNameForStruct, createSuperCall, extendsAbstract, + extendsAbstractError, implementsInterface, looseNameForStruct, throwForField, @@ -38,6 +39,7 @@ export function renderClass( node: InterfaceWithFields, state: IRenderState, isExported: boolean, + extendError: boolean = false, ): ts.ClassDeclaration { const fields: Array = [ ...createFieldsForStruct(node, state), @@ -97,7 +99,10 @@ export function renderClass( tokens(isExported), classNameForStruct(node, state).replace('__NAMESPACE__', ''), [], - [extendsAbstract(), implementsInterface(node, state)], // heritage + [ + extendError ? extendsAbstractError() : extendsAbstract(), + implementsInterface(node, state), + ], // heritage [ ...fields, ctor, 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), ] } diff --git a/src/main/render/thrift-server/struct/utils.ts b/src/main/render/thrift-server/struct/utils.ts index 4955cc1d..a0594e8b 100644 --- a/src/main/render/thrift-server/struct/utils.ts +++ b/src/main/render/thrift-server/struct/utils.ts @@ -133,6 +133,15 @@ export function extendsAbstract(): ts.HeritageClause { ]) } +export function extendsAbstractError(): ts.HeritageClause { + return ts.createHeritageClause(ts.SyntaxKind.ExtendsKeyword, [ + ts.createExpressionWithTypeArguments( + [], + THRIFT_IDENTIFIERS.ErrorStructLike, + ), + ]) +} + export function implementsInterface( node: InterfaceWithFields, state: IRenderState,