-
Notifications
You must be signed in to change notification settings - Fork 80
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
[FEAT]: Export createNodeHandler #930
Comments
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with |
I'm pretty sure that Octokit supports serverless. If there was a known problem with https://www.serverless.com/ specifically then we could add a test to address it, but adding tests just for the sake of it, I'm not sure if that's helpful? Maybe we should do the tests in Probot instead? On a related note, I always wanted to remove all the "middleware" functionalities into their own Am I interpreting your issue correct that you are particularly interested in serverless.com? Or is it about serverless environments in general? |
I am actually investigating how we can rip out express from probot. See: Maybe providing the middlware is ok, but what I would prefer is exporting the handler out. Then I could use in probot a fast router, like find-my-way, which we use in fastify and just mount the webhook handler to the right place in the router. We use at work probot with gcf and gcf expects actually a express middleware. But if we dont use any express specific stuff, we increase the compatibility with other serverless implementations like aws or azure. |
fwiw I'm using these libraries without issue (express or otherwise) in both AWS Lambdas and Azure Functions - most of them are internal for Ackama, but this is a public example (I am in support of reducing dependencies in general where it is possible to do so without a heavy increase in maintenance burden so will happily support PRs reducing any direct dependencies on |
I think the right approach here is to make probot decomposable. We started that effort already: https://probot.github.io/docs/development/#run-probot-programmatically Eventually it should be decomposable to a smooth eject of Probot altogether, into Probot is a framework, it should be opiniated and come with batteries included. We can replace |
You make valid points. I would therefore recommend for the beginning just to get the createNodeHandler part exported. |
we already do that already? In Probot https://probot.github.io/docs/development/#use-createnodemiddleware In https://github.com/octokit/webhooks.js#createnodemiddleware |
Imho createnodemiddleware is also handling routing. If we have the pure handler, we can improve the performance. |
Okay that makes sense, I'm okay with the direction #932 is going 👍 just update the README to include the new export and explain the difference between the two exports |
Describe the need
If we support serverless in the middleware, we can promote probot to be serverless compatible... well maybe after some minor changes in probot.From the discussion we come to the conclusion, that serverless tests should be done in probot. But I think we need therefore export the Handler.
SDK Version
No response
API Version
No response
Relevant log output
No response
Code of Conduct
The text was updated successfully, but these errors were encountered: