fix(refactor): async await instead of Promise chain (#711)

* fail early, fail fast * improve * improve tests * AbortSignal should result in a status code 0 * minor change * streamline further * remove case * status 500 for abort error * add Dispatcher testcase for timeout * adapt requested changes
octokit · Jul 13, 2024 · 611b275 · 611b275
1 parent bc059c8
commit 611b275
Show file tree

Hide file tree

Showing 6 changed files with 267 additions and 184 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -41,6 +41,7 @@
     "prettier": "3.3.2",
     "semantic-release-plugin-update-version-in-files": "^1.0.0",
     "typescript": "^5.0.0",
+    "undici": "^6.19.2",
     "vitest": "^2.0.0"
   },
   "release": {

diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts
@@ -1,28 +1,10 @@
 import { isPlainObject } from "./is-plain-object.js";
 import { RequestError } from "@octokit/request-error";
-import type { EndpointInterface } from "@octokit/types";
+import type { EndpointInterface, OctokitResponse } from "@octokit/types";
 
-export default function fetchWrapper(
+export default async function fetchWrapper(
   requestOptions: ReturnType<EndpointInterface>,
-) {
-  const log =
-    requestOptions.request && requestOptions.request.log
-      ? requestOptions.request.log
-      : console;
-  const parseSuccessResponseBody =
-    requestOptions.request?.parseSuccessResponseBody !== false;
-
-  if (
-    isPlainObject(requestOptions.body) ||
-    Array.isArray(requestOptions.body)
-  ) {
-    requestOptions.body = JSON.stringify(requestOptions.body);
-  }
-
-  let headers: { [header: string]: string } = {};
-  let status: number;
-  let url: string;
-
+): Promise<OctokitResponse<any>> {
   const fetch: typeof globalThis.fetch =
     requestOptions.request?.fetch || globalThis.fetch;
 
@@ -32,109 +14,46 @@ export default function fetchWrapper(
     );
   }
 
-  return fetch(requestOptions.url, {
-    method: requestOptions.method,
-    body: requestOptions.body,
-    redirect: requestOptions.request?.redirect,
-    // Header values must be `string`
-    headers: Object.fromEntries(
-      Object.entries(requestOptions.headers).map(([name, value]) => [
-        name,
-        String(value),
-      ]),
-    ),
-    signal: requestOptions.request?.signal,
-    // duplex must be set if request.body is ReadableStream or Async Iterables.
-    // See https://fetch.spec.whatwg.org/#dom-requestinit-duplex.
-    ...(requestOptions.body && { duplex: "half" }),
-  })
-    .then(async (response) => {
-      url = response.url;
-      status = response.status;
-
-      for (const keyAndValue of response.headers) {
-        headers[keyAndValue[0]] = keyAndValue[1];
-      }
-
-      if ("deprecation" in headers) {
-        const matches =
-          headers.link && headers.link.match(/<([^>]+)>; rel="deprecation"/);
-        const deprecationLink = matches && matches.pop();
-        log.warn(
-          `[@octokit/request] "${requestOptions.method} ${
-            requestOptions.url
-          }" is deprecated. It is scheduled to be removed on ${headers.sunset}${
-            deprecationLink ? `. See ${deprecationLink}` : ""
-          }`,
-        );
-      }
-
-      if (status === 204 || status === 205) {
-        return;
-      }
-
-      // GitHub API returns 200 for HEAD requests
-      if (requestOptions.method === "HEAD") {
-        if (status < 400) {
-          return;
-        }
-
-        throw new RequestError(response.statusText, status, {
-          response: {
-            url,
-            status,
-            headers,
-            data: undefined,
-          },
-          request: requestOptions,
-        });
-      }
-
-      if (status === 304) {
-        throw new RequestError("Not modified", status, {
-          response: {
-            url,
-            status,
-            headers,
-            data: await getResponseData(response),
-          },
-          request: requestOptions,
-        });
-      }
-
-      if (status >= 400) {
-        const data = await getResponseData(response);
-
-        const error = new RequestError(toErrorMessage(data), status, {
-          response: {
-            url,
-            status,
-            headers,
-            data,
-          },
-          request: requestOptions,
-        });
+  const log = requestOptions.request?.log || console;
+  const parseSuccessResponseBody =
+    requestOptions.request?.parseSuccessResponseBody !== false;
 
+  const body =
+    isPlainObject(requestOptions.body) || Array.isArray(requestOptions.body)
+      ? JSON.stringify(requestOptions.body)
+      : requestOptions.body;
+
+  // Header values must be `string`
+  const requestHeaders = Object.fromEntries(
+    Object.entries(requestOptions.headers).map(([name, value]) => [
+      name,
+      String(value),
+    ]),
+  );
+
+  let fetchResponse: Response;
+
+  try {
+    fetchResponse = await fetch(requestOptions.url, {
+      method: requestOptions.method,
+      body,
+      redirect: requestOptions.request?.redirect,
+      headers: requestHeaders,
+      signal: requestOptions.request?.signal,
+      // duplex must be set if request.body is ReadableStream or Async Iterables.
+      // See https://fetch.spec.whatwg.org/#dom-requestinit-duplex.
+      ...(requestOptions.body && { duplex: "half" }),
+    });
+    // wrap fetch errors as RequestError if it is not a AbortError
+  } catch (error) {
+    let message = "Unknown Error";
+    if (error instanceof Error) {
+      if (error.name === "AbortError") {
+        (error as RequestError).status = 500;
         throw error;
       }
 
-      return parseSuccessResponseBody
-        ? await getResponseData(response)
-        : response.body;
-    })
-    .then((data) => {
-      return {
-        status,
-        url,
-        headers,
-        data,
-      };
-    })
-    .catch((error) => {
-      if (error instanceof RequestError) throw error;
-      else if (error.name === "AbortError") throw error;
-
-      let message = error.message;
+      message = error.message;
 
       // undici throws a TypeError for network errors
       // and puts the error message in `error.cause`
@@ -146,14 +65,87 @@ export default function fetchWrapper(
           message = error.cause;
         }
       }
+    }
+
+    const requestError = new RequestError(message, 500, {
+      request: requestOptions,
+    });
+    requestError.cause = error;
+
+    throw requestError;
+  }
+
+  const status = fetchResponse.status;
+  const url = fetchResponse.url;
+  const responseHeaders: { [header: string]: string } = {};
+
+  for (const [key, value] of fetchResponse.headers) {
+    responseHeaders[key] = value;
+  }
+
+  const octokitResponse: OctokitResponse<any> = {
+    url,
+    status,
+    headers: responseHeaders,
+    data: "",
+  };
+
+  if ("deprecation" in responseHeaders) {
+    const matches =
+      responseHeaders.link &&
+      responseHeaders.link.match(/<([^>]+)>; rel="deprecation"/);
+    const deprecationLink = matches && matches.pop();
+    log.warn(
+      `[@octokit/request] "${requestOptions.method} ${
+        requestOptions.url
+      }" is deprecated. It is scheduled to be removed on ${responseHeaders.sunset}${
+        deprecationLink ? `. See ${deprecationLink}` : ""
+      }`,
+    );
+  }
+
+  if (status === 204 || status === 205) {
+    return octokitResponse;
+  }
+
+  // GitHub API returns 200 for HEAD requests
+  if (requestOptions.method === "HEAD") {
+    if (status < 400) {
+      return octokitResponse;
+    }
+
+    throw new RequestError(fetchResponse.statusText, status, {
+      response: octokitResponse,
+      request: requestOptions,
+    });
+  }
+
+  if (status === 304) {
+    octokitResponse.data = await getResponseData(fetchResponse);
+
+    throw new RequestError("Not modified", status, {
+      response: octokitResponse,
+      request: requestOptions,
+    });
+  }
+
+  if (status >= 400) {
+    octokitResponse.data = await getResponseData(fetchResponse);
 
-      throw new RequestError(message, 500, {
-        request: requestOptions,
-      });
+    throw new RequestError(toErrorMessage(octokitResponse.data), status, {
+      response: octokitResponse,
+      request: requestOptions,
     });
+  }
+
+  octokitResponse.data = parseSuccessResponseBody
+    ? await getResponseData(fetchResponse)
+    : fetchResponse.body;
+
+  return octokitResponse;
 }
 
-async function getResponseData(response: Response) {
+async function getResponseData(response: Response): Promise<any> {
   const contentType = response.headers.get("content-type");
   if (/application\/json/.test(contentType!)) {
     return (

diff --git a/test/mock-request-http-server.ts b/test/mock-request-http-server.ts
@@ -10,6 +10,7 @@ import defaults from "../src/defaults.ts";
 
 export default async function mockRequestHttpServer(
   requestListener: RequestListener,
+  fetch = globalThis.fetch,
 ): Promise<
   RequestInterface<object> & {
     closeMockServer: () => void;
@@ -25,6 +26,9 @@ export default async function mockRequestHttpServer(
   const request = withDefaults(endpoint, {
     ...defaults,
     baseUrl,
+    request: {
+      fetch,
+    },
   }) as RequestInterface<object> & {
     closeMockServer: () => void;
     baseUrlMockServer: string;