Fix merging options and beforeError hook logic

source/as-promise/core.ts

@@ -143,17 +143,8 @@ export default class PromisableRequest extends Request {
 			error = new RequestError(error.message, error, this);
 		}
 
-		try {
-			for (const hook of this.options.hooks.beforeError) {
-				// eslint-disable-next-line no-await-in-loop
-				error = await hook(error as RequestError);
-			}
-		} catch (error_) {
-			this.destroy(new RequestError(error_.message, error_, this));
-			return;
-		}
-
 		// Let the promise decide whether to abort or not
+		// It is also responsible for the `beforeError` hook
 		this.emit('error', error);
 	}
 }

source/as-promise/index.ts

@@ -27,7 +27,7 @@ export default function asPromise<T>(options: NormalizedOptions): CancelableRequ
 	let globalResponse: Response;
 	const emitter = new EventEmitter();
 
-	const promise = new PCancelable<T>((resolve, reject, onCancel) => {
+	const promise = new PCancelable<T>((resolve, _reject, onCancel) => {
 		const makeRequest = (): void => {
 			// Support retries
 			// `options.throwHttpErrors` needs to be always true,
@@ -38,10 +38,25 @@ export default function asPromise<T>(options: NormalizedOptions): CancelableRequ
 				options.throwHttpErrors = true;
 			}
 
+			// Note from @szmarczak: I think we should use `request.options` instead of the local options
 			const request = new PromisableRequest(options.url, options);
 			request._noPipe = true;
 			onCancel(() => request.destroy());
 
+			const reject = async (error: RequestError) => {
+				try {
+					for (const hook of options.hooks.beforeError) {
+						// eslint-disable-next-line no-await-in-loop
+						error = await hook(error);
+					}
+				} catch (error_) {
+					_reject(new RequestError(error_.message, error_, request));
+					return;
+				}
+
+				_reject(error);
+			};
+
 			globalRequest = request;
 
 			request.once('response', async (response: Response) => {

source/core/index.ts

@@ -364,6 +364,8 @@ export class RequestError extends Error {
 
 export class MaxRedirectsError extends RequestError {
 	declare readonly response: Response;
+	declare readonly request: Request;
+	declare readonly timings: Timings;
 
 	constructor(request: Request) {
 		super(`Redirected ${request.options.maxRedirects} times. Aborting.`, {}, request);
@@ -373,6 +375,8 @@ export class MaxRedirectsError extends RequestError {
 
 export class HTTPError extends RequestError {
 	declare readonly response: Response;
+	declare readonly request: Request;
+	declare readonly timings: Timings;
 
 	constructor(response: Response) {
 		super(`Response code ${response.statusCode} (${response.statusMessage!})`, {}, response.request);
@@ -381,20 +385,25 @@ export class HTTPError extends RequestError {
 }
 
 export class CacheError extends RequestError {
+	declare readonly request: Request;
+
 	constructor(error: Error, request: Request) {
 		super(error.message, error, request);
 		this.name = 'CacheError';
 	}
 }
 
 export class UploadError extends RequestError {
+	declare readonly request: Request;
+
 	constructor(error: Error, request: Request) {
 		super(error.message, error, request);
 		this.name = 'UploadError';
 	}
 }
 
 export class TimeoutError extends RequestError {
+	declare readonly request: Request;
 	readonly timings: Timings;
 	readonly event: string;
 
@@ -407,6 +416,10 @@ export class TimeoutError extends RequestError {
 }
 
 export class ReadError extends RequestError {
+	declare readonly request: Request;
+	declare readonly response: Response;
+	declare readonly timings: Timings;
+
 	constructor(error: Error, request: Request) {
 		super(error.message, error, request);
 		this.name = 'ReadError';
@@ -606,6 +619,8 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		// `options.headers`
 		if (is.undefined(options.headers)) {
 			options.headers = {};
+		} else if (options.headers === defaults?.headers) {
+			options.headers = {...options.headers};
 		} else {
 			options.headers = lowercaseKeys({...(defaults?.headers), ...options.headers});
 		}
@@ -702,8 +717,8 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 				getCookieString = promisify(getCookieString.bind(options.cookieJar));
 
 				options.cookieJar = {
-					setCookie: setCookie.bind(cookieJar),
-					getCookieString: getCookieString.bind(getCookieString)
+					setCookie,
+					getCookieString
 				};
 			}
 		}
@@ -750,7 +765,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		// `options.timeout`
 		if (is.number(options.timeout)) {
 			options.timeout = {request: options.timeout};
-		} else if (defaults) {
+		} else if (defaults && options.timeout !== defaults.timeout) {
 			options.timeout = {
 				...defaults.timeout,
 				...options.timeout
@@ -765,6 +780,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		}
 
 		// `options.hooks`
+		const areHooksUserDefined = options.hooks !== defaults?.hooks;
 		options.hooks = {...options.hooks};
 
 		for (const event of knownHookEvents) {
@@ -780,7 +796,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			}
 		}
 
-		if (defaults) {
+		if (defaults && areHooksUserDefined) {
 			for (const event of knownHookEvents) {
 				const defaultHooks = defaults.hooks[event];
 

test/error.ts

@@ -201,7 +201,7 @@ test('errors can have request property', withServer, async (t, server, got) => {
 	const error = await t.throwsAsync<HTTPError>(got(''));
 
 	t.truthy(error.response);
-	t.truthy(error.request!.downloadProgress);
+	t.truthy(error.request.downloadProgress);
 });
 
 // Fails randomly on Node 10:

test/hooks.ts

@@ -1,10 +1,11 @@
 import {URL} from 'url';
 import test from 'ava';
+import nock = require('nock');
 import getStream from 'get-stream';
 import delay = require('delay');
 import {Handler} from 'express';
 import Responselike = require('responselike');
-import got, {RequestError} from '../source';
+import got, {RequestError, HTTPError} from '../source';
 import withServer from './helpers/with-server';
 
 const errorString = 'oops';
@@ -829,3 +830,44 @@ test('beforeRequest hook is called before each request', withServer, async (t, s
 
 	t.is(counts, 2);
 });
+
+test('beforeError emits valid promise `HTTPError`s', async t => {
+	t.plan(3);
+
+	nock('https://ValidHTTPErrors.com').get('/').reply(() => [422, 'no']);
+
+	const instance = got.extend({
+		hooks: {
+			beforeError: [
+				error => {
+					t.true(error instanceof HTTPError);
+					t.truthy(error.response!.body);
+
+					return error;
+				}
+			]
+		},
+		retry: 0
+	});
+
+	await t.throwsAsync(instance('https://ValidHTTPErrors.com'));
+});
+
+test('hooks are not duplicated', withServer, async (t, _server, got) => {
+	let calls = 0;
+
+	await t.throwsAsync(got({
+		hooks: {
+			beforeError: [
+				error => {
+					calls++;
+
+					return error;
+				}
+			]
+		},
+		retry: 0
+	}), {message: 'Response code 404 (Not Found)'});
+
+	t.is(calls, 1);
+});