Removes the onUnhandledRequest middleware option (#740)

* refactor(middleware)!: remove onUnhandledRequest middleware option BREAKING CHANGE: middleware only and (always) handles requests at `path` * fix(doc): document way to handle unhandled requests * refactor(middleware): remove unused type declarations
octokit · Oct 3, 2022 · cfd77ad · cfd77ad
1 parent 3af01d5
commit cfd77ad
Show file tree

Hide file tree

Showing 5 changed files with 46 additions and 79 deletions.
diff --git a/README.md b/README.md
@@ -556,12 +556,19 @@ const webhooks = new Webhooks({
   secret: "mysecret",
 });
 
-const middleware = createNodeMiddleware(webhooks, { path: "/" });
-
-createServer(middleware).listen(3000);
-// can now receive user authorization callbacks at POST /
+const middleware = createNodeMiddleware(webhooks, { path: "/webhooks" });
+createServer(async (req, res) => {
+  // `middleware` returns `false` when `req` is unhandled (beyond `/webhooks`)
+  if (await middleware(req, res)) return;
+  res.writeHead(404);
+  res.end();
+}).listen(3000);
+// can now receive user authorization callbacks at POST /webhooks
 ```
 
+The middleware returned from `createNodeMiddleware` can also serve as an
+`Express.js` middleware directly.
+
 <table width="100%">
   <tbody valign="top">
     <tr>
@@ -597,32 +604,6 @@ createServer(middleware).listen(3000);
 
 Used for internal logging. Defaults to [`console`](https://developer.mozilla.org/en-US/docs/Web/API/console) with `debug` and `info` doing nothing.
 
-</td>
-    </tr>
-    <tr>
-      <td>
-        <code>onUnhandledRequest</code>
-        <em>
-          function
-        </em>
-      </td>
-      <td>
-
-Defaults to
-
-```js
-function onUnhandledRequest(request, response) {
-  response.writeHead(400, {
-    "content-type": "application/json",
-  });
-  response.end(
-    JSON.stringify({
-      error: error.message,
-    })
-  );
-}
-```
-
 </td>
     </tr>
   <tbody>

diff --git a/src/middleware/node/index.ts b/src/middleware/node/index.ts
@@ -1,20 +1,17 @@
 import { createLogger } from "../../createLogger";
 import { Webhooks } from "../../index";
 import { middleware } from "./middleware";
-import { onUnhandledRequestDefault } from "./on-unhandled-request-default";
 import { MiddlewareOptions } from "./types";
 
 export function createNodeMiddleware(
   webhooks: Webhooks,
   {
     path = "/api/github/webhooks",
-    onUnhandledRequest = onUnhandledRequestDefault,
     log = createLogger(),
   }: MiddlewareOptions = {}
 ) {
   return middleware.bind(null, webhooks, {
     path,
-    onUnhandledRequest,
     log,
   } as Required<MiddlewareOptions>);
 }
diff --git a/src/middleware/node/middleware.ts b/src/middleware/node/middleware.ts
@@ -11,14 +11,15 @@ import { WebhookEventHandlerError } from "../../types";
 import { MiddlewareOptions } from "./types";
 import { getMissingHeaders } from "./get-missing-headers";
 import { getPayload } from "./get-payload";
+import { onUnhandledRequestDefault } from "./on-unhandled-request-default";
 
 export async function middleware(
   webhooks: Webhooks,
   options: Required<MiddlewareOptions>,
   request: IncomingMessage,
   response: ServerResponse,
   next?: Function
-) {
+): Promise<boolean> {
   let pathname: string;
   try {
     pathname = new URL(request.url as string, "http://localhost").pathname;
@@ -31,17 +32,15 @@ export async function middleware(
         error: `Request URL could not be parsed: ${request.url}`,
       })
     );
-    return;
+    return true;
   }
 
-  const isUnknownRoute = request.method !== "POST" || pathname !== options.path;
-  const isExpressMiddleware = typeof next === "function";
-  if (isUnknownRoute) {
-    if (isExpressMiddleware) {
-      return next!();
-    } else {
-      return options.onUnhandledRequest(request, response);
-    }
+  if (pathname !== options.path) {
+    next?.();
+    return false;
+  } else if (request.method !== "POST") {
+    onUnhandledRequestDefault(request, response);
+    return true;
   }
 
   const missingHeaders = getMissingHeaders(request).join(", ");
@@ -56,7 +55,7 @@ export async function middleware(
       })
     );
 
-    return;
+    return true;
   }
 
   const eventName = request.headers["x-github-event"] as WebhookEventName;
@@ -85,13 +84,14 @@ export async function middleware(
     });
     clearTimeout(timeout);
 
-    if (didTimeout) return;
+    if (didTimeout) return true;
 
     response.end("ok\n");
+    return true;
   } catch (error) {
     clearTimeout(timeout);
 
-    if (didTimeout) return;
+    if (didTimeout) return true;
 
     const err = Array.from(error as WebhookEventHandlerError)[0];
     const errorMessage = err.message
@@ -106,5 +106,7 @@ export async function middleware(
         error: errorMessage,
       })
     );
+
+    return true;
   }
 }
diff --git a/src/middleware/node/types.ts b/src/middleware/node/types.ts
@@ -1,16 +1,6 @@
-// remove type imports from http for Deno compatibility
-// see https://github.com/octokit/octokit.js/issues/24#issuecomment-817361886
-// import { IncomingMessage, ServerResponse } from "http";
-type IncomingMessage = any;
-type ServerResponse = any;
-
 import { Logger } from "../../createLogger";
 
 export type MiddlewareOptions = {
   path?: string;
   log?: Logger;
-  onUnhandledRequest?: (
-    request: IncomingMessage,
-    response: ServerResponse
-  ) => void;
 };
diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts
@@ -7,7 +7,7 @@ import { sign } from "@octokit/webhooks-methods";
 // import without types
 const express = require("express");
 
-import { Webhooks, createNodeMiddleware } from "../../src";
+import { createNodeMiddleware, Webhooks } from "../../src";
 
 const pushEventPayload = readFileSync(
   "test/fixtures/push-payload.json",
@@ -168,39 +168,36 @@ describe("createNodeMiddleware(webhooks)", () => {
     server.close();
   });
 
-  test("custom non-found handler", async () => {
+  test("handle unhandled requests", async () => {
     const webhooks = new Webhooks({
       secret: "mySecret",
     });
 
-    const server = createServer(
-      createNodeMiddleware(webhooks, {
-        onUnhandledRequest(_request, response) {
-          response.writeHead(404);
-          response.end("nope");
-        },
-      })
-    ).listen();
+    const middleware = createNodeMiddleware(webhooks, {});
+    const server = createServer(async (req, res) => {
+      if (!(await middleware(req, res))) {
+        res.writeHead(404, { "Content-Type": "text/plain" });
+        res.write("nope.");
+        res.end();
+      }
+    }).listen();
 
     // @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
     const { port } = server.address();
 
-    const response = await fetch(
-      `http://localhost:${port}/api/github/webhooks`,
-      {
-        method: "PUT",
-        headers: {
-          "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
-          "X-GitHub-Event": "push",
-          "X-Hub-Signature-256": signatureSha256,
-        },
-        body: "invalid",
-      }
-    );
+    const response = await fetch(`http://localhost:${port}/foo`, {
+      method: "PUT",
+      headers: {
+        "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
+        "X-GitHub-Event": "push",
+        "X-Hub-Signature-256": signatureSha256,
+      },
+      body: "invalid",
+    });
 
     expect(response.status).toEqual(404);
 
-    await expect(response.text()).resolves.toEqual("nope");
+    await expect(response.text()).resolves.toEqual("nope.");
 
     server.close();
   });