Skip to content

Conversation

@honnix
Copy link
Contributor

@honnix honnix commented Apr 15, 2021

As I explained here, it is now required to always install this middleware to the end of the chain, otherwise it rejects all requests it cannot handle.

honnix and others added 2 commits April 15, 2021 21:44
As I explained [here](octokit#509 (comment)), it is now required to always install this middleware to the end of the chain, otherwise it rejects all requests it cannot handle.
@honnix honnix changed the title Pass on to the next middleware in case of express fix: Pass on to the next middleware in case of express Apr 15, 2021
@honnix honnix changed the title fix: Pass on to the next middleware in case of express fix: pass on to the next middleware in case of express Apr 15, 2021
@honnix
Copy link
Contributor Author

honnix commented Apr 15, 2021

@gr2m PTAL and help me understand whether this is a valid change. Thanks a lot.

@honnix
Copy link
Contributor Author

honnix commented Apr 15, 2021

Sorry I couldn't figure out how to add a correct label. I guess this is a bug.

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented, or is being fixed label Apr 15, 2021
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you @honnix 👍🏼 I'm not very familiar with express, if there is anything we can do to make it easier to use for express users, please let us know.

I also made follow up issue to make sure we test the express integration in @octokit/oauth-app and @octokit/app: octokit/app.js#236, octokit/oauth-app.js#222

@gr2m gr2m merged commit 07f19fe into octokit:master Apr 17, 2021
@github-actions
Copy link
Contributor

🎉 This PR is included in version 9.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@honnix
Copy link
Contributor Author

honnix commented Apr 17, 2021

@gr2m Thanks for approving and merging. I cannot say myself familiar with express either but I thinkthe added test cases should help catch similar problem in the future.

@honnix honnix deleted the honnix/patch-1 branch April 17, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working as documented, or is being fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants