kontent-ai / delivery-sdk-js

Kontent Delivery SDK for Javascript
https://kontent.ai
MIT License
50 stars 34 forks source link

.toAllPromise() overrides limit parameter #366

Closed Nickm615 closed 1 year ago

Nickm615 commented 1 year ago

Brief bug description

When using .toAllPromise() the .limitParameter() is not applied.

Repro steps

Create request with both limit parameter and toAllPromise applied, here is mine as an example:

const response = await(deliveryClient.items().type('article').limitParameter(1).toAllPromise())

Expected behavior

Only 1 article is returned in API response

Actual Behavior

All articles are returned

Test environment

Node 18.16.0

Additional context

Related intercom support conversation here

Nickm615 commented 1 year ago

@Enngage Are .toAllPromise() and .limitParameter() intended to be compatible? I can't think of a use case where you'd really need them to work together, but in the readme we use them together

// this executed multiple HTTP requests until it gets all items const response = await deliveryClient.items().limitParameter(5).toAllPromise();

So is it a bug that they don't work together, or is it intended and we simply need to update the readme? In either case I'm happy to take this on.

Enngage commented 1 year ago

@Nickm615 I believe this is intentional as the limit basically acts as a page size when used in combination with toAllPromise. If you have e.g. 10k items (and the Delivery limit is 2000 in a single response) and you want to load e.g. 5k items, you might want to use limit in combination with the toAllPromise to get the items you need.

Sure, in this case you could probably also use items-feed, but I think it's ok to have the option to control this with items endpoint as well.