Cleanup Lambda example and docs around `proccessBeforeResponse` #1229

ramblingenzyme · 2021-12-02T21:39:46Z

Summary

The information supplied around processBeforeResponse was rather confusing and contradictory.

The lambda example passes processBeforeResponse to the App constructor, but the AwsLambdaReciever class implements the correct behaviour & the argument isn't used in the constructor unless there isn't a receiver passed. Therefore I removed the argument and comment.
The example listener calls ack at the end to respond to the request, but this contradicts other documentation that says to call it as early as possible, and the receiver will defer it until afterwards anyway. So I've moved ack to the top, otherwise I would've reworded the comment above it.
I've reworded the reference documentation around processBeforeResponse to explain the behaviour more explicitly
Because it's a reference document and people will jump directly to the section they're interested in, I've added some information about HttpReceiver arguments being passed to the App constructor under the App options header and explained how those arguments are used.

Requirements (place an `x` in each `[ ]`)

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.

…rguments

ramblingenzyme · 2021-12-02T22:03:12Z

General feedback:

processBeforeResponse is a confusing name and doesn't explain what's being processed before response
The App constructor constructing a Receiver for you is confusing and a breaking change to require a receiver to be passed in and remove the HttpReceiver arguments from the App constructor would make initialising an App less confusing.

ramblingenzyme · 2021-12-02T22:54:35Z

I'd also be happy to look into raising a separate PR with some overload signatures for the App constructor to exclude HttpReceiver arguments when a receiver has been passed, if that would be valuable.

seratch · 2021-12-02T23:09:49Z

docs/_deployments/aws-lambda.md

