Runtime-agnostic HTTP handler #247
Conversation
We still support NodeJS v10 for now, until our next major version bump |
There was a problem hiding this comment.
Okay this is quite a lot 😁 I like your approach, but I think you've done two steps at once. I think it would have been a better approach to first create a Cloudflare Worker middleware and get it merged. And once we have that we could have looked into abstractions. But with only two use cases I fear that an abstraction might be premature and cause more work down the path, I'd probably looked into yet another middleware that is as different as possible to Node's and Cloudflare's.
But that's okay now, I'm just thinking out loud, let's be pragmatic about it. For now, can you please address my comments?
Could you also please add README.md files with some explanations in src/middleware (what is this folder / how to build your own middleware) and src/midleware/node (how to use the node/express middleware)
baoshan
force-pushed
the
runtime-agnostic-middleware
branch
7 times, most recently
from
ec73b49 to
1755cd2
Compare
gr2m
force-pushed
the
runtime-agnostic-middleware
branch
from
1755cd2 to
3852189
Compare
|
I resolved the conflict and workedaround a typescript error. I had to force-push the changes so watch out if you continue working on it. This also triggered the full CI on the PR |
| 3. Expose an HTTP handler/middleware in the dialect of the environment which performs three steps: | ||
| 1. Parse the HTTP request using (1). | ||
| 2. Process the `OctokitRequest` object using `handleRequest`. If the request is not handled by `handleRequest` (the request does not match any predefined route), [`onUnhandledRequestDefault`](on-unhandled-request-default.ts) can be used to generate a `404` response consistently. | ||
| 3. Render the `OctokitResponse` object using (2). |
There was a problem hiding this comment.
thanks for the README, this is great!
gr2m
left a comment
gr2m
left a comment
There was a problem hiding this comment.
Thanks a lot, I really like how readable the node middleware code is now!
To share the bigger picture of middlewares in Octokit land: we do have the concept several @octokit/* modules, some of which depend on each other.
@octokit/webhooks@octokit/oauth-app@octokit/app- depends on@octokit/webhookss and@octokit/oauth-app's middlewareoctokit- uses@octokit/app's middleware
I think once we are happy with the implementation in @octokit/oauth-app, it would make sense to look to move the abstract middleware to handler requests/responses into its own module, so that the other packages can utilize it. No need to worry about it now, just wanted to mention it so you better understand the context of where I'm coming from
| @@ -1,59 +1,21 @@ | |||
| // remove type imports from http for Deno compatibility | |||
| // see https://github.com/octokit/octokit.js/issues/24#issuecomment-817361886 | |||
| // see https://github.com/octokit/octokit.js/issues/2075#issuecomment-817361886 | |||
There was a problem hiding this comment.
good catch, thank you for fixing it!
|
🎉 This PR is included in version 3.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
4 participants
Follow up to octokit/auth-oauth-user-client.js#1 (comment)
This commit distills the original Node.js/Express middleware into a runtime agnostic HTTP handler which operates on a generalized request/response interface rather than
IncomingMessage.This handler hopefully can better support different runtimes (Cloudflare/Deno/AWS Lambda) with the same behavior, so the client authentication strategy can also be backend-or-platform-agnostic.
The new Node.js/Express middleware relies on the generalized HTTP handler. Test file is untouched to respect the current behavior.
fromentriesdependency is removed since Node.js v10 is out of maintanence.