From 628235e3546de9fbfecd6d2047cc42c8a1764061 Mon Sep 17 00:00:00 2001 From: Mark Doliner Date: Sun, 31 May 2020 23:11:17 -0400 Subject: [PATCH 1/3] 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. Remaining work: - Fix tests. - Update README.md to mention that end-users need to require the new version of thrift-server which includes `ErrorThriftLike`. - Update README.me to mention the naming collision limitation. - `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, From 6b2df8b9e1e32804790271fa68972f7d7ff0d763 Mon Sep 17 00:00:00 2001 From: Mark Doliner Date: Fri, 28 Aug 2020 16:39:33 -0400 Subject: [PATCH 2/3] Update README. --- README.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 2b51f183..69a0766a 100644 --- a/README.md +++ b/README.md @@ -320,7 +320,7 @@ Given Thrift: ```c exception MyException { - 1: string message + 1: string msg 2: i32 code } ``` @@ -328,16 +328,18 @@ exception MyException { Generated TypeScript: ```typescript -export class MyException extends thrift.StructLike implements IMyException { - public message: string +export class MyException extends thrift.ErrorStructLike implements IMyException { + public msg: string public code?: number - constructor(args?: { message?: string, code?: number }) { + constructor(args?: { msg?: string, code?: number }) { // ... } } ``` -Then in your service client you could just throw the exception as you would any JS error `throw new MyException({ message: 'whoops', code: 500 });` +Then in your service client you could just throw the exception as you would any JS error `throw new MyException({ msg: 'whoops', code: 500 });` + +Note that these exception classes extend thrift.ErrorStructLike which extends JavaScript's Error object. Because of this you should avoid having fields in your exceptions that shadow fields in Error. `thrift-typescript` doesn't warn about this (TODO: Though perhaps it should try?) See [the MDN JavaScript reference](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error#Instance_properties) for a list of properties to avoid. (You actually *can* use the same field names if you want, as long as the types align, but avoiding them entirely might result in fewer headaches.) #### Service From db6efb1cde1b74428b3625b2b856588149a6f414 Mon Sep 17 00:00:00 2001 From: Mark Doliner Date: Fri, 28 Aug 2020 16:39:43 -0400 Subject: [PATCH 3/3] Fix tests. --- .../thrift-server/annotations_exception.solution.ts | 2 +- .../unit/fixtures/thrift-server/basic_exception.solution.ts | 2 +- .../generated-strict/com/test/calculator/NotAGoodIdea.ts | 2 +- .../generated-strict/com/test/common/AuthException.ts | 2 +- .../com/test/exceptions/InvalidOperation.ts | 2 +- .../generated-strict/com/test/exceptions/InvalidResult.ts | 2 +- .../generated/com/test/calculator/NotAGoodIdea.ts | 2 +- .../generated/com/test/common/AuthException.ts | 2 +- .../generated/com/test/exceptions/InvalidOperation.ts | 2 +- .../generated/com/test/exceptions/InvalidResult.ts | 2 +- .../fixtures/thrift-server/nested_exception.solution.ts | 2 +- .../thrift-server/required_field_exception.solution.ts | 2 +- .../fixtures/thrift-server/throws_multi_service.solution.ts | 6 +++--- .../unit/fixtures/thrift-server/throws_service.solution.ts | 2 +- 14 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/tests/unit/fixtures/thrift-server/annotations_exception.solution.ts b/src/tests/unit/fixtures/thrift-server/annotations_exception.solution.ts index 0e0dcc5c..0d8d954f 100644 --- a/src/tests/unit/fixtures/thrift-server/annotations_exception.solution.ts +++ b/src/tests/unit/fixtures/thrift-server/annotations_exception.solution.ts @@ -71,7 +71,7 @@ export const MyExceptionCodec: thrift.IStructCodec