Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the stackAllItems option to be false by default #1645

Merged
merged 12 commits into from
Feb 26, 2021
17 changes: 10 additions & 7 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -939,8 +939,8 @@ Default: [`Link` header logic](source/index.ts)

The function takes an object with the following properties:
- `response` - The current response object.
- `allItems` - An array of the emitted items.
- `currentItems` - Items from the current response.
- `allItems` - An empty array, unless [`pagination.stackAllItems`](#pagination.stackAllItems) is set to `true`, in which case, it's an array of the emitted items.

It should return an object representing Got options pointing to the next page. The options are merged automatically with the previous request, therefore the options returned `pagination.paginate(...)` must reflect changes only. If there are no more pages, `false` should be returned.

Expand All @@ -958,7 +958,7 @@ const got = require('got');
offset: 0
},
pagination: {
paginate: ({response, allItems, currentItems}) => {
paginate: ({response, currentItems, allItems}) => {
const previousSearchParams = response.request.options.searchParams;
const previousOffset = previousSearchParams.get('offset');

Expand All @@ -983,14 +983,14 @@ const got = require('got');
###### pagination.filter

Type: `Function`\
Default: `({item, allItems, currentItems}) => true`
Default: `({item, currentItems, allItems}) => true`

Checks whether the item should be emitted or not.

###### pagination.shouldContinue

Type: `Function`\
Default: `({item, allItems, currentItems}) => true`
Default: `({item, currentItems, allItems}) => true`

Checks whether the pagination should continue.

Expand Down Expand Up @@ -1022,11 +1022,14 @@ For example, it can be helpful during development to avoid an infinite number of
###### pagination.stackAllItems

Type: `boolean`\
Default: `true`
Default: `false`

Defines how the property `allItems` in [`pagination.paginate`](#pagination.paginate), [`pagination.filter`](#pagination.filter) and [`pagination.shouldContinue`](#pagination.shouldContinue) is managed.

By default, the property `allItems` is always an empty array.
szmarczak marked this conversation as resolved.
Show resolved Hide resolved

Defines how the parameter `allItems` in [pagination.paginate](#pagination.paginate), [pagination.filter](#pagination.filter) and [pagination.shouldContinue](#pagination.shouldContinue) is managed. When set to `false`, the parameter `allItems` is always an empty array.
When set to `false`, the parameter `allItems` is always an empty array. This setting can impact memory usage when working with a large dataset.
szmarczak marked this conversation as resolved.
Show resolved Hide resolved

This option can be helpful to save on memory usage when working with a large dataset.

##### localAddress

Expand Down
19 changes: 10 additions & 9 deletions source/as-promise/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ export type ResponseType = 'json' | 'buffer' | 'text';

export interface PaginateData<BodyType, ElementType> {
response: Response<BodyType>;
allItems: ElementType[];
currentItems: ElementType[];
allItems: ElementType[];
}

export interface FilterData<ElementType> {
item: ElementType;
allItems: ElementType[];
currentItems: ElementType[];
allItems: ElementType[];
}

export interface PaginationOptions<ElementType, BodyType> {
Expand All @@ -39,15 +39,15 @@ export interface PaginationOptions<ElementType, BodyType> {
/**
Checks whether the item should be emitted or not.

@default ({item, allItems, currentItems}) => true
@default ({item, currentItems, allItems}) => true
*/
filter?: (data: FilterData<ElementType>) => boolean;

/**
The function takes an object with the following properties:
- `response` - The current response object.
- `allItems` - An array of the emitted items.
- `currentItems` - Items from the current response.
- `allItems` - An empty array, unless when `pagination.stackAllItems` is set to `true`. In the later case, an array of the emitted items.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PopGoesTheWza You forgot to keep the TS docs in sync.


It should return an object representing Got options pointing to the next page.
The options are merged automatically with the previous request, therefore the options returned `pagination.paginate(...)` must reflect changes only.
Expand All @@ -66,7 +66,7 @@ export interface PaginationOptions<ElementType, BodyType> {
offset: 0
},
pagination: {
paginate: (response, allItems, currentItems) => {
paginate: ({response, currentItems}) => {
const previousSearchParams = response.request.options.searchParams;
const previousOffset = previousSearchParams.get('offset');

Expand Down Expand Up @@ -98,7 +98,7 @@ export interface PaginationOptions<ElementType, BodyType> {
If you want to stop **after** emitting the entry, you should use
`({item, allItems}) => allItems.some(item => item.flag)` instead.

@default ({item, allItems, currentItems}) => true
@default ({item, currentItems, allItems}) => true
*/
shouldContinue?: (data: FilterData<ElementType>) => boolean;

Expand Down Expand Up @@ -126,10 +126,11 @@ export interface PaginationOptions<ElementType, BodyType> {
requestLimit?: number;

/**
Defines how the parameter `allItems` in pagination.paginate, pagination.filter and pagination.shouldContinue is managed.
When set to `false`, the parameter `allItems` is always an empty array.
Defines how the property `allItems` in pagination.paginate, pagination.filter and pagination.shouldContinue is managed.

By default, the property `allItems` is always an empty array. This setting can be helpful to save on memory usage when working with a large dataset.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szmarczak Don't forget to keep the TS docs in sync.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, overlooked this one

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: 4cce4de

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, currently I'm not home. I'll be back tomorrow.


This option can be helpful to save on memory usage when working with a large dataset.
When set to `true`, the property `allItems` is an array of the emitted items.
*/
stackAllItems?: boolean;
};
Expand Down
8 changes: 4 additions & 4 deletions source/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,8 @@ const create = (defaults: InstanceDefaults): Got => {
const currentItems: T[] = [];

for (const item of parsed) {
if (pagination.filter({item, allItems, currentItems})) {
if (!pagination.shouldContinue({item, allItems, currentItems})) {
if (pagination.filter({item, currentItems, allItems})) {
if (!pagination.shouldContinue({item, currentItems, allItems})) {
return;
}

Expand All @@ -266,8 +266,8 @@ const create = (defaults: InstanceDefaults): Got => {

const optionsToMerge = pagination.paginate({
response,
allItems,
currentItems
currentItems,
allItems
});

if (optionsToMerge === false) {
Expand Down
2 changes: 1 addition & 1 deletion source/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ const defaults: InstanceDefaults = {
countLimit: Number.POSITIVE_INFINITY,
backoff: 0,
requestLimit: 10000,
stackAllItems: true
stackAllItems: false
},
parseJson: (text: string) => JSON.parse(text),
stringifyJson: (object: unknown) => JSON.stringify(object),
Expand Down
10 changes: 5 additions & 5 deletions test/pagination.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ test('filters elements', withServer, async (t, server, got) => {

const result = await got.paginate.all<number>({
pagination: {
filter: ({item, allItems, currentItems}) => {
filter: ({item, currentItems, allItems}) => {
t.true(Array.isArray(allItems));
t.true(Array.isArray(currentItems));

Expand Down Expand Up @@ -209,7 +209,7 @@ test('`shouldContinue` works', withServer, async (t, server, got) => {

const options = {
pagination: {
shouldContinue: ({allItems, currentItems}: {allItems: number[]; currentItems: number[]}) => {
shouldContinue: ({currentItems, allItems}: {allItems: number[]; currentItems: number[]}) => {
t.true(Array.isArray(allItems));
t.true(Array.isArray(currentItems));

Expand Down Expand Up @@ -496,11 +496,11 @@ test('`stackAllItems` set to true', withServer, async (t, server, got) => {

return true;
},
paginate: ({response, allItems, currentItems}) => {
paginate: ({response, currentItems, allItems}) => {
itemCount += 1;
t.is(allItems.length, itemCount);

return got.defaults.options.pagination!.paginate({response, allItems, currentItems});
return got.defaults.options.pagination!.paginate({response, currentItems, allItems});
}
}
});
Expand All @@ -524,7 +524,7 @@ test('`stackAllItems` set to false', withServer, async (t, server, got) => {

return true;
},
paginate: ({response, allItems, currentItems}) => {
paginate: ({response, currentItems, allItems}) => {
t.is(allItems.length, 0);

return got.defaults.options.pagination!.paginate({response, allItems, currentItems});
Expand Down