Description
We need to make a decision on the error handling pattern for cases where there are no event handlers attached to the error event. There are a number of variables and cases at play.
Let's consider this case:
slackEvents.on('message', event => event.foo.bar());The handler throw a TypeError into the middleware, which will attempt to emit an error event on the adapter (slackEvents). Since there is no handler for that event type, the .emit() call will throw in the middleware. Let's take the case where there is no next middleware defined (the default if you are using the all-in-one API). The final handler from Express will respond with a 500 Internal Server Error. Express will also print the stacktrace of the error to stdout. Then, Slack will attempt reties over and over with the same result until the retries are exhausted and eventually your event subscription may be turned off on the Slack app.
This is not ideal because as the app developer, you may or may not have any indication that something is going wrong. You will see the same event hit your server three times, you will not get the result you expect, and you'll wonder what actually happened with the response. If you were listening to the error event, or listening to the proposed retry event, or using the middleware with an express application that had another error handler and chose to use the propagateErrors option, or you used the includeHeaders option and noticed the retry headers; then you would have more information about what is happening.
Another option is to "swallow" the error in the middleware and send a 200 OK response regardless. This would result in the event only showing up at your server once. But this doesn't really help us surface the error.
The EventEmitter (which SlackEventAdapter inherits from) in node core has a convention where if there is no error handler defined, then an error is thrown, with the intention that if it is not caught at the top level scope, you get one final chance to handle it in process's uncaughtException event before terminating the process. Since SlackEventAdapter is an EventEmitter, and that's the only interface a user of the all-in-one API would ever know, this feels like a strong contender for the convention this package should follow. This is at odds with Express' opinion of "keep the server up, just respond with 500 on unhandled errors".
Another distinct question is how propagateErrors should relate to the error handling system. Specifically, if propagateErrors is enabled, and the error is given to the next middleware, should the adapter also emit the error? Given the tricky nature of what happens with emitting an error, it seems simpler if propagating errors is mutually exclusive with handling them on the EventEmitter interface.
OPINIONS PLEASE!
What type of issue is this? (place an x in one of the [ ])
Requirements
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent