From 476752999f1371d645b96fa23b34f8fca2bc76ea Mon Sep 17 00:00:00 2001 From: uzlopak Date: Thu, 11 Jul 2024 22:38:25 +0200 Subject: [PATCH 01/10] fail early, fail fast --- src/fetch-wrapper.ts | 64 +++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 23576cdbb..dee293330 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -5,24 +5,6 @@ import type { EndpointInterface } from "@octokit/types"; export default function fetchWrapper( requestOptions: ReturnType, ) { - 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; - const fetch: typeof globalThis.fetch = requestOptions.request?.fetch || globalThis.fetch; @@ -32,17 +14,32 @@ export default function fetchWrapper( ); } + 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; + + const requestHeaders = Object.fromEntries( + Object.entries(requestOptions.headers).map(([name, value]) => [ + name, + String(value), + ]), + ); + + let responseHeaders: { [header: string]: string } = {}; + let status: number; + let url: string; + return fetch(requestOptions.url, { method: requestOptions.method, - body: requestOptions.body, + body, redirect: requestOptions.request?.redirect, // Header values must be `string` - headers: Object.fromEntries( - Object.entries(requestOptions.headers).map(([name, value]) => [ - name, - String(value), - ]), - ), + 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. @@ -53,17 +50,18 @@ export default function fetchWrapper( status = response.status; for (const keyAndValue of response.headers) { - headers[keyAndValue[0]] = keyAndValue[1]; + responseHeaders[keyAndValue[0]] = keyAndValue[1]; } - if ("deprecation" in headers) { + if ("deprecation" in responseHeaders) { const matches = - headers.link && headers.link.match(/<([^>]+)>; rel="deprecation"/); + 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 ${headers.sunset}${ + }" is deprecated. It is scheduled to be removed on ${responseHeaders.sunset}${ deprecationLink ? `. See ${deprecationLink}` : "" }`, ); @@ -83,7 +81,7 @@ export default function fetchWrapper( response: { url, status, - headers, + headers: responseHeaders, data: undefined, }, request: requestOptions, @@ -95,7 +93,7 @@ export default function fetchWrapper( response: { url, status, - headers, + headers: responseHeaders, data: await getResponseData(response), }, request: requestOptions, @@ -109,7 +107,7 @@ export default function fetchWrapper( response: { url, status, - headers, + headers: responseHeaders, data, }, request: requestOptions, @@ -126,7 +124,7 @@ export default function fetchWrapper( return { status, url, - headers, + headers: responseHeaders, data, }; }) From 3714accf865b8ddd11fec14a418ec6e7c56e65bf Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 04:07:42 +0200 Subject: [PATCH 02/10] improve --- src/fetch-wrapper.ts | 222 ++++++++++++++++++++++--------------------- test/request.test.ts | 23 ++--- 2 files changed, 125 insertions(+), 120 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index dee293330..313ebb947 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -1,10 +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, -) { +): Promise> { const fetch: typeof globalThis.fetch = requestOptions.request?.fetch || globalThis.fetch; @@ -30,109 +30,30 @@ export default function fetchWrapper( ]), ); - let responseHeaders: { [header: string]: string } = {}; - let status: number; - let url: string; - - return fetch(requestOptions.url, { - method: requestOptions.method, - body, - redirect: requestOptions.request?.redirect, - // Header values must be `string` - 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" }), - }) - .then(async (response) => { - url = response.url; - status = response.status; - - for (const keyAndValue of response.headers) { - responseHeaders[keyAndValue[0]] = keyAndValue[1]; - } - - 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; - } - - // 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: responseHeaders, - data: undefined, - }, - request: requestOptions, - }); - } - - if (status === 304) { - throw new RequestError("Not modified", status, { - response: { - url, - status, - headers: responseHeaders, - 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: responseHeaders, - data, - }, - request: requestOptions, - }); - + let fetchResponse: Response; + + try { + fetchResponse = await fetch(requestOptions.url, { + method: requestOptions.method, + body, + redirect: requestOptions.request?.redirect, + // Header values must be `string` + 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: responseHeaders, - 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` @@ -144,11 +65,95 @@ export default function fetchWrapper( message = error.cause; } } + } - throw new RequestError(message, 500, { - request: requestOptions, - }); + 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 keyAndValue of fetchResponse.headers) { + responseHeaders[keyAndValue[0]] = keyAndValue[1]; + } + + const octokitResponse: OctokitResponse = { + 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); + + const error = new RequestError( + toErrorMessage(octokitResponse.data), + status, + { + response: octokitResponse, + request: requestOptions, + }, + ); + + throw error; + } + + const responseBody = parseSuccessResponseBody + ? await getResponseData(fetchResponse) + : fetchResponse.body; + + return { + status, + url, + headers: responseHeaders, + data: responseBody, + }; } async function getResponseData(response: Response) { @@ -167,7 +172,10 @@ async function getResponseData(response: Response) { ); } - if (!contentType || /^text\/|charset=utf-8$/.test(contentType)) { + if ( + (!contentType || /^text\/|charset=utf-8$/.test(contentType)) && + response.text + ) { return response.text(); } diff --git a/test/request.test.ts b/test/request.test.ts index c36a52807..3a0aa37f2 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -977,28 +977,25 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== }); }; - const mock = (url: string) => { - expect(url).toEqual("https://api.github.com/"); - return delay().then(() => { - return { - status: 200, - headers: {}, - body: { - message: "OK", - }, - }; - }); - }; + const mock = fetchMock.sandbox().get("https://api.github.com/", () => + delay(3000).then(() => ({ + message: "Not Found", + documentation_url: + "https://docs.github.com/en/rest/reference/repos#get-a-repository", + })), + ); try { await request("GET /", { request: { fetch: mock, + signal: AbortSignal.timeout(1000), }, }); throw new Error("should not resolve"); } catch (error) { - expect(error.name).toEqual("HttpError"); + expect(error.name).toEqual("AbortError"); + expect(error.message).toEqual("The operation was aborted."); expect(error.status).toEqual(500); } }); From b5b21698657edee5c3f946d69f57e9de2fc25d1a Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 05:33:06 +0200 Subject: [PATCH 03/10] improve tests --- test/request-native-fetch.test.ts | 89 +++++++++++++++++++------------ test/request.test.ts | 39 +++++++++----- 2 files changed, 82 insertions(+), 46 deletions(-) diff --git a/test/request-native-fetch.test.ts b/test/request-native-fetch.test.ts index 26d46d025..1fb96dc4a 100644 --- a/test/request-native-fetch.test.ts +++ b/test/request-native-fetch.test.ts @@ -4,7 +4,6 @@ import { ReadableStream } from "node:stream/web"; import { describe, it, expect, vi } from "vitest"; import { getUserAgent } from "universal-user-agent"; -import fetchMock from "fetch-mock"; import { createAppAuth } from "@octokit/auth-app"; import type { EndpointOptions, RequestInterface } from "@octokit/types"; @@ -324,35 +323,53 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== ).rejects.toHaveProperty("status", 404); }); - it.skip("Binary response with redirect (🤔 unclear how to mock fetch redirect properly)", () => { - const mock = fetchMock - .sandbox() - .get( - "https://codeload.github.com/octokit-fixture-org/get-archive/legacy.tar.gz/master", - { - status: 200, - body: Buffer.from( - "1f8b0800000000000003cb4f2ec9cfce2cd14dcbac28292d4ad5cd2f4ad74d4f2dd14d2c4acec82c4bd53580007d060a0050bfb9b9a90203c428741ac2313436343307222320dbc010a8dc5c81c194124b8905a5c525894540a714e5e797e05347481edd734304e41319ff41ae8e2ebeae7ab92964d801d46f66668227fe0d4d51e3dfc8d0c8d808284f75df6201233cfe951590627ba01d330a46c1281805a3806e000024cb59d6000a0000", - "hex", - ), - headers: { - "content-type": "application/x-gzip", - "content-length": "172", - }, - }, - ); + it("Binary response with redirect", async () => { + expect.assertions(7); - return request("GET /repos/{owner}/{repo}/{archive_format}/{ref}", { - owner: "octokit-fixture-org", - repo: "get-archive", - archive_format: "tarball", - ref: "master", - request: { - fetch: mock, - }, - }).then((response) => { - expect(response.data.length).toEqual(172); + const payload = + "1f8b0800000000000003cb4f2ec9cfce2cd14dcbac28292d4ad5cd2f4ad74d4f2dd14d2c4acec82c4bd53580007d060a0050bfb9b9a90203c428741ac2313436343307222320dbc010a8dc5c81c194124b8905a5c525894540a714e5e797e05347481edd734304e41319ff41ae8e2ebeae7ab92964d801d46f66668227fe0d4d51e3dfc8d0c8d808284f75df6201233cfe951590627ba01d330a46c1281805a3806e000024cb59d6000a0000"; + + const request = await mockRequestHttpServer((req, res) => { + expect(req.method).toBe("GET"); + + switch (req.url) { + case "/octokit-fixture-org/get-archive-1/legacy.tar.gz/master": + { + res.writeHead(301, { + Location: + "/octokit-fixture-org/get-archive-2/legacy.tar.gz/master", + }); + res.end(); + } + break; + case "/octokit-fixture-org/get-archive-2/legacy.tar.gz/master": + { + res.writeHead(200, { + "content-type": "application/x-gzip", + "content-length": "172", + }); + res.end(Buffer.from(payload, "hex")); + } + break; + default: { + res.writeHead(500); + } + } + + res.end(); }); + + const response = await request( + `GET ${request.baseUrlMockServer}/octokit-fixture-org/get-archive-1/legacy.tar.gz/master`, + ); + + expect(response.headers["content-type"]).toEqual("application/x-gzip"); + expect(response.headers["content-length"]).toEqual("172"); + expect(response.status).toEqual(200); + expect(response.data).toBeInstanceOf(ArrayBuffer); + expect(zlib.gunzipSync(Buffer.from(payload, "hex")).buffer).toEqual( + response.data, + ); }); it("Binary response", async () => { @@ -921,14 +938,16 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== expect(response.data).toEqual({ id: 123 }); }); - it("404 not found", { skip: true }, async () => { - expect.assertions(3); + it("404 not found", async () => { + expect.assertions(5); const request = await mockRequestHttpServer(async (req, res) => { expect(req.method).toBe("GET"); expect(req.url).toBe("/repos/octocat/unknown"); - res.writeHead(404); + res.writeHead(404, { + "content-type": "application/json", + }); res.end( JSON.stringify({ message: "Not Found", @@ -950,7 +969,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } }); - it("Request timeout", { skip: true }, async () => { + it("Request timeout", async () => { expect.assertions(4); const request = await mockRequestHttpServer(async (req, res) => { @@ -967,7 +986,11 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== }); try { - await request("GET /"); + await request("GET /", { + request: { + signal: AbortSignal.timeout(500), + }, + }); throw new Error("should not resolve"); } catch (error) { expect(error.name).toEqual("HttpError"); diff --git a/test/request.test.ts b/test/request.test.ts index 3a0aa37f2..016e10fc3 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -260,11 +260,21 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== ).rejects.toHaveProperty("status", 404); }); - it.skip("Binary response with redirect (🤔 unclear how to mock fetch redirect properly)", () => { + it.skip("Binary response with redirect (🤔 unclear how to mock fetch redirect properly)", async () => { const mock = fetchMock .sandbox() .get( - "https://codeload.github.com/octokit-fixture-org/get-archive/legacy.tar.gz/master", + "https://codeload.github.com/repos/octokit-fixture-org/get-archive-1/tarball/master", + { + status: 301, + headers: { + location: + "https://codeload.github.com/repos/octokit-fixture-org/get-archive-2/tarball/master", + }, + }, + ) + .get( + "https://codeload.github.com/repos/octokit-fixture-org/get-archive-2/tarball/master", { status: 200, body: Buffer.from( @@ -278,17 +288,20 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== }, ); - return request("GET /repos/{owner}/{repo}/{archive_format}/{ref}", { - owner: "octokit-fixture-org", - repo: "get-archive", - archive_format: "tarball", - ref: "master", - request: { - fetch: mock, + const response = await request( + "GET https://codeload.github.com/repos/{owner}/{repo}/{archive_format}/{ref}", + { + owner: "octokit-fixture-org", + repo: "get-archive-1", + archive_format: "tarball", + ref: "master", + request: { + fetch: mock, + }, }, - }).then((response) => { - expect(response.data.length).toEqual(172); - }); + ); + expect(response.status).toEqual(200); + expect(response.data.length).toEqual(172); }); it("Binary response", async () => { @@ -989,7 +1002,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== await request("GET /", { request: { fetch: mock, - signal: AbortSignal.timeout(1000), + signal: AbortSignal.timeout(500), }, }); throw new Error("should not resolve"); From aa37e27dc4a18127af7de885f9101aff96cab2e0 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 06:32:25 +0200 Subject: [PATCH 04/10] AbortSignal should result in a status code 0 --- src/fetch-wrapper.ts | 2 +- test/request.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 313ebb947..6c621c0f0 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -49,7 +49,7 @@ export default async function fetchWrapper( let message = "Unknown Error"; if (error instanceof Error) { if (error.name === "AbortError") { - (error as RequestError).status = 500; + (error as RequestError).status = 0; throw error; } diff --git a/test/request.test.ts b/test/request.test.ts index 016e10fc3..99d3f2a5d 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -1009,7 +1009,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } catch (error) { expect(error.name).toEqual("AbortError"); expect(error.message).toEqual("The operation was aborted."); - expect(error.status).toEqual(500); + expect(error.status).toEqual(0); } }); From e0c10ff0fdfaa2036f826051d71cdf93eae0d02c Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 07:08:52 +0200 Subject: [PATCH 05/10] minor change --- src/fetch-wrapper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 6c621c0f0..326512cd9 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -79,8 +79,8 @@ export default async function fetchWrapper( const url = fetchResponse.url; const responseHeaders: { [header: string]: string } = {}; - for (const keyAndValue of fetchResponse.headers) { - responseHeaders[keyAndValue[0]] = keyAndValue[1]; + for (const [key, value] of fetchResponse.headers) { + responseHeaders[key] = value; } const octokitResponse: OctokitResponse = { From 611d4c0a60a02b936f928f83ee3154763804f3d9 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 07:35:42 +0200 Subject: [PATCH 06/10] streamline further --- src/fetch-wrapper.ts | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 326512cd9..3f8a7f386 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -132,31 +132,20 @@ export default async function fetchWrapper( if (status >= 400) { octokitResponse.data = await getResponseData(fetchResponse); - const error = new RequestError( - toErrorMessage(octokitResponse.data), - status, - { - response: octokitResponse, - request: requestOptions, - }, - ); - - throw error; + throw new RequestError(toErrorMessage(octokitResponse.data), status, { + response: octokitResponse, + request: requestOptions, + }); } - const responseBody = parseSuccessResponseBody + octokitResponse.data = parseSuccessResponseBody ? await getResponseData(fetchResponse) : fetchResponse.body; - return { - status, - url, - headers: responseHeaders, - data: responseBody, - }; + return octokitResponse; } -async function getResponseData(response: Response) { +function getResponseData(response: Response) { const contentType = response.headers.get("content-type"); if (/application\/json/.test(contentType!)) { return ( From 75ae333788a6aaccc3d0862936d29f68e75f2c90 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 07:38:03 +0200 Subject: [PATCH 07/10] remove case --- src/fetch-wrapper.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 3f8a7f386..04c2eb267 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -161,10 +161,7 @@ function getResponseData(response: Response) { ); } - if ( - (!contentType || /^text\/|charset=utf-8$/.test(contentType)) && - response.text - ) { + if (!contentType || /^text\/|charset=utf-8$/.test(contentType)) { return response.text(); } From 7bc70aa5ed12a0d1e463d858177cda2faac6b28a Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 09:57:30 +0200 Subject: [PATCH 08/10] status 500 for abort error --- src/fetch-wrapper.ts | 2 +- test/request.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 04c2eb267..2a136cc0e 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -49,7 +49,7 @@ export default async function fetchWrapper( let message = "Unknown Error"; if (error instanceof Error) { if (error.name === "AbortError") { - (error as RequestError).status = 0; + (error as RequestError).status = 500; throw error; } diff --git a/test/request.test.ts b/test/request.test.ts index 99d3f2a5d..016e10fc3 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -1009,7 +1009,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } catch (error) { expect(error.name).toEqual("AbortError"); expect(error.message).toEqual("The operation was aborted."); - expect(error.status).toEqual(0); + expect(error.status).toEqual(500); } }); From a8fc1bf7c07212c9141a7f03b012cd82f53a6054 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 10:29:37 +0200 Subject: [PATCH 09/10] add Dispatcher testcase for timeout --- package-lock.json | 10 +++++++ package.json | 1 + test/mock-request-http-server.ts | 4 +++ test/request-native-fetch.test.ts | 47 +++++++++++++++++++++++++++++-- test/request.test.ts | 2 +- 5 files changed, 61 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9233754eb..880105add 100644 --- a/package-lock.json +++ b/package-lock.json @@ -25,6 +25,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" }, "engines": { @@ -2570,6 +2571,15 @@ "node": ">=14.17" } }, + "node_modules/undici": { + "version": "6.19.2", + "resolved": "https://registry.npmjs.org/undici/-/undici-6.19.2.tgz", + "integrity": "sha512-JfjKqIauur3Q6biAtHJ564e3bWa8VvT+7cSiOJHFbX4Erv6CLGDpg8z+Fmg/1OI/47RA+GI2QZaF48SSaLvyBA==", + "dev": true, + "engines": { + "node": ">=18.17" + } + }, "node_modules/undici-types": { "version": "5.25.3", "dev": true, diff --git a/package.json b/package.json index 3f895d4ef..bdedb73fe 100644 --- 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/test/mock-request-http-server.ts b/test/mock-request-http-server.ts index 3e5f68e99..eb9231e64 100644 --- 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 & { closeMockServer: () => void; @@ -25,6 +26,9 @@ export default async function mockRequestHttpServer( const request = withDefaults(endpoint, { ...defaults, baseUrl, + request: { + fetch, + }, }) as RequestInterface & { closeMockServer: () => void; baseUrlMockServer: string; diff --git a/test/request-native-fetch.test.ts b/test/request-native-fetch.test.ts index 1fb96dc4a..0017337ed 100644 --- a/test/request-native-fetch.test.ts +++ b/test/request-native-fetch.test.ts @@ -6,10 +6,10 @@ import { describe, it, expect, vi } from "vitest"; import { getUserAgent } from "universal-user-agent"; import { createAppAuth } from "@octokit/auth-app"; import type { EndpointOptions, RequestInterface } from "@octokit/types"; +import { fetch as undiciFetch, Agent, RequestInit } from "undici"; import bodyParser from "./body-parser.ts"; import mockRequestHttpServer from "./mock-request-http-server.ts"; -import { request } from "../src/index.ts"; const userAgent = `octokit-request.js/0.0.0-development ${getUserAgent()}`; const __filename = new URL(import.meta.url); @@ -724,6 +724,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== method: "GET", request: { hook, + fetch: globalThis.fetch, }, url: "/", }); @@ -969,7 +970,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } }); - it("Request timeout", async () => { + it("Request timeout via an AbortSignal", async () => { expect.assertions(4); const request = await mockRequestHttpServer(async (req, res) => { @@ -998,6 +999,48 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } }); + it("Request timeout via a Dispatcher", async () => { + expect.assertions(5); + + const fetch = (url: string, options: RequestInit) => { + return undiciFetch(url, { + ...options, + dispatcher: new Agent({ + bodyTimeout: 500, + headersTimeout: 500, + keepAliveTimeout: 500, + keepAliveMaxTimeout: 500, + }), + }); + }; + + const request = await mockRequestHttpServer(async (req, res) => { + expect(req.method).toBe("GET"); + expect(req.url).toBe("/"); + + await new Promise((resolve) => setTimeout(resolve, 3000)); + res.writeHead(200); + res.end( + JSON.stringify({ + message: "OK", + }), + ); + }); + + try { + await request("GET /", { + request: { + fetch, + }, + }); + throw new Error("should not resolve"); + } catch (error) { + expect(error.message).not.toEqual("should not resolve"); + expect(error.name).toEqual("HttpError"); + expect(error.status).toEqual(500); + } + }); + it("validate request with readstream data", async () => { expect.assertions(6); diff --git a/test/request.test.ts b/test/request.test.ts index 016e10fc3..464f7eb95 100644 --- a/test/request.test.ts +++ b/test/request.test.ts @@ -981,7 +981,7 @@ x//0u+zd/R/QRUzLOw4N72/Hu+UG6MNt5iDZFCtapRaKt6OvSBwy8w== } }); - it("Request timeout", async () => { + it("Request timeout via an AbortSignal", async () => { expect.assertions(3); const delay = (millis = 3000) => { From a94e966131796e6e5b8c4031584fad31559b2a50 Mon Sep 17 00:00:00 2001 From: uzlopak Date: Fri, 12 Jul 2024 20:56:52 +0200 Subject: [PATCH 10/10] adapt requested changes --- src/fetch-wrapper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fetch-wrapper.ts b/src/fetch-wrapper.ts index 2a136cc0e..3db220bf5 100644 --- a/src/fetch-wrapper.ts +++ b/src/fetch-wrapper.ts @@ -23,6 +23,7 @@ export default async function fetchWrapper( ? JSON.stringify(requestOptions.body) : requestOptions.body; + // Header values must be `string` const requestHeaders = Object.fromEntries( Object.entries(requestOptions.headers).map(([name, value]) => [ name, @@ -37,7 +38,6 @@ export default async function fetchWrapper( method: requestOptions.method, body, redirect: requestOptions.request?.redirect, - // Header values must be `string` headers: requestHeaders, signal: requestOptions.request?.signal, // duplex must be set if request.body is ReadableStream or Async Iterables. @@ -145,7 +145,7 @@ export default async function fetchWrapper( return octokitResponse; } -function getResponseData(response: Response) { +async function getResponseData(response: Response): Promise { const contentType = response.headers.get("content-type"); if (/application\/json/.test(contentType!)) { return (