Fix #355 - processMiddleware doesn't wait for last global middleware's call to next()

[artgillespie]

Summary

This provides a test case for #355 (see src/middleware/process.spec.ts) as well as a proposed fix.

What I think is happening is that for the last global middleware, processMiddleware is passing a noop next function and calling next on the next process tick. This leads to unexpected results when the last (or only) global middleware is async, as is the case if you implement a conversation store as outlined here

I had to update a couple global middleware implementations in App.spec.ts that were relying on this behavior and not calling next().

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.

[codecov]

Codecov Report

Merging #394 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #394   +/-   ##
=======================================
  Coverage   81.33%   81.33%           
=======================================
  Files           7        7           
  Lines         525      525           
  Branches      150      150           
=======================================
  Hits          427      427           
  Misses         70       70           
  Partials       28       28

---

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 074a0f0...7a04c34. Read the comment docs.

[seratch]

Thanks for fixing this! LGTM. We should merge this for Bolt 1.x series. Bolt v2 no longer has this issue (the new implementation by @barlock addressed the issue)

[seratch]

codecov/project — 81.33% (+-2.35%) compared to 95d35f6

I'd like to eliminate this 2% drop. I'll make a pull request to your branch.

[seratch]

codecov/patch — Coverage not affected when comparing 074a0f0...7a04c34

hmm, it's a bit weird but now it seems to be fine. Looks ready to merge anyways.

@aoberoi @stevengill I'd like to merge this PR tomorrow in my local time. Take a look at this when you have a chance until then.

[artgillespie]

@seratch That is weird. I couldn't figure out how coverage had dropped after I merged latest master. Thanks for looking!

[seratch]

Let me merge this now.
@artgillespie Thank you very much for your great contribution! 🎉

[artgillespie]

@seratch @stevengill I was just thinking that if folks using bolt upgrade to a release with this fix and had custom global middleware that incorrectly relied on this behavior (specifically, that didn't call next()) this fix will break their installation until they figure it out. (I saw this with a few App.spec.ts tests when I was putting together the PR.) I'm new around here, so I'm not familiar with the release / changelog process, but if I can help document this for the community to avoid frustration, please let me know!

[seratch]

@artgillespie
This is crucial. Thanks for the great suggestion 👍

I think communication can be like this.

As described in issue #355, the global middleware (the ones given with app.use) had an issue on the way to handle next() invocation. Since version 1.7.0, we have addressed the issue. When upgrading to the version, we recommend all users making sure if all existing global middleware surely call next() inside.

Probably, the English sentence can be polished by others, but I think we should have the above message in the release note and the document in English/Japanese.

Any thoughts? @stevengill @aoberoi @shaydewael