Skip to content

Commit

Permalink
Fix duplicated hooks when paginating
Browse files Browse the repository at this point in the history
  • Loading branch information
@delilahw @szmarczak
delilahw authored and szmarczak committed Jul 28, 2020
1 parent a3e171c commit e02845f
Show file tree
Hide file tree
Showing 2 changed files with 254 additions and 11 deletions.
19 changes: 9 additions & 10 deletions source/create.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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++](
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -199,7 +199,7 @@ const create = (defaults: InstanceDefaults): Got => {
};

// Pagination
const paginateEach = (async function * <T, R>(url: string | URL, options?: OptionsWithPagination<T, R>) {
const paginateEach = (async function * <T, R>(url: string | URL, options?: OptionsWithPagination<T, R>): AsyncIterableIterator<T> {
// TODO: Remove this `@ts-expect-error` when upgrading to TypeScript 4.
// Error: Argument of type 'Merge<Options, PaginationOptions<T, R>> | undefined' is not assignable to parameter of type 'Options | undefined'.
// @ts-expect-error
Expand All @@ -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);
Expand All @@ -236,7 +237,7 @@ const create = (defaults: InstanceDefaults): Got => {
return;
}

yield item;
yield item as T;

if (pagination.stackAllItems) {
all.push(item as T);
Expand Down Expand Up @@ -266,14 +267,12 @@ const create = (defaults: InstanceDefaults): Got => {
}
});

got.paginate = (<T, R>(url: string | URL, options?: OptionsWithPagination<T, R>) => {
return paginateEach(url, options);
}) as GotPaginate;
got.paginate = paginateEach as GotPaginate;

got.paginate.all = (async <T, R>(url: string | URL, options?: OptionsWithPagination<T, R>) => {
const results: T[] = [];

for await (const item of got.paginate<T, R>(url, options)) {
for await (const item of paginateEach<T, R>(url, options)) {
results.push(item);
}

Expand Down
246 changes: 245 additions & 1 deletion test/hooks.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);
});

0 comments on commit e02845f

Please sign in to comment.