feat: createNodeMiddleware()

[gr2m]

fixes #507. fixes #510
Part of octokit/octokit.js#15

TODOs

• implement log option: createNodeMiddleware(webhooks, { log })

---

View rendered README.md
View rendered src/middleware-legacy/README.md

[G-Rath]

Have left a few nity comments - the test ones are a bit more nity (a result of being a core maintainer of eslint-plugin-jest 😅)

[G-Rath]

Suggested change

	expect(response.status).toEqual(200);
	expect(response.status).toBe(200);

nit: prefer using toBe for literal matchers (or otherwise toStrictEqual)/

[gr2m]

could you elaborate, I'd like to better understand your point. I use .equalTo for comparing simple types such as numbers. I don't think there is a difference to .toStrictEqual() when comparing numbers?

[G-Rath]

because this is async, ideally you should use the optional done callback or (preferably) create a promise that gets resolved in this event, which you can await at the end of the test body.

[gr2m]

I'm not sure if I follow you here. The event handler is guaranteed to be executed before sending a response, as long as it doesn't take more than 9s, so the code should be executed before the await fetch below resolves

[gr2m]

@octokit/maintainers this is good to go now

Ignore white space when reviewing changes: https://github.com/octokit/webhooks.js/pull/509/files?w=1

[gr2m]

I intentionally left out the x-hub-signature header here, we want to nudge users to use best practices. GHES support is good enough for the new header

[wolfy1339]

Nothing stands out to me.

There were 2 changes that weren't strictly related to this PR

[wolfy1339]

This isn't strictly related to this PR, but I get the changes

[gr2m]

yeah sorry for that, you are totally right, I've been lazy creating separate PRs for that. Same with the formatting changes in the README, I've to show octokit on Wednesday, I'm in a bit of a rush

[wolfy1339]

Not strictly related to this PR.

[gr2m]

I have to go ahead and merge this, but please review and let me know if I missed something, I'll fix it in a follow up PR.

I'll also start a beta release PR for v9 that I can use for @octokit/app and octokit

[github-actions]

🎉 This PR is included in version 8.9.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[honnix]

Thanks for making the change!

With this change, what is the correct way to set middleware to express app?

The deprecated way as following still works, that's great.

const { Webhooks, createNodeMiddleware } = require("@octokit/webhooks");
const webhooks = new Webhooks({
  secret: "mysecret",
  path: "/webhooks",
});

webhooks.onAny(({ id, name, payload }) => {
  console.log(name, "event received");
});

const express = require("express");
const app = express();

app.use(webhooks.middleware);

app.get("/", function (req, res) {
  res.send("hello world");
});

app.listen(3000);

However I tried the following, but it doesn't work well. It seems the middleware hijacks all routes and I got

$ curl localhost:3000/
{"error":"Required headers missing: x-github-event, x-hub-signature-256, x-github-delivery"}% 

const { Webhooks, createNodeMiddleware } = require("@octokit/webhooks");
const webhooks = new Webhooks({
  secret: "mysecret",
});

webhooks.onAny(({ id, name, payload }) => {
  console.log(name, "event received");
});

const express = require("express");
const app = express();

app.use(createNodeMiddleware(webhooks, { path: "/webhooks" }));

app.get("/", function (req, res) {
  res.send("hello world");
});

app.listen(3000);

I tried to do app.use after setting up routes then it works. Is this intended?

[honnix]

I guess the new middleware stops forwarding to next route? We put the middleware before other routes because webhooks URL is much more frequently hit. If this is intended change we can live with it.

[gr2m]

I guess the new middleware stops forwarding to next route

that was an unintended change, it slipped due to lack of tests, sorry for that! And thanks for the PR 👍🏼