Allows the use of supplied `Router` object to `ExpressReceiver` #870

rr-codes · 2021-04-08T04:41:05Z

Summary

The goal of this PR is to allow for using a custom express router by an express app to be used for ExpressReceiver instead of having the receiver itself own the express app and router. This enhancement also relates to #212 and the upward modularity philosophy.

From a software design perspective, this isn't the most sensical design ever, as it essentially defeats the purpose of the Receiver interface more or less, but it gets the job done and is usable without requiring a major refactor to Bolt.

In this implementation, if a Router is supplied to the constructor, no express app or router is created. As a result, there is nothing for start or stop to do, so both are no-op in this case and return a Promise which resolves to void.

Closes #868 .

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.

…ver.ts

CLAassistant · 2021-04-08T04:41:10Z

All committers have signed the CLA.

codecov · 2021-04-08T04:42:41Z

Codecov Report

Merging #870 (8389038) into main (23b4f80) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

❗ Current head 8389038 differs from pull request most recent head 56ba51f. Consider uploading reports for the commit 56ba51f to get more accurate results

@@            Coverage Diff             @@
##             main     #870      +/-   ##
==========================================
- Coverage   66.08%   66.05%   -0.03%     
==========================================
  Files          13       13              
  Lines        1200     1205       +5     
  Branches      353      359       +6     
==========================================
+ Hits          793      796       +3     
- Misses        338      339       +1     
- Partials       69       70       +1

Impacted Files	Coverage Δ
src/receivers/ExpressReceiver.ts	`66.66% <75.00%> (-0.21%)`	⬇️

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 23b4f80...56ba51f. Read the comment docs.

stevengill

I'd be lying if I said setting app to undefined doesn't make me uncomfortable. I could see this type change causing some confusion for some developers.

To me, it seems you just want to take advantage of the receivers ability to setup routes (like slack/events) and use the built in middleware.

Passing in your own router does really make ExpressReceiver pretty useless.

You could plug your bolt app into existing Express apps already via something like #212 (comment). What benefit would this approach have over the one in the comment?

src/receivers/ExpressReceiver.ts

seratch · 2021-04-09T02:39:15Z

src/receivers/ExpressReceiver.ts