@@ -140,12 +140,7 @@ const awsLambdaReceiver = new AwsLambdaReceiver({
 // Initializes your app with your bot token and the AWS Lambda ready receiver
 const app = new App({
    token: process.env.SLACK_BOT_TOKEN,
-    receiver: awsLambdaReceiver,
-    // The `processBeforeResponse` option is required for all FaaS environments.


Instead of deleting this, we want to say "If you go with AwsLambdaReceiver like this example does, you can omit processBeforeResponse: true. But, if you use other receivers such as ExpressReceiver for OAuth flow support, processBeforeResponse: true is required."

See also:

Update AWS Lambda deployment guide to have notice about OAuth support #1062

Slack Bolt's built-in oauth with AWS Lambda #1028

https://github.com/slackapi/bolt-js/pull/1066/files#diff-2130e8227ce7a48a4f16812225568f8c3bca634968bd620e3c47fe9fa5f692c3R130

Sounds good, I'll rework this then.

seratch · 2021-12-02T23:11:09Z

docs/_tutorials/reference.md

@@ -89,7 +89,7 @@ Bolt includes a collection of initialization options to customize apps. There ar
 | :---: | :--- |
 | `signingSecret` | A `string` from your app's configuration (under "Basic Information") which verifies that incoming events are coming from Slack |
 | `endpoints` | A `string` or `object` that specifies the endpoint(s) that the receiver will listen for incoming requests from Slack. Currently, the only key for the object is `key`, the value of which is the customizable endpoint (ex: `/myapp/events`). **By default, all events are sent to the `/slack/events` endpoint** |
-| `processBeforeResponse` | `boolean` that determines whether events should be immediately acknowledged. This is primarily useful when running apps on FaaS to ensure events listeners don't unexpectedly terminate by setting it to `true`. Defaults to `false`.  |
+| `processBeforeResponse` | `boolean` that determines whether events should be immediately acknowledged. This is primarily useful when running apps on FaaS since listeners will terminate immediately once the request has completed. When set to `true` it will defer sending the acknowledgement until after your handlers run to prevent early termination. Defaults to `false`.  |


This is a clearer description. Thanks for the suggestion 👍

seratch · 2021-12-02T23:21:13Z

docs/_tutorials/reference.md

@@ -98,7 +98,7 @@ Bolt includes a collection of initialization options to customize apps. There ar
 | `installerOptions` | Optional object that can be used to customize [the default OAuth support](/bolt-js/concepts#authenticating-oauth). Read more in the OAuth documentation. |

 ### App options
-App options are passed into the `App` constructor.
+App options are passed into the `App` constructor. The `App` constructor also accepts [`HttpReceiver`](#receiver-options) options which will be used to initialise a [Receiever](/bolt-js/concepts#receiver) when the `receiver` argument is `undefined` and are ignored otherwise.


Mentioning HttpReceiver options is correct but it can be confusing for the people new to bolt-js. We can simply say the receiver options above are also available in the argument list.

Suggested change

App options are passed into the `App` constructor. The `App` constructor also accepts [`HttpReceiver`](#receiver-options) options which will be used to initialise a [Receiever](/bolt-js/concepts#receiver) when the `receiver` argument is `undefined` and are ignored otherwise.

App options are passed into the `App` constructor. Also, when the `receiver` argument is `undefined`, the `App` constructor accepts [the above receiver options](#receiver-options) to initialize an appropriate `Receiver` for you. `

I didn't mention HTTPReceiver because it can be SocketModeReceiver if socketMode: true.

Ah, right. I had my blinders on when making this change I didn't realise it was the generic Receiver options and not specific to HttpReceiver. How would you feel about mentioning socketMode and calling out the specific Receivers that could be constructed?

Yeah, that sounds good to me 👍

seratch · 2021-12-02T23:25:46Z

examples/deploy-aws-lambda/app.js

-  // It allows Bolt methods (e.g. `app.message`) to handle a Slack request
-  // before the Bolt framework responds to the request (e.g. `ack()`). This is
-  // important because FaaS immediately terminate handlers after the response.
-  processBeforeResponse: true


Comment out this option is fine but as I mentioned above, we would like to keep the comment for OAuth flow use cases.

See also:

Update AWS Lambda deployment guide to have notice about OAuth support #1062

Slack Bolt's built-in oauth with AWS Lambda #1028

https://github.com/slackapi/bolt-js/pull/1066/files#diff-2130e8227ce7a48a4f16812225568f8c3bca634968bd620e3c47fe9fa5f692c3R130

seratch · 2021-12-02T23:26:09Z

examples/deploy-aws-lambda/app.js

@@ -43,10 +38,9 @@ app.message('hello', async ({ message, say }) => {

 // Listens for an action from a button click
 app.action('button_click', async ({ body, ack, say }) => {
-  await say(`<@${body.user.id}> clicked the button`);
-
-  // Acknowledge the action after say() to exit the Lambda process


Thanks. Indeed, this comment can be confusing.

… you need to use it

seratch

one more minor request 🙇

docs/_tutorials/reference.md

Standardize on American spelling of initialize Co-authored-by: Kazuhiro Sera <[email protected]>

seratch

Thanks a lot for your time and efforts!

ramblingenzyme · 2021-12-03T01:29:56Z

Thank you for making it a great experience! It shouldn't be unusual, and yet...

ramblingenzyme added 2 commits December 2, 2021 17:08

Update docs and examples around processBeforeRespons

58e26d8

Reword information about the App constructor accepting HttpReceiver a…

674a7be

…rguments

seratch requested changes Dec 2, 2021

View reviewed changes

seratch self-assigned this Dec 2, 2021

seratch added area:examples issues related to example or sample code docs M-T: Documentation work only labels Dec 2, 2021

seratch added this to the 3.9.0 milestone Dec 2, 2021

ramblingenzyme added 2 commits December 3, 2021 11:30

Reword App options information explaining Receiver options more clearly

aaec7d8

Add processBeforeResponse back to lambda examples but explaining when…

2906b39

… you need to use it

ramblingenzyme requested a review from seratch December 3, 2021 00:38

seratch reviewed Dec 3, 2021

View reviewed changes

docs/_tutorials/reference.md Outdated Show resolved Hide resolved

Update docs/_tutorials/reference.md

388ab4b

Standardize on American spelling of initialize Co-authored-by: Kazuhiro Sera <[email protected]>

ramblingenzyme requested a review from seratch December 3, 2021 01:21

seratch mentioned this pull request Dec 3, 2021

Japanese docs: #1229 changes (FaaS document improvements) #1230

Closed

10 tasks

seratch approved these changes Dec 3, 2021

View reviewed changes

seratch merged commit a1da64d into slackapi:main Dec 3, 2021

wongjas mentioned this pull request Jul 20, 2022

Adds JP translation for processBeforeResponse & AWS guide #1524

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Lambda example and docs around `proccessBeforeResponse` #1229

Cleanup Lambda example and docs around `proccessBeforeResponse` #1229

ramblingenzyme commented Dec 2, 2021

ramblingenzyme commented Dec 2, 2021

ramblingenzyme commented Dec 2, 2021

seratch Dec 2, 2021

ramblingenzyme Dec 3, 2021

seratch Dec 2, 2021

seratch Dec 2, 2021

ramblingenzyme Dec 3, 2021

seratch Dec 3, 2021

seratch Dec 2, 2021

seratch Dec 2, 2021

seratch left a comment

seratch left a comment

ramblingenzyme commented Dec 3, 2021

	App options are passed into the `App` constructor. The `App` constructor also accepts [`HttpReceiver`](#receiver-options) options which will be used to initialise a [Receiever](/bolt-js/concepts#receiver) when the `receiver` argument is `undefined` and are ignored otherwise.
	App options are passed into the `App` constructor. Also, when the `receiver` argument is `undefined`, the `App` constructor accepts [the above receiver options](#receiver-options) to initialize an appropriate `Receiver` for you. `

Cleanup Lambda example and docs around proccessBeforeResponse #1229

Cleanup Lambda example and docs around proccessBeforeResponse #1229

Conversation

ramblingenzyme commented Dec 2, 2021

Summary

Requirements (place an x in each [ ])

ramblingenzyme commented Dec 2, 2021

ramblingenzyme commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

seratch left a comment

Choose a reason for hiding this comment

ramblingenzyme commented Dec 3, 2021

Cleanup Lambda example and docs around `proccessBeforeResponse` #1229

Cleanup Lambda example and docs around `proccessBeforeResponse` #1229

Requirements (place an `x` in each `[ ]`)