-
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
fix: export createNodeHandler #932
base: main
Are you sure you want to change the base?
Conversation
d7d8f30
to
7bb7de6
Compare
@wolfy1339 |
I don't understand the goal here, why do we need to separate that code from the middleware? |
@wolfy1339 If the handler is separated and exported we can directly use it, instantiate it once and mount it in the router. This should improve the performance significantly. If this gets merged, i can provide a follow up PR in few hours and everything should make more sense ;) |
Once we start shipping this it will be a breaking change to remove so we need it to "make more sense" before its merged, not after. Are you able to provide some proper benchmarks showing the performance gains you're talking about? Have you also considered inling this within probot so you can start living with it for a while and gather some feedback to help justify us adopting it here? |
K, when I get back at my PC i will implement a poc in probot |
I go to sleep now :) |
I have the feeling I can improve the performance more. Have to deep dive. |
This comment was marked as spam.
This comment was marked as spam.
@Muhammadamjadm |
Don't worry about the comment, it's spam |
Resolves #930
I would like to have the middleware separated from the handler to use it in probot.
Resolves #ISSUE_NUMBER
Before the change?
After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!