Repository: octokit/oauth-app.js
Pull request #340 (2022-09-07 03:08:42)

feat: `handleRequest`. Deprecate `onUnhandledRequest` middleware option
To:

1. be idiomatic for all runtimes and frameworks (Node.js / Express.js / Deno / AWS Lambda / Cloudflare), and 
2. simplify the implementation of a specific middleware

this PR deprecates the `onUnhandledRequest` option.

Without the `onUnhandledRequest` option, middlewares are simpler and more idiomatic, which can be verified in my next PR #341.

timrogers: Thanks for taking the time to make this PR ✨ 

What's the impact on the user of not being able to define an `onUnhandledRequest` handler? What would people realistically use that for? Reducing the complexity of creating middleware is a good thing, but I'm wondering what we lose.
wolfy1339: For reference, this was split from https://github.com/octokit/oauth-app.js/pull/333

In that PR it was mentioned:

> I removed the onUnhandledRequest option. Callers can still custom unhandled requests: use the express * router (as covered by the test cases) or check if the response is a 404.
timrogers: Thanks! Do all of the current middleware use Express? Or would the pattern for doing that be different across middleware? For example, it doesn't look like the Lambda one uses Express.
gr2m: > Do all of the current middleware use Express

No. Hence the `onUnhandledRequest` option. This middleware is meant to be universal, it should run in Deno and browsers and future JavaScript runtime environments. We cannot assume `express` or even Node APIs to be present
baoshan: > We cannot assume `express` or even Node APIs to be present

I understand that. That’s exactly why this PR is proposed.

> the `@octokit/app` module is using @oauth-app.js internally

Thanks for that information. I believe `@octokit/app` can be updated (or even made simpler) with `onUnhandledRequest` removed (I’m willing to help.)

First, I suggest all middlewares do the same thing:

- Returns null when the request does not match `pathPrefix` (e.g. `GET /foo`)
- Return `404` for unhandled requests matching `pathPrefix` (e.g., `GET /api/github/oauth/foo`)

Second, injecting the `onUnhandledRequest` is not the idiomatic way for all runtimes or frameworks. Actually is not the idiomatic way for any runtime/framework IMHO.

