refactor(middleware)!: remove onUnhandledRequest middleware option

BREAKING CHANGE: onUnhandledRequest middleware option is removed

README.md

@@ -391,31 +391,6 @@ Used for internal logging. Defaults to [`console`](https://developer.mozilla.org
 
 </td>
     </tr>
-    <tr>
-      <th>
-        <code>options.onUnhandledRequest</code>
-      </th>
-      <th>
-        <code>function</code>
-      </th>
-      <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>
 </table>
 

package.json

@@ -21,7 +21,7 @@
     "@octokit/auth-app": "^4.0.0",
     "@octokit/auth-unauthenticated": "^3.0.0",
     "@octokit/core": "^4.0.0",
-    "@octokit/oauth-app": "^4.0.7",
+    "@octokit/oauth-app": "5.0.0-beta.1",
     "@octokit/plugin-paginate-rest": "^4.0.0",
     "@octokit/types": "^7.0.0",
     "@octokit/webhooks": "^10.0.0"

src/middleware/node/index.ts

@@ -4,7 +4,11 @@
 type IncomingMessage = any;
 type ServerResponse = any;
 
-import { createNodeMiddleware as oauthNodeMiddleware } from "@octokit/oauth-app";
+import {
+  createNodeMiddleware as oauthNodeMiddleware,
+  sendNodeResponse,
+  unknownRouteResponse,
+} from "@octokit/oauth-app";
 import { createNodeMiddleware as webhooksNodeMiddleware } from "@octokit/webhooks";
 
 import { App } from "../../index";
@@ -46,41 +50,44 @@ export function createNodeMiddleware(
   const webhooksMiddleware = webhooksNodeMiddleware(app.webhooks, {
     path: optionsWithDefaults.pathPrefix + "/webhooks",
     log,
+    // TODO: remove onUnhandledRequest option
     onUnhandledRequest: optionsWithDefaults.onUnhandledRequest,
   });
 
   const oauthMiddleware = oauthNodeMiddleware(app.oauth, {
     pathPrefix: optionsWithDefaults.pathPrefix + "/oauth",
-    onUnhandledRequest: optionsWithDefaults.onUnhandledRequest,
   });
 
-  return middleware.bind(null, optionsWithDefaults, {
+  return middleware.bind(
+    null,
+    optionsWithDefaults.pathPrefix,
     webhooksMiddleware,
-    oauthMiddleware,
-  });
+    oauthMiddleware
+  );
 }
 
 export async function middleware(
-  options: Required<MiddlewareOptions>,
-  { webhooksMiddleware, oauthMiddleware }: any,
+  pathPrefix: string,
+  webhooksMiddleware: any,
+  oauthMiddleware: any,
   request: IncomingMessage,
   response: ServerResponse,
   next?: Function
-) {
+): Promise<boolean> {
   const { pathname } = new URL(request.url as string, "http://localhost");
 
-  if (pathname === `${options.pathPrefix}/webhooks`) {
-    return webhooksMiddleware(request, response, next);
+  if (pathname.startsWith(`${pathPrefix}/`)) {
+    if (pathname === `${pathPrefix}/webhooks`) {
+      // TODO: omit `next` (middleware should handle request matches path)
+      webhooksMiddleware(request, response, next);
+    } else if (pathname.startsWith(`${pathPrefix}/oauth/`)) {
+      oauthMiddleware(request, response);
+    } else {
+      sendNodeResponse(unknownRouteResponse(request), response);
+    }
+    return true;
+  } else {
+    next?.();
+    return false;
   }
-  if (pathname.startsWith(`${options.pathPrefix}/oauth/`)) {
-    return oauthMiddleware(request, response, next);
-  }
-
-  const isExpressMiddleware = typeof next === "function";
-  if (isExpressMiddleware) {
-    // @ts-ignore `next` must be a function as we check two lines above
-    return next();
-  }
-
-  return options.onUnhandledRequest(request, response);
 }

src/middleware/node/on-unhandled-request-default.ts

@@ -1,3 +1,5 @@
+// TODO: remove this file once onUnhandledRequest option is gone
+
 // remove type imports from http for Deno compatibility
 // see https://github.com/octokit/octokit.js/issues/24#issuecomment-817361886
 // import { IncomingMessage, ServerResponse } from "http";

test/node-middleware.test.ts

@@ -36,30 +36,99 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w==
 -----END RSA PRIVATE KEY-----`;
 
 describe("createNodeMiddleware()", () => {
-  test("unknown route", async () => {
-    const app = new App({
-      appId: APP_ID,
-      privateKey: PRIVATE_KEY,
-      webhooks: {
-        secret: "mysecret",
-      },
-      oauth: {
-        clientId: "",
-        clientSecret: "",
-      },
+  test("unknown route (outside pathPrefix)", async () => {
+    const middleware = createNodeMiddleware({} as unknown as App);
+    const server = createServer(async (req, res) => {
+      if (!(await middleware(req, res))) {
+        res.writeHead(404, { "Content-Type": "text/plain" });
+        res.write("Not found.");
+        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}/unknown-route`);
+
+    expect(response.status).toEqual(404);
+    expect(response.headers.get("Content-Type")).toBe("text/plain");
+    await expect(response.text()).resolves.toBe("Not found.");
+
+    server.close();
+  });
+
+  test("unknown route (within pathPrefix)", async () => {
+    const middleware = createNodeMiddleware({} as unknown as App);
+    const server = createServer(async (req, res) => {
+      if (!(await middleware(req, res))) {
+        res.writeHead(404, { "Content-Type": "text/plain" });
+        res.write("Not found.");
+        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/unknown-route`
+    );
+
+    expect(response.status).toEqual(404);
+    expect(response.headers.get("Content-Type")).toBe("application/json");
+    await expect(response.json()).resolves.toEqual({
+      error: "Unknown route: GET /api/github/unknown-route",
     });
 
-    const server = createServer(createNodeMiddleware(app)).listen();
+    server.close();
+  });
+
+  test("unknown route (within webhooks path)", async () => {
+    const middleware = createNodeMiddleware({} as unknown as App);
+    const server = createServer(async (req, res) => {
+      if (!(await middleware(req, res))) {
+        res.writeHead(404, { "Content-Type": "text/plain" });
+        res.write("Not found.");
+        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}/unknown-route`);
+    const response = await fetch(
+      `http://localhost:${port}/api/github/webhooks`
+    );
 
     expect(response.status).toEqual(404);
-    await expect(response.text()).resolves.toMatch(
-      /Unknown route: GET \/unknown-route/
+    expect(response.headers.get("Content-Type")).toBe("application/json");
+    await expect(response.json()).resolves.toEqual({
+      error: "Unknown route: GET /api/github/webhooks",
+    });
+
+    server.close();
+  });
+
+  test("unknown route (within oauth pathPrefix)", async () => {
+    const middleware = createNodeMiddleware({} as unknown as App);
+    const server = createServer(async (req, res) => {
+      if (!(await middleware(req, res))) {
+        res.writeHead(404, { "Content-Type": "text/plain" });
+        res.write("Not found.");
+        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/oauth/unknown-route`
     );
 
+    expect(response.status).toEqual(404);
+    expect(response.headers.get("Content-Type")).toBe("application/json");
+    await expect(response.json()).resolves.toEqual({
+      error: "Unknown route: GET /api/github/oauth/unknown-route",
+    });
+
     server.close();
   });