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

Removes the onUnhandledRequest middleware option #740

Merged
merged 5 commits into from
Sep 12, 2022

Conversation

baoshan
Copy link
Contributor

@baoshan baoshan commented Sep 11, 2022

expected to be merged into beta branch

What?

This PR removes the onUnhandledRequest middleware option.

Why?

To respect a proposed general rule that all Octokit middlewares (oauth-app.js, webhooks.js, app.js):

  • do NOT handle requests outside pathPrefix (or path) option but provide a natural way for concrete runtimes/frameworks to handle unhandled requests
  • always handle requests within pathPrefix (or path) option (returns 404 for unknown routes)

But my code relies on the onUnhandledRequest option!

Because webhook.js only exposes a (shared) middleware for Node.js and Express.js middleware currently, the natural way (to handle unhandled requests) is (AFAICT):

  • For native Node.js (http.createServer), check returned boolean value:
const middleware = createNodeMiddleware(webhooks, {});
createServer(async (req, res) => {
  if (await middleware(req, res)) return;
  res.writeHead(404);
  res.end();
}).listen();
  • For Express.js, rely on proper handler(s) registered at your desired path(s).

View rendered README.md

BREAKING CHANGE: middleware only and (always) handles requests at `path`
@wolfy1339 wolfy1339 added Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request labels Sep 11, 2022
@wolfy1339 wolfy1339 changed the base branch from master to beta September 11, 2022 12:50
@baoshan baoshan marked this pull request as ready for review September 11, 2022 14:05
@wolfy1339
Copy link
Member

Test failures are unrelated to this PR.

They should have been caught in #732

@gr2m
Copy link
Contributor

gr2m commented Sep 11, 2022

Because webhook.js only exposes a (shared) middleware for Node.js and Express.js middleware currently, the natural way (to handle unhandled requests) is (AFAICT):

* For native Node.js (`http.createServer`), check returned boolean value:
const middleware = createNodeMiddleware(webhooks, {});
createServer(async (req, res) => {
  if (await middleware(req, res)) return;
  res.writeHead(404);
  res.end();
}).listen();

Should we add that to the README?

renovate bot and others added 2 commits September 11, 2022 22:13
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@baoshan
Copy link
Contributor Author

baoshan commented Sep 12, 2022

Thanks! I’ve fixed that.

@baoshan baoshan force-pushed the remove_on-unhandled-request branch from 74f1421 to 453f55b Compare September 12, 2022 02:57
@wolfy1339 wolfy1339 merged commit b454be6 into octokit:beta Sep 12, 2022
@github-actions
Copy link
Contributor

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

The release is available on:

Your semantic-release bot 📦🚀

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

🎉 This PR is included in version 10.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

wolfy1339 pushed a commit that referenced this pull request Oct 3, 2022
* refactor(middleware)!: remove onUnhandledRequest middleware option

BREAKING CHANGE: middleware only and (always) handles requests at `path`

* build(deps): lock file maintenance (#742)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* fix(doc): document way to handle unhandled requests

* refactor(middleware): remove unused type declarations

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
wolfy1339 pushed a commit that referenced this pull request Oct 3, 2022
* refactor(middleware)!: remove onUnhandledRequest middleware option

BREAKING CHANGE: middleware only and (always) handles requests at `path`

* fix(doc): document way to handle unhandled requests
* refactor(middleware): remove unused type declarations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Breaking change Used to note any change that requires a major version bump Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants