webhooks.js

webhooks.js copied to clipboard

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

• [x] 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
• [x] No

---

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

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

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

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

@G-Rath @wolfy1339

see https://github.com/probot/probot/pull/1933

I go to sleep now :)

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

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

• [x] 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
• [x] No

@Muhammadamjadm What do you mean?

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

@G-Rath @wolfy1339

see probot/probot#1933

I go to sleep now :)

@Uzlopak Would something like what is described in https://github.com/octokit/webhooks.js/issues/379 be useful and helpful for you? I think it goes along with the spirit of this PR