Skip to content

Commit

Permalink
Change exceptions to extend Error.
Browse files Browse the repository at this point in the history
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](creditkarma#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.
  • Loading branch information
markdoliner-doma committed Jun 1, 2020
1 parent e8931ff commit fd7fe5f
Show file tree
Hide file tree
Showing 5 changed files with 19 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)
}
1 change: 1 addition & 0 deletions src/main/render/thrift-server/identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export const THRIFT_IDENTIFIERS = {
'thrift.InputBufferUnderrunError',
),
StructLike: ts.createIdentifier('thrift.StructLike'),
ErrorStructLike: ts.createIdentifier('thrift.ErrorStructLike'),
}

export const THRIFT_TYPES = {
Expand Down
7 changes: 6 additions & 1 deletion src/main/render/thrift-server/struct/class.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
classNameForStruct,
createSuperCall,
extendsAbstract,
extendsAbstractError,
implementsInterface,
looseNameForStruct,
throwForField,
Expand All @@ -38,6 +39,7 @@ export function renderClass(
node: InterfaceWithFields,
state: IRenderState,
isExported: boolean,
extendError: boolean = false,
): ts.ClassDeclaration {
const fields: Array<ts.PropertyDeclaration> = [
...createFieldsForStruct(node, state),
Expand Down Expand Up @@ -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,
Expand Down
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),
]
}
9 changes: 9 additions & 0 deletions src/main/render/thrift-server/struct/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit fd7fe5f

Please sign in to comment.