feat: new configuration setting to opt-in to throwing on errors #1098
Conversation
cprice404
force-pushed
the
throw-on-errors
branch
from
6c72238
to
3a69f84
Compare
cprice404
changed the title
cprice404
requested review from
bruuuuuuuce,
malandis,
a team and
allenheltondev
|
|
||
| constructor(throwOnError: boolean) { | ||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
| console.log(`CONSTRUCTING CACHE SERVICE ERROR MAPPER: ${throwOnError}`); |
There was a problem hiding this comment.
| console.log(`CONSTRUCTING CACHE SERVICE ERROR MAPPER: ${throwOnError}`); |
| private readonly throwOnError: boolean; | ||
|
|
||
| constructor(throwOnError: boolean) { | ||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions |
There was a problem hiding this comment.
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions |
| this.cacheServiceErrorMapper.handleError( | ||
| err, | ||
| e => new DeleteCache.Error(e), | ||
| resolve, |
There was a problem hiding this comment.
is there a compilation failure if you accidentally swap the resolve and reject functions?
There was a problem hiding this comment.
I don't think so. Presumably you would get some int test failures though because nothing would succeed?
Also open to ideas on how to improve this.
There was a problem hiding this comment.
the idea that comes to mind first is having the function take in an object with named arguments, but I don't think that its a dealbreaker, and that would require changing basically every line in this pr 😅 . I think how it currently is is good
| @@ -244,7 +253,7 @@ export class PubsubClient extends AbstractPubsubClient { | |||
| serviceError.code === Status.INTERNAL && | |||
| serviceError.details === PubsubClient.RST_STREAM_NO_ERROR_MESSAGE; | |||
| const momentoError = new TopicSubscribe.Error( | |||
| cacheServiceErrorMapper(serviceError) | |||
| this.cacheServiceErrorMapper.convertError(serviceError) | |||
There was a problem hiding this comment.
We are not throwing here because we might retry the subscription? Do we want to throw when its the first subscription message?
There was a problem hiding this comment.
this is a good question, I probably need to look into this part a little more closely.
There was a problem hiding this comment.
The thing here is that our JS topic subscriptions are callback-based already. So what that means is that there is no thread of execution to try to throw the exception into; if an error occurs during a subscription then the onError callback gets called.
I can't immediately think of a behavior change that the ThrowOnErrors flag could reasonably introduce for a callback-based subscription model. In other languages where the subscriptions are iterator-style, there's a thread to throw the execption into, but not for callback-style.
Open to ideas/discussion, though!
…cheServiceErrorMapper
This commit adds a new configuration setting to the various Configuration objects:
ThrowOnErrors.The default behavior for all of the Momento clients is to return errors as part of the function's return value, to try to make error handling feel more like a first-class part of the API. However there are some cases where it is easier to allow errors to bubble up the call stack and handle them via a try/catch, and some users may prefer that style of error handling since it is more prevalent in JS.
If the new ThrowOnErrors config option can be set to
true, then the clients will throw exceptions rather than returning them as errors.