feat: handleRequest. Deprecate onUnhandledRequest middleware option

[baoshan]

To:

1. be idiomatic for all runtimes and frameworks (Node.js / Express.js / Deno / AWS Lambda / Cloudflare), and
2. simplify the implementation of a specific middleware

this PR deprecates the onUnhandledRequest option.

Without the onUnhandledRequest option, middlewares are simpler and more idiomatic, which can be verified in my next PR #341.

[timrogers]

Thanks for taking the time to make this PR ✨

What's the impact on the user of not being able to define an onUnhandledRequest handler? What would people realistically use that for? Reducing the complexity of creating middleware is a good thing, but I'm wondering what we lose.

[wolfy1339]

For reference, this was split from #333

In that PR it was mentioned:

I removed the onUnhandledRequest option. Callers can still custom unhandled requests: use the express * router (as covered by the test cases) or check if the response is a 404.

[timrogers]

Thanks! Do all of the current middleware use Express? Or would the pattern for doing that be different across middleware? For example, it doesn't look like the Lambda one uses Express.

[baoshan]

This is a great question!

I believe the below behavior for the generic handler (aka handle-request.ts) is more reasonable and predictable:

• Returns null when the request is not under the pathPrefix (e.g. GET /foo)
• Return 404 for unhandled requests which is under the pathPrefix (e.g., GET /api/github/oauth/foo)

All middlewares naturally share the same pattern when dealing with /api/github/oauth/foo. How they deal with /foo is different by the nature of the runtime/framework:

• For native Node.js (createServer), the middleware could return true when the request is handled (response already written to res) or false when not handled. You can check the return value and render a custom response. Anyway, we name it as createNodeMiddleware, not createNodeHandler, right?
• Express.js relies on proper router middlewares for other requests.
• For Cloudflare / Deno / AWS Lambda, the middleware should simply map the generic handler: returns a Response/APIGatewayProxyStructuredResultV2 for requests at pathPrefix or null for other requests.

Does that make sense? If not, may I know your use case?

[gr2m]

Do all of the current middleware use Express

No. Hence the onUnhandledRequest option. This middleware is meant to be universal, it should run in Deno and browsers and future JavaScript runtime environments. We cannot assume express or even Node APIs to be present

If I recall correctly, I think we depend on onUnhandledRequest to compose middlewares. E.g. the @octokit/app module is using @oauth-app.js internally and uses onUnhandledRequest to add more routes. But I didn't check the code

[baoshan]

We cannot assume express or even Node APIs to be present

I understand that. That’s exactly why this PR is proposed.

the @octokit/app module is using @oauth-app.js internally

Thanks for that information. I believe @octokit/app can be updated (or even made simpler) with onUnhandledRequest removed (I’m willing to help.)

First, I suggest all the middlewares (the cartesian product of [@octokit/app, @octokit/oauth-app] ⨯ [Node.js, Express.js, Deno, Browser, Cloudflare, AWS Lambda]) do the same thing:

• Do not handle the request when the request does not match pathPrefix (e.g. GET /foo)
• Return 404 for unhandled requests matching pathPrefix (e.g., GET /api/github/oauth/foo)

Second, injecting the onUnhandledRequest is not the idiomatic way for all runtimes or frameworks. Actually is not the idiomatic way for any runtime/framework IMHO.

Making middlewares idiomatic is more important than unifying the interface. A more functional approach (#341) makes Deno / Cloudflare / AWS Lambda feels better, IMHO.

[wolfy1339]

@gr2m @oscard0m Can you re-review this so this can get merged

[gr2m]

beautiful, thank you!!!

all requests have been addressed

[github-actions]

🎉 This PR is included in version 4.1.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[github-actions]

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

The release is available on:

• GitHub release
• npm package (@beta dist-tag)

Your semantic-release bot 📦🚀