From d914a7e7a0d97b03745b9b47623e4ac3532b4dae Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Mon, 20 Apr 2020 18:16:15 +0200 Subject: [PATCH] Fix merging options and `beforeError` hook logic --- source/as-promise/core.ts | 11 +--------- source/as-promise/index.ts | 17 ++++++++++++++- source/core/index.ts | 24 +++++++++++++++++---- test/error.ts | 2 +- test/hooks.ts | 44 +++++++++++++++++++++++++++++++++++++- 5 files changed, 81 insertions(+), 17 deletions(-) diff --git a/source/as-promise/core.ts b/source/as-promise/core.ts index 41baebebb..b62fbf938 100644 --- a/source/as-promise/core.ts +++ b/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); } } diff --git a/source/as-promise/index.ts b/source/as-promise/index.ts index 1a1a1424f..94f81666c 100644 --- a/source/as-promise/index.ts +++ b/source/as-promise/index.ts @@ -27,7 +27,7 @@ export default function asPromise(options: NormalizedOptions): CancelableRequ let globalResponse: Response; const emitter = new EventEmitter(); - const promise = new PCancelable((resolve, reject, onCancel) => { + const promise = new PCancelable((resolve, _reject, onCancel) => { const makeRequest = (): void => { // Support retries // `options.throwHttpErrors` needs to be always true, @@ -38,10 +38,25 @@ export default function asPromise(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) => { diff --git a/source/core/index.ts b/source/core/index.ts index 8f59cfb2b..76f588aa9 100644 --- a/source/core/index.ts +++ b/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,6 +385,8 @@ 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'; @@ -388,6 +394,8 @@ export class CacheError extends RequestError { } export class UploadError extends RequestError { + declare readonly request: Request; + constructor(error: Error, request: Request) { super(error.message, error, request); this.name = 'UploadError'; @@ -395,6 +403,7 @@ export class UploadError extends RequestError { } 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 { // `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 { 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 { // `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 { } // `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 { } } - if (defaults) { + if (defaults && areHooksUserDefined) { for (const event of knownHookEvents) { const defaultHooks = defaults.hooks[event]; diff --git a/test/error.ts b/test/error.ts index 464aae465..a134f9098 100644 --- a/test/error.ts +++ b/test/error.ts @@ -201,7 +201,7 @@ test('errors can have request property', withServer, async (t, server, got) => { const error = await t.throwsAsync(got('')); t.truthy(error.response); - t.truthy(error.request!.downloadProgress); + t.truthy(error.request.downloadProgress); }); // Fails randomly on Node 10: diff --git a/test/hooks.ts b/test/hooks.ts index bbbe17c94..18f542c0a 100644 --- a/test/hooks.ts +++ b/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); +});