From e02845f1aa737d55dce23381d0f4f2a61b1eb5e1 Mon Sep 17 00:00:00 2001 From: David Wu Date: Tue, 28 Jul 2020 10:44:16 +0200 Subject: [PATCH] Fix duplicated hooks when paginating --- source/create.ts | 19 ++-- test/hooks.ts | 246 ++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 254 insertions(+), 11 deletions(-) diff --git a/source/create.ts b/source/create.ts index a930dd230..09231a42d 100644 --- a/source/create.ts +++ b/source/create.ts @@ -38,7 +38,7 @@ import { StreamOptions } from './types'; import createRejection from './as-promise/create-rejection'; -import Request, {kIsNormalizedAlready, setNonEnumerableProperties} from './core'; +import Request, {kIsNormalizedAlready, setNonEnumerableProperties, Defaults} from './core'; import deepFreeze from './utils/deep-freeze'; const errors = { @@ -114,7 +114,7 @@ const create = (defaults: InstanceDefaults): Got => { })); // Got interface - const got: Got = ((url: string | URL, options?: Options): GotReturn => { + const got: Got = ((url: string | URL, options?: Options, _defaults?: Defaults): GotReturn => { let iteration = 0; const iterateHandlers = (newOptions: NormalizedOptions): GotReturn => { return defaults.handlers[iteration++]( @@ -147,7 +147,7 @@ const create = (defaults: InstanceDefaults): Got => { } // Normalize options & call handlers - const normalizedOptions = normalizeArguments(url, options, defaults.options); + const normalizedOptions = normalizeArguments(url, options, _defaults ?? defaults.options); normalizedOptions[kIsNormalizedAlready] = true; if (initHookError) { @@ -199,7 +199,7 @@ const create = (defaults: InstanceDefaults): Got => { }; // Pagination - const paginateEach = (async function * (url: string | URL, options?: OptionsWithPagination) { + const paginateEach = (async function * (url: string | URL, options?: OptionsWithPagination): AsyncIterableIterator { // TODO: Remove this `@ts-expect-error` when upgrading to TypeScript 4. // Error: Argument of type 'Merge> | undefined' is not assignable to parameter of type 'Options | undefined'. // @ts-expect-error @@ -222,9 +222,10 @@ const create = (defaults: InstanceDefaults): Got => { await delay(pagination.backoff); } + // @ts-expect-error FIXME! // TODO: Throw when result is not an instance of Response // eslint-disable-next-line no-await-in-loop - const result = (await got(normalizedOptions)) as Response; + const result = (await got(undefined, undefined, normalizedOptions)) as Response; // eslint-disable-next-line no-await-in-loop const parsed = await pagination.transform(result); @@ -236,7 +237,7 @@ const create = (defaults: InstanceDefaults): Got => { return; } - yield item; + yield item as T; if (pagination.stackAllItems) { all.push(item as T); @@ -266,14 +267,12 @@ const create = (defaults: InstanceDefaults): Got => { } }); - got.paginate = ((url: string | URL, options?: OptionsWithPagination) => { - return paginateEach(url, options); - }) as GotPaginate; + got.paginate = paginateEach as GotPaginate; got.paginate.all = (async (url: string | URL, options?: OptionsWithPagination) => { const results: T[] = []; - for await (const item of got.paginate(url, options)) { + for await (const item of paginateEach(url, options)) { results.push(item); } diff --git a/test/hooks.ts b/test/hooks.ts index a72d44247..130b8199e 100644 --- a/test/hooks.ts +++ b/test/hooks.ts @@ -7,7 +7,7 @@ import sinon = require('sinon'); import delay = require('delay'); import {Handler} from 'express'; import Responselike = require('responselike'); -import got, {RequestError, HTTPError} from '../source'; +import got, {RequestError, HTTPError, Response} from '../source'; import withServer from './helpers/with-server'; const errorString = 'oops'; @@ -950,3 +950,247 @@ test('beforeRequest hook respect `url` option', withServer, async (t, server, go } })).body, 'ok'); }); + +test('no duplicate hook calls in single-page paginated requests', withServer, async (t, server, got) => { + server.get('/get', (_request, response) => { + response.end('i <3 koalas'); + }); + + let beforeHookCount = 0; + let beforeHookCountAdditional = 0; + let afterHookCount = 0; + let afterHookCountAdditional = 0; + + const hooks = { + beforeRequest: [ + () => { + beforeHookCount++; + } + ], + afterResponse: [ + (response: any) => { + afterHookCount++; + return response; + } + ] + }; + + // Test only one request + const instance = got.extend({ + hooks, + pagination: { + paginate: () => false, + countLimit: 2009, + transform: response => [response] + } + }); + + await instance.paginate.all('get'); + t.is(beforeHookCount, 1); + t.is(afterHookCount, 1); + + await instance.paginate.all('get', { + hooks: { + beforeRequest: [ + () => { + beforeHookCountAdditional++; + } + ], + afterResponse: [ + (response: any) => { + afterHookCountAdditional++; + return response; + } + ] + } + }); + t.is(beforeHookCount, 2); + t.is(afterHookCount, 2); + t.is(beforeHookCountAdditional, 1); + t.is(afterHookCountAdditional, 1); + + await got.paginate.all('get', { + hooks, + pagination: { + paginate: () => false, + transform: response => [response] + } + }); + + t.is(beforeHookCount, 3); + t.is(afterHookCount, 3); +}); + +test('no duplicate hook calls in sequential paginated requests', withServer, async (t, server, got) => { + server.get('/get', (_request, response) => { + response.end('i <3 unicorns'); + }); + + let requestNumber = 0; + let beforeHookCount = 0; + let afterHookCount = 0; + + const hooks = { + beforeRequest: [ + () => { + beforeHookCount++; + } + ], + afterResponse: [ + (response: any) => { + afterHookCount++; + return response; + } + ] + }; + + // Test only two requests, one after another + const paginate = () => requestNumber++ === 0 ? {} : false; + + const instance = got.extend({ + hooks, + pagination: { + paginate, + countLimit: 2009, + transform: response => [response] + } + }); + + await instance.paginate.all('get'); + + t.is(beforeHookCount, 2); + t.is(afterHookCount, 2); + requestNumber = 0; + + await got.paginate.all('get', { + hooks, + pagination: { + paginate, + transform: response => [response] + } + }); + + t.is(beforeHookCount, 4); + t.is(afterHookCount, 4); +}); + +test('intentional duplicate hooks in pagination with extended instance', withServer, async (t, server, got) => { + server.get('/get', (_request, response) => { + response.end('<3'); + }); + + let beforeCount = 0; // Number of times the hooks from `extend` are called + let afterCount = 0; + let beforeCountAdditional = 0; // Number of times the added hooks are called + let afterCountAdditional = 0; + + const beforeHook = () => { + beforeCount++; + }; + + const afterHook = (response: any) => { + afterCount++; + return response; + }; + + const instance = got.extend({ + hooks: { + beforeRequest: [ + beforeHook, + beforeHook + ], + afterResponse: [ + afterHook, + afterHook + ] + }, + pagination: { + paginate: () => false, + countLimit: 2009, + transform: response => [response] + } + }); + + // Add duplicate hooks when calling paginate + const beforeHookAdditional = () => { + beforeCountAdditional++; + }; + + const afterHookAdditional = (response: any) => { + afterCountAdditional++; + return response; + }; + + await instance.paginate.all('get', { + hooks: { + beforeRequest: [ + beforeHook, + beforeHookAdditional, + beforeHookAdditional + ], + afterResponse: [ + afterHook, + afterHookAdditional, + afterHookAdditional + ] + } + }); + + t.is(beforeCount, 3); + t.is(afterCount, 3); + t.is(beforeCountAdditional, 2); + t.is(afterCountAdditional, 2); +}); + +test('no duplicate hook calls when returning original request options', withServer, async (t, server, got) => { + server.get('/get', (_request, response) => { + response.end('i <3 unicorns'); + }); + + let requestNumber = 0; + let beforeHookCount = 0; + let afterHookCount = 0; + + const hooks = { + beforeRequest: [ + () => { + beforeHookCount++; + } + ], + afterResponse: [ + (response: any) => { + afterHookCount++; + return response; + } + ] + }; + + // Test only two requests, one after another + const paginate = (response: Response) => requestNumber++ === 0 ? response.request.options : false; + + const instance = got.extend({ + hooks, + pagination: { + paginate, + countLimit: 2009, + transform: response => [response] + } + }); + + await instance.paginate.all('get'); + + t.is(beforeHookCount, 2); + t.is(afterHookCount, 2); + requestNumber = 0; + + await got.paginate.all('get', { + hooks, + pagination: { + paginate, + transform: response => [response] + } + }); + + t.is(beforeHookCount, 4); + t.is(afterHookCount, 4); +});