Skip to content

Commit

Permalink
updated based on feedback from #380/#381
Browse files Browse the repository at this point in the history
  • Loading branch information
stevengill committed Mar 24, 2020
1 parent a8abaf9 commit 880a008
Show file tree
Hide file tree
Showing 7 changed files with 31 additions and 47 deletions.
3 changes: 1 addition & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down
4 changes: 2 additions & 2 deletions docs/_advanced/receiver.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
</div>

```javascript
Expand Down
2 changes: 1 addition & 1 deletion docs/_basic/listening_responding_shortcuts.md
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/App.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
54 changes: 22 additions & 32 deletions src/ExpressReceiver.ts
Original file line number Diff line number Diff line change
@@ -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?
Expand All @@ -29,14 +29,14 @@ export default class ExpressReceiver implements Receiver {

private server: Server;
private bolt: App | undefined;
private logger: Logger;

constructor({
signingSecret = '',
logger = new ConsoleLogger(),
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);

Expand All @@ -47,40 +47,36 @@ 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);
}
}

private async requestHandler(req: Request, res: Response): Promise<void> {
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<void> => {
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<void> => {
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);
}
},
};
Expand Down Expand Up @@ -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) => {
Expand Down
4 changes: 2 additions & 2 deletions src/errors.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import {
AppInitializationError,
AuthorizationError,
ContextMissingPropertyError,
ReceiverAckTimeoutError,
ReceiverAuthenticityError,
ReceiverMultipleAckError,
UnknownError,
} from './errors';

Expand All @@ -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')),
};

Expand Down
9 changes: 2 additions & 7 deletions src/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',

/**
Expand Down Expand Up @@ -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.");
Expand Down

0 comments on commit 880a008

Please sign in to comment.