Making middlewares idiomatic is more important than unifying the interface. A more functional approach (#341) makes Deno / Cloudflare / AWS Lambda feels better, IMHO.

wolfy1339: @gr2m @oscard0m Can you re-review this so this can get merged

Patch:
diff --git a/README.md b/README.md
index c3898ed..5c1db44 100644
--- a/README.md
+++ b/README.md
@@ -939,7 +939,7 @@ All exposed paths will be prefixed with the provided prefix. Defaults to `"/api/
     </tr>
     <tr>
       <th>
-        <code>options.onUnhandledRequest</code>
+        <code>options.onUnhandledRequest</code> __deprecated__
       </th>
       <th>
         <code>function</code>
@@ -1029,7 +1029,7 @@ All exposed paths will be prefixed with the provided prefix. Defaults to `"/api/
     </tr>
     <tr>
       <th>
-        <code>options.onUnhandledRequest</code>
+        <code>options.onUnhandledRequest</code> __deprecated__
       </th>
       <th>
         <code>function</code>
@@ -1119,7 +1119,7 @@ All exposed paths will be prefixed with the provided prefix. Defaults to `"/api/
     </tr>
     <tr>
       <th>
-        <code>options.onUnhandledRequest</code>
+        <code>options.onUnhandledRequest</code> __deprecated__
       </th>
       <th>
         <code>function</code>
@@ -1145,6 +1145,81 @@ function onUnhandledRequest(request) {
   </tbody>
 </table>
 
+### Build Custom Middlewares
+
+When above middlewares do not meet your needs, you can build your own
+using the exported `handleRequest` function.
+
+[`handleRequest`](./src/middleware/handle-request.ts) function is an abstract HTTP handler which accepts an `OctokitRequest` and returns an `OctokitResponse` if the request matches any predefined route.
+
+> Different environments (e.g., Node.js, Cloudflare Workers, Deno, etc.) exposes different APIs when processing HTTP requests (e.g., [`IncomingMessage`](https://nodejs.org/api/http.html#http_class_http_incomingmessage) for Node.js, [`Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) for Cloudflare workers, etc.). Two HTTP-related types ([`OctokitRequest` and `OctokitResponse`](.src/middleware/types.ts)) are generalized to make an abstract HTTP handler possible.
+
+To share the behavior and capability with the existing Node.js middleware (and be compatible with [OAuth user authentication strategy in the browser](https://github.com/octokit/auth-oauth-user-client.js)), it is better to implement your HTTP handler/middleware based on `handleRequest` function.
+
+`handleRequest` function takes three parameters:
+
+<table width="100%">
+  <thead align=left>
+    <tr>
+      <th width=150>
+        name
+      </th>
+      <th width=70>
+        type
+      </th>
+      <th>
+        description
+      </th>
+    </tr>
+  </thead>
+  <tbody align=left valign=top>
+    <tr>
+      <th>
+        <code>app</code>
+      </th>
+      <th>
+        <code>OAuthApp instance</code>
+      </th>
+      <td>
+        <strong>Required</strong>.
+      </td>
+    </tr>
+    <tr>
+      <th>
+        <code>options.pathPrefix</code>
+      </th>
+      <th>
+        <code>string</code>
+      </th>
+      <td>
+
+All exposed paths will be prefixed with the provided prefix. Defaults to `"/api/github/oauth"`
+
+</td>
+    </tr>
+    <tr>
+      <th>
+        <code>request</code>
+      </th>
+      <th>
+        <code>OctokitRequest</code>
+      </th>
+      <td>
+        Generalized HTTP request in `OctokitRequest` type.
+      </td>
+    </tr>
+  </tbody>
+</table>
+
+Implementing an HTTP handler/middleware for a certain environment involves three steps:
+
+1. Write a function to parse the HTTP request (e.g., `IncomingMessage` in Node.js) into an `OctokitRequest` object. See [`node/parse-request.ts`](.src/middleware/node/parse-request.ts) for reference.
+2. Write a function to render an `OctokitResponse` object (e.g., as `ServerResponse` in Node.js). See [`node/send-response.ts`](.src/middleware/node/send-response.ts) for reference.
+3. Expose an HTTP handler/middleware in the dialect of the environment which performs three steps:
+   1. Parse the HTTP request using (1).
+   2. Process the `OctokitRequest` object using `handleRequest`.
+   3. Render the `OctokitResponse` object using (2).
+
 ## Contributing
 
 See [CONTRIBUTING.md](CONTRIBUTING.md)
diff --git a/src/index.ts b/src/index.ts
index a129bb2..353d4de 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -50,6 +50,17 @@ import type {
   Options,
   State,
 } from "./types";
+
+// types required by external handlers (aws-lambda, etc)
+export type {
+  HandlerOptions,
+  OctokitRequest,
+  OctokitResponse,
+} from "./middleware/types";
+
+// generic handlers
+export { handleRequest } from "./middleware/handle-request";
+
 export { createNodeMiddleware } from "./middleware/node/index";
 export {
   createCloudflareHandler,
diff --git a/src/middleware/aws-lambda/api-gateway-v2.ts b/src/middleware/aws-lambda/api-gateway-v2.ts
index 873c9ae..7eea54a 100644
--- a/src/middleware/aws-lambda/api-gateway-v2.ts
+++ b/src/middleware/aws-lambda/api-gateway-v2.ts
@@ -4,7 +4,7 @@ import { handleRequest } from "../handle-request";
 import { onUnhandledRequestDefault } from "../on-unhandled-request-default";
 import { HandlerOptions } from "../types";
 import { OAuthApp } from "../../index";
-import { Options, ClientType } from "../../types";
+import { ClientType, Options } from "../../types";
 import type {
   APIGatewayProxyEventV2,
   APIGatewayProxyStructuredResultV2,
@@ -22,16 +22,22 @@ export function createAWSLambdaAPIGatewayV2Handler(
   app: OAuthApp<Options<ClientType>>,
   {
     pathPrefix,
-    onUnhandledRequest = onUnhandledRequestDefaultAWSAPIGatewayV2,
+    onUnhandledRequest,
   }: HandlerOptions & {
     onUnhandledRequest?: (
       event: APIGatewayProxyEventV2
     ) => Promise<APIGatewayProxyStructuredResultV2>;
   } = {}
 ) {
+  if (onUnhandledRequest) {
+    app.octokit.log.warn(
+      "[@octokit/oauth-app] `onUnhandledRequest` is deprecated and will be removed from the next major version."
+    );
+  }
+  onUnhandledRequest ??= onUnhandledRequestDefaultAWSAPIGatewayV2;
   return async function (event: APIGatewayProxyEventV2) {
     const request = parseRequest(event);
     const response = await handleRequest(app, { pathPrefix }, request);
-    return response ? sendResponse(response) : onUnhandledRequest(event);
+    return response ? sendResponse(response) : onUnhandledRequest!(event);
   };
 }
diff --git a/src/middleware/node/index.ts b/src/middleware/node/index.ts
index 9bc05f7..d152ede 100644
--- a/src/middleware/node/index.ts
+++ b/src/middleware/node/index.ts
@@ -25,7 +25,7 @@ export function createNodeMiddleware(
   app: OAuthApp<Options<ClientType>>,
   {
     pathPrefix,
-    onUnhandledRequest = onUnhandledRequestDefaultNode,
+    onUnhandledRequest,
   }: HandlerOptions & {
     onUnhandledRequest?: (
       request: IncomingMessage,
@@ -33,6 +33,12 @@ export function createNodeMiddleware(
     ) => void;
   } = {}
 ) {
+  if (onUnhandledRequest) {
+    app.octokit.log.warn(
+      "[@octokit/oauth-app] `onUnhandledRequest` is deprecated and will be removed from the next major version."
+    );
+  }
+  onUnhandledRequest ??= onUnhandledRequestDefaultNode;
   return async function (
     request: IncomingMessage,
     response: ServerResponse,
@@ -50,7 +56,7 @@ export function createNodeMiddleware(
     } else if (typeof next === "function") {
       next();
     } else {
-      onUnhandledRequest(request, response);
+      onUnhandledRequest!(request, response);
     }
   };
 }
diff --git a/src/middleware/web-worker/index.ts b/src/middleware/web-worker/index.ts
index 6851133..8e91975 100644
--- a/src/middleware/web-worker/index.ts
+++ b/src/middleware/web-worker/index.ts
@@ -18,11 +18,17 @@ export function createWebWorkerHandler<T extends Options<ClientType>>(
   app: OAuthApp<T>,
   {
     pathPrefix,
-    onUnhandledRequest = onUnhandledRequestDefaultWebWorker,
+    onUnhandledRequest,
   }: HandlerOptions & {
     onUnhandledRequest?: (request: Request) => Response | Promise<Response>;
   } = {}
 ) {
+  if (onUnhandledRequest) {
+    app.octokit.log.warn(
+      "[@octokit/oauth-app] `onUnhandledRequest` is deprecated and will be removed from the next major version."
+    );
+  }
+  onUnhandledRequest ??= onUnhandledRequestDefaultWebWorker;
   return async function (request: Request): Promise<Response> {
     const octokitRequest = parseRequest(request);
     const octokitResponse = await handleRequest(
@@ -32,7 +38,7 @@ export function createWebWorkerHandler<T extends Options<ClientType>>(
     );
     return octokitResponse
       ? sendResponse(octokitResponse)
-      : await onUnhandledRequest(request);
+      : await onUnhandledRequest!(request);
   };
 }
 


Test patch:
diff --git a/test/deprecations.test.ts b/test/deprecations.test.ts
index 7d9c9e8..6b52189 100644
--- a/test/deprecations.test.ts
+++ b/test/deprecations.test.ts
@@ -1,7 +1,13 @@
 import { URL } from "url";
 import * as nodeFetch from "node-fetch";
 import fromEntries from "fromentries";
-import { createCloudflareHandler, OAuthApp } from "../src";
+import {
+  createAWSLambdaAPIGatewayV2Handler,
+  createCloudflareHandler,
+  createNodeMiddleware,
+  createWebWorkerHandler,
+  OAuthApp,
+} from "../src";
 import { Octokit } from "@octokit/core";
 
 describe("deprecations", () => {
@@ -52,4 +58,37 @@ describe("deprecations", () => {
     expect(url.searchParams.get("state")).toMatch(/^\w+$/);
     expect(url.searchParams.get("scope")).toEqual(null);
   });
+
+  it("`onUnhandledRequest` is deprecated and will be removed from the next major version", async () => {
+    const warn = jest.fn().mockResolvedValue(undefined);
+    const handleRequest = createAWSLambdaAPIGatewayV2Handler(
+      new OAuthApp({
+        clientType: "github-app",
+        clientId: "client_id_123",
+        clientSecret: "client_secret_456",
+        Octokit: Octokit.defaults({
+          log: {
+            debug: () => undefined,
+            info: () => undefined,
+            warn,
+            error: () => undefined,
+          },
+        }),
+      }),
+      {
+        onUnhandledRequest: async (request) => {
+          return {
+            statusCode: 404,
+            headers: {},
+            body: "",
+          };
+        },
+      }
+    );
+
+    expect(warn.mock.calls.length).toEqual(1);
+    expect(warn.mock.calls[0][0]).toEqual(
+      "[@octokit/oauth-app] `onUnhandledRequest` is deprecated and will be removed from the next major version."
+    );
+  });
 });
diff --git a/test/smoke.test.ts b/test/smoke.test.ts
index 2cb6c4e..4d2264c 100644
--- a/test/smoke.test.ts
+++ b/test/smoke.test.ts
@@ -1,4 +1,4 @@
-import { OAuthApp } from "../src";
+import { handleRequest, OAuthApp } from "../src";
 
 describe("Smoke test", () => {
   it("OAuthApp is a function", () => {
@@ -12,4 +12,8 @@ describe("Smoke test", () => {
   it("OAuthApp.VERSION is set", () => {
     expect(OAuthApp.VERSION).toEqual("0.0.0-development");
   });
+
+  it("handleRequest is a function", () => {
+    expect(typeof handleRequest).toEqual("function");
+  });
 });