@@ -33,6 +34,7 @@ export interface ExpressReceiverOptions {
  installationStore?: InstallProviderOptions['installationStore']; // default MemoryInstallationStore
  scopes?: InstallURLOptions['scopes'];
  installerOptions?: InstallerOptions;
+  router?: core.Router;


IRouter should be a more relevant type for this argument as express.Application instance inherits the type not core.Router.

ahh gotcha, thanks! Tbh I didn't know IRouter even existed 😅

seratch · 2021-04-09T02:41:31Z

src/receivers/ExpressReceiver.ts

-  ): Promise<Server | HTTPSServer> {
+  ): Promise<Server | HTTPSServer | void> {
+    if (!this.app) {
+      return Promise.resolve();


How about adding logs here? For instance,

this.logger.info('As the Express app instance is not managed by this receiver, #start() method does nothing.');

or something like that. If it's fine, we can do the same for the stop() method

Ya that seems like a good idea to me!

seratch · 2021-04-09T02:43:50Z

Thanks for working on this! For your information, I've created my suggestion branch, which is based on your branch: seratch@b11b04b

As I did in the branch, adding a new example app for this use case may be a good addition. What do you think? @stevengill

seratch · 2021-04-09T02:55:47Z

Ah, sorry - I overlooked @stevengill's above comment.

So, is one possible solution to enable passing express.Application (not a router) in the constructor? With this way, we can pass express.Application from external code and we don't need to unnecessarily call express() internally. Also, we don't need to change start/stop methods at all (perhaps, those methods won't be used in this scenario though).

Am I missing something?

stevengill · 2021-04-09T23:46:42Z

Passing App is an interesting idea. Start and Stop would still be useless but atleast the types won't look off. What do you think @rr-codes

rr-codes · 2021-04-09T23:57:23Z

Hey @stevengill @seratch sorry for the delay!

Passing App is an interesting idea. Start and Stop would still be useless but atleast the types won't look off. What do you think @rr-codes

This is interesting! But in start, since it will be doing nothing in this case, how can we make the port parameter optional in this specific case? Since the parameter wouldn't be doing anything.

seratch · 2021-04-10T23:39:00Z

@rr-codes I may not understand your concern about the start method yet.

When you directly use express.Application#listen method instead, Bolt's App#start method won't be used in the scenario. If you want to use start method with the external Application instance, creating a new HTTP server with the express app instance in the method should work. Let us know if there is anything else we should consider.

stevengill · 2021-04-13T22:26:32Z

Hey @rr-codes

I agree with Kaz, I don't think the developer will have to call start so they won't have to worry about the port. Or am I missing something obvious?

rr-codes · 2021-04-14T01:21:19Z

@seratch @stevengill oh true ok ya I was confusing myself. So yup I agree with both of you!

rr-codes · 2021-04-14T01:27:22Z

I'd be lying if I said setting app to undefined doesn't make me uncomfortable. I could see this type change causing some confusion for some developers.

To me, it seems you just want to take advantage of the receivers ability to setup routes (like slack/events) and use the built in middleware.

Passing in your own router does really make ExpressReceiver pretty useless.

You could plug your bolt app into existing Express apps already via something like #212 (comment). What benefit would this approach have over the one in the comment?

also @stevengill you mentioned this alternative; however, receiver is a private field of App, so this doesn't even seem possible?

seratch · 2021-04-21T22:12:20Z

@rr-codes

however, receiver is a private field of App, so this doesn't even seem possible?

This is correct. app.receiver.router does not work in TS. Although it's technically possible to access private fields in JS as the example there does, it's not allowed in TS.

By the way, are you going to work on this one very soon? No rush at all but I'm thinking about the release timing of v3.4. If we need more time for this PR, it's totally fine to move this one to v3.5 milestone.

rr-codes · 2021-04-21T22:17:01Z

@seratch hey, so ya I've been working on this, and I've been thinking about the possibilities of making the receiver act as an express middleware, so like expressApp.use(myReceiver). I think this approach would be better than passing in the express App as this is more idiomatic Express and is a common use.

So I've just been trying to figure out how to / if its even possible to accomplish this.

What do you think?

seratch · 2021-04-21T22:58:18Z

@rr-codes If I understand correctly, it should be possible to implement a Receiver as an Express middleware and I agree that's a clearer design. However, the receiver can be a bit different thing from the current ExpressReceiver. More importantly, our top priority for built-in receivers is to keep backward compatibility for existing apps (including the use cases where an app uses ExpressReceiver#router for adding custom routes).

For these reasons, my suggestion here is to continue exploring a greater design for a while and decide where to go once we've figured it out. Let me move this from v3.4 scopes from future versions (v3.5 as a tentative plan). Thanks for your efforts here!

rr-codes · 2021-04-21T23:59:22Z

@seratch thoughts on my most recent commit? I extracted all the middleware logic into its own class, which can be used standalone with any receiver. I really like this approach as it provides much more flexibility, without affecting ExpressReceiver too much.

seratch · 2021-04-22T02:45:01Z

@rr-codes
The change may be nice refactoring, and it can be beneficial for 3rd party Receiver implementations. However, I think the current implementation could cause breaking changes. RequestHandlers like respondToSslCheck are already exported from ExpressReceiver. Changing the location is not backward compatible.

To safely expose the main RequestHandler (= the private method named as requestHandler), as you did, it may be fine to have a new holder class and provide a way to get the set of RequestHandlers. The class can refer to the existing "stateless" RequestHandlers without changing their locations.

Going back to the original topic you raised at #868, the latest revision is no longer a solution for improving ExpressReceiver. Are you more interested in building your own Express-based Receiver now? If so, let's stop working on this pull request and start a new conversation in a new task issue.

I also think that the built-in ExpressReceiver should allow developers to attach Bolt apps to an external Express application. I believe the fix for it would be much simpler than this one. I'm happy to continue working on the task #868 .

seratch · 2021-08-25T23:00:13Z

As #1084 resolves the issue #868 in a simpler way, let us close this pull request now.

I am still thinking that some of the changes here can be great addition. If we want to continue for other improvement purposes, we are happy to do so. Thanks for taking the time to make these change suggestions here!

Richard Robinson added 2 commits April 7, 2021 14:19

added ability to use custom express app and/or router in ExpressRecei…

5ef6291

…ver.ts

removed needless options / logic

dcaf5b0

gitwave bot added the untriaged label Apr 8, 2021

seratch requested review from stevengill, seratch and mwbrooks April 8, 2021 05:00

seratch added enhancement M-T: A feature request for new functionality semver:minor and removed untriaged labels Apr 8, 2021

seratch added this to the 3.4.0 milestone Apr 8, 2021

Richard Robinson added 3 commits April 8, 2021 01:05

added tests

2da2da9

fixed formatting

d795683

fixed linting

436853f

stevengill reviewed Apr 8, 2021

View reviewed changes

src/receivers/ExpressReceiver.ts Outdated Show resolved Hide resolved

seratch added a commit to seratch/bolt-js that referenced this pull request Apr 9, 2021

suggestions for pull request slackapi#870

b11b04b

seratch reviewed Apr 9, 2021

View reviewed changes

Merge branch 'main' into main

e13797d

seratch modified the milestones: 3.4.0, 3.5.0 Apr 21, 2021

redesigned express receiver

56ba51f

seratch removed this from the 3.5.0 milestone Jul 2, 2021

seratch closed this Aug 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows the use of supplied `Router` object to `ExpressReceiver` #870

Allows the use of supplied `Router` object to `ExpressReceiver` #870

rr-codes commented Apr 8, 2021

CLAassistant commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

stevengill left a comment

seratch Apr 9, 2021

rr-codes Apr 9, 2021

seratch Apr 9, 2021

rr-codes Apr 9, 2021

seratch commented Apr 9, 2021

seratch commented Apr 9, 2021 •

edited

Loading

stevengill commented Apr 9, 2021

rr-codes commented Apr 9, 2021

seratch commented Apr 10, 2021

stevengill commented Apr 13, 2021

rr-codes commented Apr 14, 2021

rr-codes commented Apr 14, 2021

seratch commented Apr 21, 2021

rr-codes commented Apr 21, 2021

seratch commented Apr 21, 2021

rr-codes commented Apr 21, 2021

seratch commented Apr 22, 2021

seratch commented Aug 25, 2021

Allows the use of supplied Router object to ExpressReceiver #870

Allows the use of supplied Router object to ExpressReceiver #870

Conversation

rr-codes commented Apr 8, 2021

Summary

Requirements (place an x in each [ ])

CLAassistant commented Apr 8, 2021 • edited Loading

codecov bot commented Apr 8, 2021 • edited Loading

Codecov Report

stevengill left a comment

Choose a reason for hiding this comment

seratch Apr 9, 2021

Choose a reason for hiding this comment

rr-codes Apr 9, 2021

Choose a reason for hiding this comment

seratch Apr 9, 2021

Choose a reason for hiding this comment

rr-codes Apr 9, 2021

Choose a reason for hiding this comment

seratch commented Apr 9, 2021

seratch commented Apr 9, 2021 • edited Loading

stevengill commented Apr 9, 2021

rr-codes commented Apr 9, 2021

seratch commented Apr 10, 2021

stevengill commented Apr 13, 2021

rr-codes commented Apr 14, 2021

rr-codes commented Apr 14, 2021

seratch commented Apr 21, 2021

rr-codes commented Apr 21, 2021

seratch commented Apr 21, 2021

rr-codes commented Apr 21, 2021

seratch commented Apr 22, 2021

seratch commented Aug 25, 2021

Allows the use of supplied `Router` object to `ExpressReceiver` #870

Allows the use of supplied `Router` object to `ExpressReceiver` #870

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

CLAassistant commented Apr 8, 2021 •

edited

Loading

codecov bot commented Apr 8, 2021 •

edited

Loading

seratch commented Apr 9, 2021 •

edited

Loading