Convert say, ack, and respond to return promises #369

barlock · 2020-01-10T15:58:24Z

Summary

Makes say, ack, and respond return promises. Per discussion in #353

I possibly changed a few things from the discussion above, I'll leave comments.

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.

claassistantio · 2020-01-10T15:58:42Z

All committers have signed the CLA.

barlock · 2020-01-10T15:59:11Z

package.json

@@ -29,7 +29,7 @@
    "build": "tsc",
    "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
    "lint": "tslint --project .",
-    "test-lint": "tslint \"src/**/*.spec.ts\" && tslint \"src/test-helpers.ts\"",
+    "test-lint": "tslint --project tsconfig.test.json \"src/**/*.spec.ts\" && tslint --project tsconfig.test.json \"src/test-helpers.ts\"",


I wanted to be able to lint the test cases for proper async handling, that requires types in tslint, so I made a config specifically for test.

barlock · 2020-01-10T16:00:43Z

src/App.ts

        const postMessageArguments: ChatPostMessageArguments = (typeof message === 'string') ?
          { token, text: message, channel: channelId } : { ...message, token, channel: channelId };
-        this.client.chat.postMessage(postMessageArguments)
+        return this.client.chat.postMessage(postMessageArguments)


I thought this made more sense resolving the result of this call rather than void.

Enables folks who want to the ability to get things like the ts off the result for later updating.

codecov · 2020-01-10T16:00:52Z

Codecov Report

Merging #369 into master will increase coverage by 0.26%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   83.29%   83.56%   +0.26%     
==========================================
  Files           7        7              
  Lines         497      505       +8     
  Branches      143      145       +2     
==========================================
+ Hits          414      422       +8     
  Misses         54       54              
  Partials       29       29

Impacted Files	Coverage Δ
src/ExpressReceiver.ts	`72.38% <100%> (+2.27%)`	⬆️
src/App.ts	`87.35% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8469052...7735013. Read the comment docs.

barlock · 2020-01-10T16:01:46Z

src/ExpressReceiver.ts

@@ -82,7 +82,7 @@ export default class ExpressReceiver extends EventEmitter implements Receiver {
    );
    const event: ReceiverEvent = {
      body: req.body as { [key: string]: any },
-      ack: (response: any): void => {
+      ack: async (response: any): Promise<void> => {


Express's res.send is actually synchronous and won't ever error. I believe this still makes sense to force the signature to a promise for other types of receivers.

barlock · 2020-01-10T16:02:44Z

src/ExpressReceiver.ts

-            this.emit('error', e);
-          });
+      event.respond = async (response) => {
+        await this.axios.post(req.body.response_url, response);


I removed the catch so this will behave more like promises would expect. Left unchecked in the new middleware pattern, it will be bubbled up to the global error handler anyway I figured this was safe and more expected.

Update: I'm going to update this pr to move this block into onIncommingEvent so that every receiver doesn't need to implement it.

Incoming shortly.

seratch · 2020-01-14T15:48:31Z

docs/_basic/acknowledging_requests.md

  // it’s a valid email, accept the submission
  if (isEmail.test(action.submission.email)) {
-    ack();
+    await ack();
  } else {
    // if it isn’t a valid email, acknowledge with an error
    ack({


nit: Is it intentional not to have await here?

🤦‍♂ Not a nit, great catch. After missing this, I took another pass searching for these and updated a few more 🎉

If only we could lint js inside markdown.

seratch · 2020-01-14T15:48:49Z

docs/_basic/ja_acknowledging_requests.md

  // メールアドレスが有効。ダイアログを受信
  if (isEmail.test(action.submission.email)) {
-    ack();
+    await ack();
  } else {
    // メールアドレスが無効。エラーを確認
    ack({


(same as above)

seratch · 2020-01-14T15:50:53Z

package.json

@@ -29,7 +29,7 @@
    "build": "tsc",
    "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
    "lint": "tslint --project .",
-    "test-lint": "tslint \"src/**/*.spec.ts\" && tslint \"src/test-helpers.ts\"",
+    "test-lint": "tslint --project tsconfig.test.json \"src/**/*.spec.ts\" && tslint --project tsconfig.test.json \"src/test-helpers.ts\"",


seratch · 2020-01-14T15:55:44Z

src/App.ts

@@ -400,10 +418,10 @@ export default class App {
    // Factory for say() utility
    const createSay = (channelId: string): SayFn => {
      const token = context.botToken !== undefined ? context.botToken : context.userToken;
-      return (message: Parameters<SayFn>[0]) => {
+      return (message: Parameters<SayFn>[0]): Promise<void | WebAPICallResult> => {


I'm still thinking but it may be better to return Promise<WebAPICallResult | CodedError> here even after applying onGlobalError. Any thoughts?

I clearly wasn't thinking straight here, I removed the global error handler as it will be called anyway if this goes uncaught.

This means that this returns what chat.postMessage is which is simply Promise<WebAPICallResult>

I just checked on CodedError, looks cool, I would think the right thing to do would be to update @slack/web-api to handle that rather than bolt. That way you have a consistent api between say and client.chat.postMessage What do you think?

🙏 for the late reply here: I agree that we can remove the global error handler here to be consistent with others such as app.client calls and respond 👍

seratch · 2020-01-14T15:58:14Z

src/App.ts

-      listenerArgs.respond = respond;
+    if (body.response_url) {
+      listenerArgs.respond = async (response): Promise<any> => {
+        const postResponse = await this.axios.post(body.response_url, response);


Related to above, respond does await here. This means respond may return a failure while the current say doesn't. Also, should we be consistent about when to apply App's global error handler?

Awesome catch, yes, I meant to remove the global handler catch from say as well. I'll update that.

barlock · 2020-01-15T14:21:27Z

Updated, added new commit 8469052

seratch

This PR looks great to me. Just in case, could you take a look at this? @aoberoi @stevengill

barlock · 2020-01-22T23:32:13Z

@aoberoi @stevengill Sorry for the bump, but it's been a while.

I have the one pr stacked on this one (#375) and another branch in progress. It would be great to start getting feedback soon or merge this so I don't have to keep merging up!

barlock · 2020-01-24T13:42:25Z

src/App.spec.ts

@@ -761,30 +814,6 @@ describe('App', () => {
          // Assert
          assert.equal(assertionAggregator.callCount, dummyReceiverEvents.length);
        });
-
-        it('should handle failures through the App\'s global error handler', async () => {


I believe this is failing b/c the global handler doesn't know how to handle thrown errors (yet). I'm adding this test back in a later pr. Will link it when it's up.

Added back 269f462#diff-a7a8d4758cfeb70999a809869b45fca0R976

aoberoi · 2020-02-03T13:14:53Z

src/App.spec.ts

+          fakeReceiver.emit('message', { // IncomingEventType.Action (app.action)
+            body: {
+              type: 'block_actions',
+              response_url: responseUrl,


nit: block_actions type events will never have a response_url. not a big deal since the functionality that's being tested is independent of the actual event type.

Oh! didn't know, the types lists it as a valid response. https://github.com/slackapi/bolt/blob/master/src/types/actions/block-action.ts#L220

I tend to not use response_urls in favor of actual actions, so I'm not always sure which ones do.

Is that a bug then?

aoberoi · 2020-02-03T14:29:40Z

@barlock this is amazing work! i'm sorry my review took longer than i had initially hoped. you were super thorough and i appreciate that. i'm landing this now, and moving on to review the other PRs you've sent.

aoberoi · 2020-02-03T14:33:55Z

Notes for producing the changelog:

Implements changes to say(), ack(), and respond() from Proposal: Chaining middleware using Promises #353
say() returns a Promise<WebAPICallResult>
Receivers no longer need to implement respond() for incoming messages. Now a Receiver can be implemented without importing yet another HTTP client.

barlock commented Jan 10, 2020

View reviewed changes

stevengill added the enhancement M-T: A feature request for new functionality label Jan 10, 2020

seratch reviewed Jan 14, 2020

View reviewed changes

Michael Barlock added 4 commits January 15, 2020 09:20

Convert say, ack, and respond to return promises

f08787a

Update docs and readme

a0609b6

Add test cases for respond arg

2d5f05b

Update documentation and say return values

8469052

barlock force-pushed the receiver-promises branch from 5794b8f to 8469052 Compare January 15, 2020 14:20

seratch approved these changes Jan 17, 2020

View reviewed changes

seratch requested review from aoberoi and stevengill January 17, 2020 05:28

barlock mentioned this pull request Jan 17, 2020

Compose middleware #375

Merged

2 tasks

Michael Barlock added 2 commits January 23, 2020 00:03

Merge branch 'master' into receiver-promises

d000c36

Move response url tests

7735013

barlock commented Jan 24, 2020

View reviewed changes

This was referenced Jan 24, 2020

[v2] Convert receivers to use App#processEvent #380

Merged

[v2] Add documentation for async middleware and App#processEvent #381

Merged

stevengill added this to the V2 milestone Jan 30, 2020

stevengill assigned aoberoi Jan 30, 2020

seratch mentioned this pull request Jan 30, 2020

Extract some functionality to make writing custom App/Receiver easier #388

Closed

9 tasks

aoberoi changed the base branch from master to @slack/bolt@next February 3, 2020 12:58

aoberoi approved these changes Feb 3, 2020

View reviewed changes

aoberoi merged commit 5881471 into slackapi:@slack/bolt@next Feb 3, 2020

barlock deleted the receiver-promises branch February 3, 2020 17:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert say, ack, and respond to return promises #369

Convert say, ack, and respond to return promises #369

barlock commented Jan 10, 2020

claassistantio commented Jan 10, 2020 •

edited

Loading

barlock Jan 10, 2020

seratch Jan 14, 2020

barlock Jan 10, 2020

codecov bot commented Jan 10, 2020 •

edited

Loading

barlock Jan 10, 2020

barlock Jan 10, 2020

barlock Jan 10, 2020

seratch Jan 14, 2020

barlock Jan 15, 2020 •

edited

Loading

barlock Jan 15, 2020

seratch Jan 14, 2020

seratch Jan 14, 2020

seratch Jan 14, 2020

barlock Jan 15, 2020

seratch Jan 17, 2020

seratch Jan 14, 2020

barlock Jan 15, 2020

barlock commented Jan 15, 2020

seratch left a comment

barlock commented Jan 22, 2020

barlock Jan 24, 2020

barlock Jan 24, 2020

aoberoi Feb 3, 2020

barlock Feb 3, 2020

aoberoi commented Feb 3, 2020

aoberoi commented Feb 3, 2020

Convert say, ack, and respond to return promises #369

Convert say, ack, and respond to return promises #369

Conversation

barlock commented Jan 10, 2020

Summary

Requirements (place an x in each [ ])

claassistantio commented Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2020 • edited Loading

Codecov Report

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

barlock Jan 15, 2020 • edited Loading

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

Choose a reason for hiding this comment

barlock commented Jan 15, 2020

seratch left a comment

Choose a reason for hiding this comment

barlock commented Jan 22, 2020

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

aoberoi commented Feb 3, 2020

aoberoi commented Feb 3, 2020

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

claassistantio commented Jan 10, 2020 •

edited

Loading

codecov bot commented Jan 10, 2020 •

edited

Loading

barlock Jan 15, 2020 •

edited

Loading