Skip to content

Remove onUnhandledRequest middleware option#341

Merged
gr2m merged 2 commits intooctokit:betafrom
baoshan:remove_on-unhandled-request-handler
Sep 10, 2022
Merged

Remove onUnhandledRequest middleware option#341
gr2m merged 2 commits intooctokit:betafrom
baoshan:remove_on-unhandled-request-handler

Conversation

@baoshan
Copy link
Copy Markdown
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 gr2m, oscard0m and wolfy1339 and removed request for wolfy1339 September 7, 2022 16:22
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

🎉 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