From 880a008df68d90884629a855828e69a164958f3f Mon Sep 17 00:00:00 2001 From: Steve Gill Date: Tue, 24 Mar 2020 15:30:07 -0700 Subject: [PATCH] updated based on feedback from #380/#381 --- README.md | 3 +- docs/_advanced/receiver.md | 4 +- docs/_basic/listening_responding_shortcuts.md | 2 +- src/App.spec.ts | 2 +- src/ExpressReceiver.ts | 54 ++++++++----------- src/errors.spec.ts | 4 +- src/errors.ts | 9 +--- 7 files changed, 31 insertions(+), 47 deletions(-) diff --git a/README.md b/README.md index 1875085a3..e1688124d 100644 --- a/README.md +++ b/README.md @@ -256,7 +256,6 @@ async function authWithAcme({ payload, context, say, next }) { // When the user lookup is successful, add the user details to the context context.user = user; } catch (error) { - // middleware/listeners continue if (error.message === 'Not Found') { // In the real world, you would need to check if the say function was defined, falling back to the respond // function if not, and then falling back to only logging the error as a last resort. @@ -327,7 +326,7 @@ In general, a middleware can run both before and after the remaining middleware How you use `next` can have four different effects: -* **To both preprocess and post-process events** - You can choose to do work going _before_ listener functions by putting code +* **To both preprocess and post-process events** - You can choose to do work both _before_ listener functions by putting code before `await next()` and _after_ by putting code after `await next()`. `await next()` passes control down the middleware stack in the order it was defined, then back up it in reverse order. diff --git a/docs/_advanced/receiver.md b/docs/_advanced/receiver.md index 87e189f0c..eb9af501a 100644 --- a/docs/_advanced/receiver.md +++ b/docs/_advanced/receiver.md @@ -15,11 +15,11 @@ A receiver is responsible for handling and parsing any incoming events from Slac | `stop()` | None | `Promise` | `init()` is called after Bolt for JavaScript app is created. This method gives the receiver a reference to an `App` to store so that it can call: -* `await app.processEvent(event)` whenever your app receives an event from Slack. It will reject if there is an unhandled error. +* `await app.processEvent(event)` whenever your app receives an event from Slack. It will throw if there is an unhandled error. To use a custom receiver, you can pass it into the constructor when initializing your Bolt for JavaScript app. Here is what a basic custom receiver might look like. -For a more in-depth look at a receiver, [read the source code for the built-in Express receiver](https://github.com/slackapi/bolt/blob/master/src/ExpressReceiver.ts) +For a more in-depth look at a receiver, [read the source code for the built-in `ExpressReceiver`](https://github.com/slackapi/bolt/blob/master/src/ExpressReceiver.ts) ```javascript diff --git a/docs/_basic/listening_responding_shortcuts.md b/docs/_basic/listening_responding_shortcuts.md index c0db2d905..5d3227eab 100644 --- a/docs/_basic/listening_responding_shortcuts.md +++ b/docs/_basic/listening_responding_shortcuts.md @@ -82,7 +82,7 @@ app.shortcut('open_modal', async ({ shortcut, ack, context, client }) => { ```javascript // Your middleware will only be called when the callback_id matches 'open_modal' AND the type matches 'message_action' - app.shortcut({ callback_id: 'open_modal', type: 'message_action' }, async ({ action, ack, context, client }) => { + app.shortcut({ callback_id: 'open_modal', type: 'message_action' }, async ({ shortcut, ack, context, client }) => { try { // Acknowledge shortcut request await ack(); diff --git a/src/App.spec.ts b/src/App.spec.ts index ad04612ac..5805e1619 100644 --- a/src/App.spec.ts +++ b/src/App.spec.ts @@ -440,7 +440,7 @@ describe('App', () => { app.error(async (actualError) => { assert.instanceOf(actualError, UnknownError); assert.equal(actualError.message, error.message); - await delay(); // Make this async to make sure error handlers can be tested + // await delay(); // Make this async to make sure error handlers can be tested }); await fakeReceiver.sendEvent(dummyReceiverEvent); diff --git a/src/ExpressReceiver.ts b/src/ExpressReceiver.ts index 7230a338b..19a37ea7c 100644 --- a/src/ExpressReceiver.ts +++ b/src/ExpressReceiver.ts @@ -1,12 +1,12 @@ import { AnyMiddlewareArgs, Receiver, ReceiverEvent } from './types'; import { createServer, Server } from 'http'; -import express, { Request, Response, Application, RequestHandler, NextFunction } from 'express'; +import express, { Request, Response, Application, RequestHandler } from 'express'; import rawBody from 'raw-body'; import querystring from 'querystring'; import crypto from 'crypto'; import tsscmp from 'tsscmp'; import App from './App'; -import { ReceiverAuthenticityError, ReceiverAckTimeoutError, ReceiverMultipleAckError } from './errors'; +import { ReceiverAuthenticityError, ReceiverMultipleAckError } from './errors'; import { Logger, ConsoleLogger } from '@slack/logger'; // TODO: we throw away the key names for endpoints, so maybe we should use this interface. is it better for migrations? @@ -29,6 +29,7 @@ export default class ExpressReceiver implements Receiver { private server: Server; private bolt: App | undefined; + private logger: Logger; constructor({ signingSecret = '', @@ -36,7 +37,6 @@ export default class ExpressReceiver implements Receiver { endpoints = { events: '/slack/events' }, }: ExpressReceiverOptions) { this.app = express(); - this.app.use(this.errorHandler.bind(this)); // TODO: what about starting an https server instead of http? what about other options to create the server? this.server = createServer(this.app); @@ -47,6 +47,7 @@ export default class ExpressReceiver implements Receiver { this.requestHandler.bind(this), ]; + this.logger = logger; const endpointList: string[] = typeof endpoints === 'string' ? [endpoints] : Object.values(endpoints); for (const endpoint of endpointList) { this.app.post(endpoint, ...expressMiddleware); @@ -54,33 +55,28 @@ export default class ExpressReceiver implements Receiver { } private async requestHandler(req: Request, res: Response): Promise { - let timer: NodeJS.Timeout | undefined = setTimeout( - () => { - this.bolt?.handleError(new ReceiverAckTimeoutError( - 'An incoming event was not acknowledged before the timeout. ' + - 'Ensure that the ack() argument is called in your listeners.', - )); - timer = undefined; - }, - 2800, - ); + let isAcknowledged = false; + setTimeout(() => { + if (!isAcknowledged) { + this.logger.error('An incoming event was not acknowledged within 3 seconds. ' + + 'Ensure that the ack() argument is called in a listener.'); + } + // tslint:disable-next-line: align + }, 3001); const event: ReceiverEvent = { body: req.body, - ack: async (response): Promise => { - if (timer !== undefined) { - clearTimeout(timer); - timer = undefined; - - if (!response) { - res.send(''); - } else if (typeof response === 'string') { - res.send(response); - } else { - res.json(response); - } + ack: async (response: any): Promise => { + if (isAcknowledged) { + throw new ReceiverMultipleAckError(); + } + isAcknowledged = true; + if (!response) { + res.send(''); + } else if (typeof response === 'string') { + res.send(response); } else { - this.bolt?.handleError(new ReceiverMultipleAckError()); + res.json(response); } }, }; @@ -129,12 +125,6 @@ export default class ExpressReceiver implements Receiver { }); }); } - - private errorHandler(err: any, _req: Request, _res: Response, next: NextFunction): void { - this.bolt?.handleError(err); - // Forward to express' default error handler (which knows how to print stack traces in development) - next(err); - } } export const respondToSslCheck: RequestHandler = (req, res, next) => { diff --git a/src/errors.spec.ts b/src/errors.spec.ts index b06e6e5ec..ac56a2819 100644 --- a/src/errors.spec.ts +++ b/src/errors.spec.ts @@ -7,8 +7,8 @@ import { AppInitializationError, AuthorizationError, ContextMissingPropertyError, - ReceiverAckTimeoutError, ReceiverAuthenticityError, + ReceiverMultipleAckError, UnknownError, } from './errors'; @@ -19,8 +19,8 @@ describe('Errors', () => { [ErrorCode.AppInitializationError]: new AppInitializationError(), [ErrorCode.AuthorizationError]: new AuthorizationError('auth failed', new Error('auth failed')), [ErrorCode.ContextMissingPropertyError]: new ContextMissingPropertyError('foo', "can't find foo"), - [ErrorCode.ReceiverAckTimeoutError]: new ReceiverAckTimeoutError(), [ErrorCode.ReceiverAuthenticityError]: new ReceiverAuthenticityError(), + [ErrorCode.ReceiverMultipleAckError]: new ReceiverMultipleAckError(), [ErrorCode.UnknownError]: new UnknownError(new Error('It errored')), }; diff --git a/src/errors.ts b/src/errors.ts index f8150e9c6..0e20a75b2 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -8,8 +8,7 @@ export enum ErrorCode { ContextMissingPropertyError = 'slack_bolt_context_missing_property_error', - ReceiverAckTimeoutError = 'slack_bolt_receiver_ack_timeout_error', - ReceiverAckTwiceError = 'slack_bolt_receiver_ack_twice_error', + ReceiverMultipleAckError = 'slack_bolt_receiver_ack_multiple_error', ReceiverAuthenticityError = 'slack_bolt_receiver_authenticity_error', /** @@ -52,12 +51,8 @@ export class ContextMissingPropertyError extends Error implements CodedError { } } -export class ReceiverAckTimeoutError extends Error implements CodedError { - public code = ErrorCode.ReceiverAckTimeoutError; -} - export class ReceiverMultipleAckError extends Error implements CodedError { - public code = ErrorCode.ReceiverAckTimeoutError; + public code = ErrorCode.ReceiverMultipleAckError; constructor() { super("The receiver's `ack` function was called multiple times.");