Fix beforeRequest hooks being skipped when a Request is returned (#832)

Author: sindresorhus

readme.md

@@ -351,7 +351,7 @@ This hook enables you to modify the request right before it is sent. Ky will mak
 
 The `retryCount` is `0` for the initial request and increments with each retry. This allows you to distinguish between initial requests and retries, which is useful when you need different behavior for retries (e.g., avoiding overwriting headers set in `beforeRetry`).
 
-The hook can return a [`Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) to replace the outgoing request, or return a [`Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response) to completely avoid making an HTTP request. This can be used to mock a request, check an internal cache, etc. An **important** consideration when returning a request or response from this hook is that any remaining `beforeRequest` hooks will be skipped, so you may want to only return them from the last hook.
+The hook can return a [`Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) to replace the outgoing request (remaining hooks will still run with the updated request). It can also return a [`Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response) to completely avoid making an HTTP request, in which case remaining `beforeRequest` hooks are skipped. This can be used to mock a request, check an internal cache, etc.
 
 ```js
 import ky from 'ky';

source/core/Ky.ts

@@ -336,8 +336,6 @@ export class Ky {
 			if (!supportsRequestStreams) {
 				throw new Error('Request streams are not supported in your environment. The `duplex` option for `Request` is not available.');
 			}
-
-			this.request = this.#wrapRequestWithUploadProgress(this.request, this.#options.body ?? undefined);
 		}
 	}
 
@@ -685,30 +683,31 @@ export class Ky {
 
 			if (result instanceof globalThis.Request) {
 				this.#assignRequest(result);
-				break;
 			}
 		}
 
 		const nonRequestOptions = findUnknownOptions(this.request, this.#options);
+		const retryRequest = this.#options.retry.limit > 0 ? this.request.clone() : undefined;
+		const request = this.#wrapRequestWithUploadProgress(this.request, this.#options.body ?? undefined);
 
 		// Cloning is done here to prepare in advance for retries.
 		// Skip cloning when retries are disabled — cloning a streaming body calls ReadableStream#tee()
 		// which buffers the entire stream in memory, causing excessive memory usage for large uploads.
-		this.#originalRequest = this.request;
-		if (this.#options.retry.limit > 0) {
-			this.request = this.#originalRequest.clone();
+		this.#originalRequest = request;
+		if (retryRequest) {
+			this.request = retryRequest;
 		}
 
 		if (this.#options.timeout === false) {
-			return this.#options.fetch(this.#originalRequest, nonRequestOptions);
+			return this.#options.fetch(request, nonRequestOptions);
 		}
 
 		const remainingTimeout = this.#getRemainingTimeout() ?? this.#options.timeout;
 		if (remainingTimeout <= 0) {
 			throw new TimeoutError(this.request);
 		}
 
-		return timeout(this.#originalRequest, nonRequestOptions, this.#abortController, {
+		return timeout(request, nonRequestOptions, this.#abortController, {
 			...this.#options,
 			timeout: remainingTimeout,
 		} as TimeoutOptions);
@@ -754,7 +753,7 @@ export class Ky {
 
 	#assignRequest(request: Request): void {
 		this.#cachedNormalizedOptions = undefined;
-		this.request = this.#wrapRequestWithUploadProgress(request);
+		this.request = request;
 	}
 
 	#wrapRequestWithUploadProgress(request: Request, originalBody?: BodyInit): Request {

source/types/hooks.ts

@@ -65,7 +65,7 @@ export type Hooks = {
 
 	The `retryCount` is `0` for the initial request and increments with each retry. This allows you to distinguish between initial requests and retries, which is useful when you need different behavior for retries (e.g., avoiding overwriting headers set in `beforeRetry`).
 
-	A [`Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response) can be returned from this hook to completely avoid making an HTTP request. This can be used to mock a request, check an internal cache, etc. An **important** consideration when returning a `Response` from this hook is that all the following hooks will be skipped, so **ensure you only return a `Response` from the last hook**.
+	A [`Request`](https://developer.mozilla.org/en-US/docs/Web/API/Request) can be returned from this hook to replace the outgoing request; remaining hooks will still run with the updated request. A [`Response`](https://developer.mozilla.org/en-US/docs/Web/API/Response) can be returned to completely avoid making an HTTP request, in which case remaining `beforeRequest` hooks are skipped. This can be used to mock a request, check an internal cache, etc.
 
 	@example
 	```

test/hooks.ts

@@ -11,6 +11,12 @@ import ky, {
 import {type Options} from '../source/types/options.js';
 import {createHttpTestServer} from './helpers/create-http-test-server.js';
 
+const withHeader = (request: Request, name: string, value: string) => {
+	const headers = new Headers(request.headers);
+	headers.set(name, value);
+	return new Request(request, {headers});
+};
+
 const createStreamBody = (text: string) => new ReadableStream<Uint8Array>({
 	start(controller) {
 		controller.enqueue(new TextEncoder().encode(text));
@@ -1143,6 +1149,76 @@ test('beforeError runs when beforeRetry rethrows network errors', async t => {
 	t.true(thrownError instanceof TypeError);
 });
 
+test('hooks beforeRequest returning Request continues running remaining hooks', async t => {
+	let capturedRequest: Request | undefined;
+
+	await ky.get('https://example.com', {
+		async fetch(request) {
+			capturedRequest = request as Request;
+			return new Response('ok');
+		},
+		hooks: {
+			beforeRequest: [
+				({request}) => withHeader(request, 'x-hook-1', 'hook-1'),
+				({request}) => {
+					// Verify hook 2 receives the updated request produced by hook 1
+					t.is(request.headers.get('x-hook-1'), 'hook-1');
+					return withHeader(request, 'x-hook-2', 'hook-2');
+				},
+			],
+		},
+	});
+
+	t.truthy(capturedRequest);
+	t.is(capturedRequest!.headers.get('x-hook-1'), 'hook-1');
+	t.is(capturedRequest!.headers.get('x-hook-2'), 'hook-2');
+});
+
+test('hooks beforeRequest returning Request then non-returning hook both run', async t => {
+	let capturedRequest: Request | undefined;
+	let hook2Ran = false;
+
+	await ky.get('https://example.com', {
+		async fetch(request) {
+			capturedRequest = request as Request;
+			return new Response('ok');
+		},
+		hooks: {
+			beforeRequest: [
+				({request}) => withHeader(request, 'x-hook-1', 'hook-1'),
+				() => {
+					hook2Ran = true;
+				},
+			],
+		},
+	});
+
+	t.truthy(capturedRequest);
+	t.true(hook2Ran);
+	t.is(capturedRequest!.headers.get('x-hook-1'), 'hook-1');
+});
+
+test('hooks beforeRequest returning Request then Response skips HTTP request', async t => {
+	const expectedResponse = 'intercepted';
+	let fetchCalled = false;
+
+	const response = await ky.get('https://example.com', {
+		async fetch() {
+			fetchCalled = true;
+			return new Response('should not reach');
+		},
+		hooks: {
+			beforeRequest: [
+				({request}) => withHeader(request, 'x-hook-1', 'hook-1'),
+				() => new Response(expectedResponse, {status: 200}),
+			],
+		},
+	}).text();
+
+	t.false(fetchCalled);
+	t.is(response, expectedResponse);
+});
+
 test('hooks beforeRequest returning Response skips HTTP Request', async t => {
 	const expectedResponse = 'empty hook';