Skip to content
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

Remove onUnhandledRequest middleware option #341

Merged
merged 2 commits into from
Sep 10, 2022

Conversation

baoshan
Copy link
Contributor

@baoshan baoshan commented Sep 7, 2022

This PR is based on the idea that Octokit middlewares (not limited to oauth-app.js) respect below proposed principle:

  1. handle all requests matching pathPrefix (returns a predictable 404 response for unknown routes)
  2. do not handle requests outside pathPrefix (but could be easily customized in an idiomatic way of the specific framework/runtime)

This PR also removes the (already deprecated) Cloudflare handler because this is a breaking change which leads to a major version bump.

@baoshan baoshan changed the title Remove on unhandled request handler Remove onUnhandledRequest handler Sep 7, 2022
@baoshan baoshan changed the title Remove onUnhandledRequest handler Remove onUnhandledRequest middleware option Sep 7, 2022
@baoshan baoshan marked this pull request as draft September 7, 2022 12:49
@oscard0m oscard0m added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Sep 7, 2022
@oscard0m oscard0m requested review from wolfy1339 and gr2m and removed request for wolfy1339 September 7, 2022 16:22
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.

I suggest we release this as a beta to verify that @octokit/app still works without the onUnhandledRequest hook and to verify that you can easily build middleware.

@baoshan baoshan force-pushed the remove_on-unhandled-request-handler branch 2 times, most recently from 882f7ac to 4e5da8a Compare September 9, 2022 09:04
@baoshan baoshan force-pushed the remove_on-unhandled-request-handler branch from 4e5da8a to 6f5f025 Compare September 9, 2022 09:25
@baoshan
Copy link
Contributor Author

baoshan commented Sep 9, 2022

There is also a draft PR for app.js. I’d appreciate it if someone could give both a review.

@gr2m gr2m changed the base branch from master to beta September 9, 2022 17:41
@gr2m
Copy link
Contributor

gr2m commented Sep 9, 2022

I've updated both PRs to merge into beta, so we are save to merge.

Can you add BREAKING CHANGE: ... sections to the pull request description so we don't miss that when we merge?

BREAKING CHANGE: drop onUnhandledRequest middleware option support
@baoshan baoshan force-pushed the remove_on-unhandled-request-handler branch from 6f5f025 to f14ff6d Compare September 10, 2022 02:44
@baoshan
Copy link
Contributor Author

baoshan commented Sep 10, 2022

Thanks.

I’ve appended a BREAKING CHANGE: line to the commit message if that’s what you mean.

@baoshan baoshan marked this pull request as ready for review September 10, 2022 02:47
@gr2m gr2m merged commit 4fd0a76 into octokit:beta Sep 10, 2022
@github-actions
Copy link

🎉 This PR is included in version 5.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants