-
Notifications
You must be signed in to change notification settings - Fork 26
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
refactor: New util to fetch same url multiple times to avoid exceeding the max length with one query parameter #265
refactor: New util to fetch same url multiple times to avoid exceeding the max length with one query parameter #265
Conversation
… avoid exceding the max length
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.
Great abstraction 👍 I've left some comments to review.
return (data as PaginatedResponse<T>).results !== undefined | ||
} | ||
|
||
export async function queryMultipleTimesWhenExceedingUrlLimit<T>( |
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.
Would you mind adding tests for this new auxiliary function?
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.
Done!
src/ports/utils.ts
Outdated
const param = | ||
(alreadyHasQueryParams ? `&` : '') + `${queryParameterName}=${value}` |
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.
This should, if it's the first element of the array, choose between using ?
or &
depending on if it already has query parameters or not, and then, for all of the other cases, append &
.
As it is here, if the base url doesn't have query parameters, it will concatenate all of the query parameters without the use of &.
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.
Done!
Co-authored-by: Lautaro Petaccio <1120791+LautaroPetaccio@users.noreply.github.com> Signed-off-by: Kevin Szuchet <31735779+kevinszuchet@users.noreply.github.com>
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.
Looks great 🚀
Closes #263