Add max batch size config option#351
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new configuration option for controlling the maximum batch size for property batch loaders.
- Adds an optional maxBatchSize option to BatchResourceConfig in config.ts.
- Updates the property batch loader to conditionally include maxBatchSize in DataLoader options in implementation.ts.
- Provides a new test to verify that batching is handled properly when maxBatchSize is set.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/implementation.ts | Injects maxBatchSize into DataLoader options for property batch loaders |
| src/config.ts | Defines the new optional maxBatchSize property in BatchResourceConfig |
| tests/implementation.test.js | Adds a new test to validate the batching behavior with maxBatchSize |
| } | ||
| ...cacheKeyOptions | ||
| ...cacheKeyOptions, | ||
| ${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''} |
There was a problem hiding this comment.
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.
| ${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''} | |
| ${resourceConfig.maxBatchSize !== undefined ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
0 is probably not a valid value tbf here @mikoarce so we should be good to ignore this
| } | ||
| ...cacheKeyOptions | ||
| ...cacheKeyOptions, | ||
| ${resourceConfig.maxBatchSize ? `maxBatchSize: ${resourceConfig.maxBatchSize},` : ''} |
There was a problem hiding this comment.
Do we need to add this to getBatchLoader too?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Made change here: fec60be added a test for this one as well.
| isBatchKeyASet?: boolean; | ||
| propertyBatchKey?: string; | ||
| responseKey?: string; | ||
| maxBatchSize?: number; |
There was a problem hiding this comment.
Probably want to update the docs as well https://sourcegraph.yelpcorp.com/Yelp/dataloader-codegen/-/blob/API_DOCS.md
There was a problem hiding this comment.
++ good catch, totally forgot about that.
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new configuration option – maxBatchSize – to allow large requests to be broken up into smaller batches. The changes include propagating the new configuration through the DataLoader creation logic (in both batch and property batch loaders), updating the configuration interface, and adding tests and documentation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/implementation.ts | Updated DataLoader initialization to include maxBatchSize when set |
| src/config.ts | Extended the BatchResourceConfig interface with maxBatchSize |
| tests/implementation.test.js | Added tests to validate batching behavior based on maxBatchSize |
| API_DOCS.md | Documented the new maxBatchSize configuration option |
This PR exposes the
maxBatchSizeoption so we can break up large requests if we have to.