-
Notifications
You must be signed in to change notification settings - Fork 184
docs: add "Batching" examples #1161
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
base: main
Are you sure you want to change the base?
Conversation
|
|
||
| ### Multiple Tables & Entities | ||
|
|
||
| ### Handling `BatchGetItem` Limits |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plan for this section is to have examples for handling partial responses via UnprocessedKeys and batch size edge cases (i.e. chunking requests to 100 items per request).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to sprinkle limits inside the examples, or at least give a warning (with :::caution admonitions for instance) that points to that section. Otherwise some devs might just overlook it.
| const pokemonIds = ['bulbasaur', 'charmander', 'squirtle'] | ||
|
|
||
| // 👇 Map data into a collection of `BatchGetRequest` actions | ||
| const requests = pokemonIds.map((pokemonId) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick but you can remove the bracket (and simply use pokemonId as key):
const requests = pokemonIds.map((pokemonId) =>
PokemonEntity.build(BatchGetRequest).key({ pokemonId })
)| const { Responses } = await execute(command) | ||
|
|
||
| // 👇 Extract your data from the responses | ||
| const [pokemon] = Responses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this will be clearer?
// 👇 `Responses` is an array of size 1 as we only sent one `BatchGetCommand`
const [pokemons] = Responses
for (const pokemon of pokemons) {
// ...do something with pokemon here
// (note that pokemon might be `undefined`)
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure! I do think using .map() to iterate would translate better to real-world use cases though as internally we haven't come across a scenario where using a for of loop has been needed. I'm not too precious on syntax though, thoughts?
|
@tranhl Awesome! Did some minor feedbacks but it looks good to me, thanks a lot! |
| return pokemon.filter(item => item !== undefined) | ||
| ``` | ||
|
|
||
| ### Single Table, Multiple Entities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasAribart It turns out we don't have a use case internally where we need to retrieve multiple entities in batch so I'm coming up with an example myself. Since we're limited to one BatchGetCommand per table, I tried passing multiple types of BatchGetRequests into a BatchGetCommand but ran into a ts(2859) Excessive complexity comparing types error. Any recommendations on how to best approach this use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder whether this use case is even worth including in the documentation 🤔? Batch retrieving multiple entities from a single table screams code smell to me in the context of single table design -- I'm struggling to think of a scenario where this would be useful/valid. Perhaps we should remove it?
|
|
||
| ### Multiple Tables & Entities | ||
|
|
||
| ### Handling `BatchGetItem` Edge Cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasAribart I've fleshed out the "Handling BatchGetItem Edge Cases" section, let me know what you think!
| #### Handling Empty Input Keys | ||
|
|
||
| TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still thinking about how to best write this section. Also considering rolling it into the previous section as we've handled this internally with a wrapper function, but I'm not 100% happy with the solution.
This PR adds a "Batching" page to the "Examples" section on the documentation website.