Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Returning a promise from requestListener so we know when processing is done #755

Closed
4 of 10 tasks
mwildehahn opened this issue Jan 14, 2021 · 5 comments
Closed
4 of 10 tasks
Labels
auto-triage-stale discussion M-T: An issue where more input is needed to reach a decision

Comments

@mwildehahn
Copy link

Description

Hooking bolt into next.js and got it working with:

import { App, HTTPReceiver, GenericMessageEvent } from '@slack/bolt'
import { NextApiRequest, NextApiResponse } from 'next';

const receiver = new HTTPReceiver({
  endpoints: ['/api/slack/events'],
  signingSecret: process.env.SIGNING_SECRET,
  processBeforeResponse: true
})

// Initializes your app with your bot token and the AWS Lambda ready receiver
const app = new App({
  token: process.env.ACCESS_TOKEN,
  receiver,
  ignoreSelf: true
});

app.message('hi', async ({ message, say }) => {
  const msg = message as GenericMessageEvent
  await say(`:wave: <@${msg.user}>`)
})

export default receiver.requestListener;

export const config = {
  api: {
    bodyParser: false,
  }
}

Next.js will complain that:

API resolved without sending a response for /api/slack/events, this may result in stalled requests.

From here: vercel/next.js#10439 (comment) we need to return a promise so it knows when to kill the function on it's end.

Instead of wrapping in an async closure, can we just return a promise?

What type of issue is this? (place an x in one of the [ ])

  • bug
  • enhancement (feature request)
  • question
  • documentation related
  • example code related
  • testing related
  • discussion

Requirements (place an x in each of the [ ])

  • I've read and understood the Contributing guidelines and have done my best effort to follow them.
  • I've read and agree to the Code of Conduct.
  • I've searched for any related issues and avoided creating a duplicate issue.
@gitwave gitwave bot added the untriaged label Jan 14, 2021
@stevengill stevengill added discussion M-T: An issue where more input is needed to reach a decision and removed untriaged labels Jan 14, 2021
@aoberoi
Copy link
Contributor

aoberoi commented Jan 14, 2021

This is an interesting idea. Returning a Promise would deviate from the function signature of the standard Node request listener. I'm not opposed to adjusting that signature a bit if it adds value and doesn't cause any issues.

Here's the issue I think it might cause: If a Promise is returned and the caller does nothing to handle it (no await, no .then()/.catch()), and the Promise rejects, then we could potentially crash the program due to an UnhandledPromiseRejectionWarning. Some of us may have seen this related warning in our consoles.

Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Aside from that issue, I think it would technically be a breaking change. Currently, when the HTTPReceiver defers handling a specific request, it synchronously throws. Calling code would need to be adjusted to handle promise rejects (also known as async throws).

These problems both feel a little theoretical at this point, esp since this release shipped so recently. I'm open to your thoughts on it.

Meanwhile, I'm going to go create a little toy Next.js app to see if I have any bright ideas/suggestions. I'll report back.

@aoberoi
Copy link
Contributor

aoberoi commented Jan 14, 2021

I got a Next.js app working without the warning. Got it deployed to Vercel as well.

I split things up a bit so that the pages/api/slack/[...all].js file just sets up the HTTP receiver.

Then in lib/bolt.js i did all the normal Bolt things.

What do you think?

@mwildehahn
Copy link
Author

mwildehahn commented Jan 14, 2021 via email

@seratch
Copy link
Member

seratch commented Mar 23, 2021

If I understand the above discussion, it seems we can close this issue now. I will close this after waiting for a few more days.

@github-actions
Copy link

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-triage-stale discussion M-T: An issue where more input is needed to reach a decision
Projects
None yet
Development

No branches or pull requests

4 participants