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

[FEAT]: Export createNodeHandler #930

Open
1 task done
Uzlopak opened this issue Nov 24, 2023 · 11 comments · May be fixed by #932
Open
1 task done

[FEAT]: Export createNodeHandler #930

Uzlopak opened this issue Nov 24, 2023 · 11 comments · May be fixed by #932
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 24, 2023

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

  • I agree to follow this project's Code of Conduct
@Uzlopak Uzlopak added Status: Triage This is being looked at and prioritized Type: Feature New feature or request labels Nov 24, 2023
Copy link
Contributor

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@gr2m
Copy link
Contributor

gr2m commented Nov 24, 2023

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 @octokit/middleware-* packages. We could then create custom middlewares for custom deployment targets, and invite the community to provide their own. There could be a dedicated one for serverless. Would that be something that would be interesting for you to work on?

Am I interpreting your issue correct that you are particularly interested in serverless.com? Or is it about serverless environments in general?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 24, 2023

@gr2m

I am actually investigating how we can rip out express from probot. See:

probot/probot#1930

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.
Or without a router and just check always if the webhook path is correct and call directly the webhook path and the other routes are then slower, but we dont care about their performance.

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.

@G-Rath
Copy link
Member

G-Rath commented Nov 24, 2023

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 express that is stable)

@gr2m
Copy link
Contributor

gr2m commented Nov 24, 2023

I am actually investigating how we can rip out express from probot.

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 octokit.

Probot is a framework, it should be opiniated and come with batteries included. We can replace express with something better / more performant, but there should always be a server built in. And the high-level usage of probot run ./my-app.js or new Server({ Probot }) should not expose too low-level hooks to replace these built-in batteries. Instead you should be able to bring you own server and only use probot for the parts that are relevant to you

@wolfy1339 wolfy1339 added Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR and removed Type: Feature New feature or request Status: Triage This is being looked at and prioritized labels Nov 25, 2023
@Uzlopak Uzlopak changed the title [FEAT]: Add serverless test suite to ensure serverless support [FEAT]: Export createNodeHandler Nov 25, 2023
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 25, 2023

You make valid points.

I would therefore recommend for the beginning just to get the createNodeHandler part exported.
We can later "reshuffle" if necessary. ;)

@Uzlopak Uzlopak linked a pull request Nov 25, 2023 that will close this issue
4 tasks
@gr2m
Copy link
Contributor

gr2m commented Nov 27, 2023

get the createNodeHandler part exported

we already do that already?

In Probot

https://probot.github.io/docs/development/#use-createnodemiddleware

In @octokit/webhooks

https://github.com/octokit/webhooks.js#createnodemiddleware

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Dec 2, 2023

Imho createnodemiddleware is also handling routing. If we have the pure handler, we can improve the performance.

@gr2m
Copy link
Contributor

gr2m commented Dec 5, 2023

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

@Muhammadamjadm

This comment was marked as spam.

1 similar comment
@Muhammadamjadm

This comment was marked as spam.

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
Status: 🏗 In progress
5 participants