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

fix: export createNodeHandler #932

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 25, 2023

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

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 25, 2023

@wolfy1339
Can you review and merge it please? :)

@wolfy1339
Copy link
Member

I don't understand the goal here, why do we need to separate that code from the middleware?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 25, 2023

@wolfy1339
The node middleware currently is also doing routing and always gets instanted.

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 ;)

@G-Rath
Copy link
Member

G-Rath commented Nov 25, 2023

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?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 25, 2023

K, when I get back at my PC i will implement a poc in probot

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 26, 2023

@G-Rath
@wolfy1339

see probot/probot#1933

I go to sleep now :)

@Uzlopak Uzlopak marked this pull request as draft November 26, 2023 02:25
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 26, 2023

I have the feeling I can improve the performance more. Have to deep dive.

@gr2m gr2m mentioned this pull request Dec 5, 2023
1 task
@Muhammadamjadm

This comment was marked as spam.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Jan 23, 2024

@Muhammadamjadm
What do you mean?

@wolfy1339
Copy link
Member

Don't worry about the comment, it's spam

@Muhammadamjadm

This comment was marked as spam.

@wolfy1339
Copy link
Member

@Uzlopak Would something like what is described in #379 be useful and helpful for you? I think it goes along with the spirit of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔥 Backlog
Development

Successfully merging this pull request may close these issues.

[FEAT]: Export createNodeHandler
4 participants