-
Notifications
You must be signed in to change notification settings - Fork 393
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
Compose middleware #375
Compose middleware #375
Conversation
src/middleware/process.ts
Outdated
): Promise<unknown> { | ||
return composeMiddleware(middleware)({ | ||
...context, | ||
next: () => Promise.resolve(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An interesting thing to note here, it became quickly obvious to me why koa-compose chose to have their middleware signatures be (ctx, next)
instead of just (ctx)
with next
inside ctx
.
The reason here is that one of the purposes of ctx is to be passed around and modified in place, it's very awkward to do that when the processing has to modify the context mid stream. (As next needs to basically be the next function in the chain)
That lead to this object spread here. This will cause some (possibly) weirdness when it comes to listener middleware. Each listener middleware will have it's own context separate from the rest of them... sometimes. It would depend on the shape of context and how it was accessed and changed in processing.
I can see this being sorta helpful (partial isolation of middleware running at the same time), but could lead to unexpected results if folks are used to writing koa style middleware.
For this reason I'd still recommend using koa-style middleware rather than this one. But, if pseudo-backwards-compatibility is still more important, this works for the most part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction, not sure why this didn't click before, because all of the listener middleware is run at once, in order for them not to be constantly overriding each other's next
function, this must be spread.
Changing the signature would let us not have to do this.
7dff61b
to
016a61c
Compare
Codecov Report
@@ Coverage Diff @@
## @slack/bolt@next #375 +/- ##
====================================================
+ Coverage 83.56% 85.37% +1.81%
====================================================
Files 7 7
Lines 505 506 +1
Branches 145 144 -1
====================================================
+ Hits 422 432 +10
+ Misses 54 49 -5
+ Partials 29 25 -4
Continue to review full report at Codecov.
|
016a61c
to
ccb1c5d
Compare
ccb1c5d
to
8c6abcd
Compare
Rebased change on new branch: Still interested to discuss #375 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left one comment on a test code but others look wonderful 👍
…nto compose-middleware-resolve-conflicts
@barlock #393 brought a few conflicts with this PR. I've resolved them by barlock#2. |
^ @barlock The test failures were my bad. Your code works perfectly. I think my PR to your branch is ready to merge. Then, I'd love to merge this PR to the |
Compose middleware resolve conflicts with slackapi#393
Thanks @barlock ! I'm sure this PR is ready to merge 🎉 |
The issue has not been detected by the existing tests in Bolt v1. The PR #394 addresses the issue for Bolt v1. |
Summary
This converts all middleware to be promise based as per #353
Depends on #369
NOTE I didn't include any documentation updates as #369 got giant because of it. I'll follow this with a pr for docs alone.
Requirements (place an
x
in each[ ]
)