Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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