Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions API_DOCS.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ Describes the shape and behaviour of the resources object you will pass to `getL
| `isBatchKeyASet` | (Optional) Set to true if the interface of the resource takes the batch key as a set (rather than an array). For example, when using a generated clientlib based on swagger where `uniqueItems: true` is set for the batchKey parameter. Default: false. |
| `propertyBatchKey` | (Optional) The argument to the resource that represents the optional properties we want to fetch. (e.g. usually 'properties' or 'features'). |
| `responseKey` | (Non-optional when propertyBatchKey is used) The key in the response objects corresponds to `batchKey`. This should be the only field that are marked as required in your swagger endpoint response, except nestedPath. |
| `maxBatchSize` | (Optional) Limits the number of items that can be batched together in a single request. When more items are requested than this limit, multiple requests will be made. This can help prevent hitting URI length limits or timeouts for large batches. |

### `typings`

Expand Down
121 changes: 121 additions & 0 deletions __tests__/implementation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1246,6 +1246,127 @@ test('bail if errorHandler does not return an error', async () => {
});
});

test('batch endpoint with maxBatchSize', async () => {
const config = {
resources: {
foo: {
isBatchResource: true,
docsLink: 'example.com/docs/bar',
batchKey: 'foo_ids',
newKey: 'foo_id',
responseKey: 'id',
maxBatchSize: 3,
},
},
};

// Track each batch of IDs that the resource function receives
const receivedBatches = [];

const resources = {
foo: ({ foo_ids, properties }) => {
receivedBatches.push([...foo_ids]);
return Promise.resolve(
foo_ids.map(id => ({
id,
name: `name-${id}`,
rating: id + 1
}))
);
},
};

await createDataLoaders(config, async (getLoaders) => {
const loaders = getLoaders(resources);

// Request 5 items at once, which should be split by maxBatchSize later in the test.
const results = await Promise.all([
loaders.foo.load({ foo_id: 1, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 2, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 3, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 4, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 5, properties: ['name', 'rating'] }),
]);

// Verify that all results were returned correctly
expect(results).toEqual([
{ id: 1, name: 'name-1', rating: 2 },
{ id: 2, name: 'name-2', rating: 3 },
{ id: 3, name: 'name-3', rating: 4 },
{ id: 4, name: 'name-4', rating: 5 },
{ id: 5, name: 'name-5', rating: 6 },
]);

// Verify that the requests were batched correctly
expect(receivedBatches.map(batch => batch.length)).toEqual([3, 2]);

// Verify that all IDs were requested
const allRequestedIds = receivedBatches.flat().sort();
expect(allRequestedIds).toEqual([1, 2, 3, 4, 5]);
});
});

test('batch endpoint with propertyBatchKey and maxBatchSize', async () => {
const config = {
resources: {
foo: {
isBatchResource: true,
docsLink: 'example.com/docs/bar',
batchKey: 'foo_ids',
newKey: 'foo_id',
propertyBatchKey: 'properties',
responseKey: 'id',
maxBatchSize: 2,
},
},
};

// Track each batch of IDs that the resource function receives
const receivedBatches = [];

const resources = {
foo: ({ foo_ids, properties }) => {
receivedBatches.push([...foo_ids]);
return Promise.resolve(
foo_ids.map(id => ({
id,
name: `name-${id}`,
rating: id + 1
}))
);
},
};

await createDataLoaders(config, async (getLoaders) => {
const loaders = getLoaders(resources);

// Request 5 items at once, which should be split by maxBatchSize later in the test.
const results = await Promise.all([
loaders.foo.load({ foo_id: 1, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 2, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 3, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 4, properties: ['name', 'rating'] }),
loaders.foo.load({ foo_id: 5, properties: ['name', 'rating'] }),
]);

// Verify that all results were returned correctly
expect(results).toEqual([
{ id: 1, name: 'name-1', rating: 2 },
{ id: 2, name: 'name-2', rating: 3 },
{ id: 3, name: 'name-3', rating: 4 },
{ id: 4, name: 'name-4', rating: 5 },
{ id: 5, name: 'name-5', rating: 6 },
]);

// Verify that the requests were batched correctly
expect(receivedBatches.map(batch => batch.length)).toEqual([2, 2, 1]);

// Verify that all IDs were requested
const allRequestedIds = receivedBatches.flat().sort();
expect(allRequestedIds).toEqual([1, 2, 3, 4, 5]);
});
});

test('batch endpoint (multiple requests) with propertyBatchKey', async () => {
const config = {
resources: {
Expand Down
1 change: 1 addition & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export interface BatchResourceConfig {
isBatchKeyASet?: boolean;
propertyBatchKey?: string;
responseKey?: string;
maxBatchSize?: number;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

++ good catch, totally forgot about that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

}

export interface NonBatchResourceConfig {
Expand Down
6 changes: 4 additions & 2 deletions src/implementation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,8 @@ function getBatchLoader(resourceConfig: BatchResourceConfig, resourcePath: Reado
* flow errors :(
*/ ''
}
...cacheKeyOptions
...cacheKeyOptions,
${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''}
}
)`;
}
Expand Down Expand Up @@ -514,7 +515,8 @@ function getPropertyBatchLoader(resourceConfig: BatchResourceConfig, resourcePat
* flow errors :(
*/ ''
}
...cacheKeyOptions
...cacheKeyOptions,
${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''}
Copy link

Copilot AI May 2, 2025

Choose a reason for hiding this comment

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

The conditional check uses a truthy test for resourceConfig.maxBatchSize, which will skip valid values like 0. Consider explicitly checking for undefined (e.g. resourceConfig.maxBatchSize !== undefined) to ensure all valid numeric values are included.

Suggested change
${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''}
${resourceConfig.maxBatchSize !== undefined ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@mikoarce mikoarce May 2, 2025

Choose a reason for hiding this comment

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

Checking this as falsy conditional cause I don't think we'd want to set a max batch size of 0 anyway.

But I can swap this to fall back to Infinity instead since that seems like what it defaults to under the hood anyway.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

0 is probably not a valid value tbf here @mikoarce so we should be good to ignore this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we need to add this to getBatchLoader too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if we had to, but we probably should since it's also about batched calls. I can add that in there too!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made change here: fec60be added a test for this one as well.

}
)`;
}
Expand Down
Loading