Exception classes ought to inherit from Error class

[jlai]

Currently, this library generates exception objects as regular thrift structs which do not extend Error. This is problematic because it doesn't have a stack trace, and behaves weirdly for frameworks that expect that throw is only used with Error, such as graphql-js.

This is also different from the Apache thrift generator, which has exception types inheriting from thrift.TException.

Overall, it would be a lot cleaner (and in line with standard JS coding practices) to make exceptions inherit from Error.

[markdoliner]

I spent some time looking at this today because I think it would address the related issue #181, and I thought it might be helpful to share my findings.

Currently the code generation for exceptions is identical to structs. It wouldn't be too difficult to tweak that code so that Thrift exception classes derive from Error. There's a small wrinkle that structs extend the StructLike class (see the extendsAbstract() function call here, which is defined here) and we wouldn't want to change StructLike to derive from Error so we might need to add some sort of ErrorStructLike class that does the same thing as StructLike and also extends Error, but that's fine. No big deal.

The bigger question is that the decode functions don't actually instantiate objects. They return plain old JavaScript objects that match the exception's interface. For example, if you have CustomException with an errorCode field then the decode function will return {errorCode: 12345}. It won't return new CustomException({errorCode: 12345}). I don't know the reasoning that went into the decision to return plain objects vs. instantiating classes.

I only investigated --target thrift-server. I didn't investigate --target apache.

So now I have some questions for the fine folks at Credit Karma. cc @kevin-greene-ck

1. Anyone have thoughts on this? Should Thrift exceptions derive from Error? Should they not? No opinion?
2. Should decode functions instantiate objects? Should they not? Is there a reason for doing it one way or the other? Is this decision documented somewhere? I'm not familiar with the source of thrift-typescript or thrift-server or thrift-client... I haven't even thought about why interfaces are used. Do they serve an important purpose?
3. I'm surprised more people haven't had problems with this. How are other folks distinguishing between different types of exceptions? Maybe passing --withNameField to thrift-typescript to add an __name field to all structs?
4. I noticed a GitHub issue about version 4 being in development a year ago. It doesn't mention exceptions or object instantiation... does that mean these things aren't on the roadmap?
5. This question is getting off topic, but I'm curious about the overall status of the project. It seems like there hasn't been a lot of activity from the Credit Karma folks. Are you still using Thrift, I hope? Any plans to make improvements? Or hard to allocate people to work on it?

[markdoliner]

Other consideration: If exception classes inherit from Error class, what do we do if the exception has fields named message, name, or any other field that might exist on the Error class ('stack', 'fileName', 'lineNumber', etc)? Maybe the compiler should disallow them with a helpful message? Or it could allow them, which would result in generated ts that doesn't compile when the exception class uses a different type for the field than the Error class (like makes message optional or changes the type to a bool).

[markdoliner-doma]

I took a stab at this in https://github.com/creditkarma/thrift-typescript/pull/185 and https://github.com/creditkarma/thrift-typescript/pull/184 and https://github.com/creditkarma/thrift-server/pull/132