Skip to content
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

feat: throttle getAll* methods and rename getAll to dangerouslyGetAll #193

Merged
merged 16 commits into from
Nov 8, 2021

Conversation

angeloashmore
Copy link
Member

@angeloashmore angeloashmore commented Oct 1, 2021

Types of changes

  • Chore (a non-breaking change which is related to package maintenance)
  • Bug fix (a non-breaking change which fixes an issue)
  • New feature (a non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Description

This adds throttle-like behavior to all getAll* methods. 500ms is used as the duration between network requests.

This ensures API performance is sustainable even with large queries. getAll* methods should be treated as long-running requests that may make multiple network requests in a safe timely manner.

The following algorithm is used:

  1. Query for a page of results.

  2. If the result tells us there are additional pages:

    1. Wait the throttle threshold duration (500ms).
    2. Return to Step 1 with the next page.

    If there are no additional pages:

    1. Return the aggregate list of pages.

For example, with a 3 page query:

Request page 1 - 250ms
[wait 500ms]
Request page 2 - 350ms
[wait 500ms]
Request page 3 - 250ms

DONE
Total time: 1850ms

With a 1 page query:

Request page 1 - 250ms

DONE
Total time: 250ms

This PR also renames getAll to dangerouslyGetAll to dissuade usage.

Checklist:

  • My change requires an update to the official documentation.
  • All TSDoc comments are up-to-date and new ones have been added where necessary.
  • All new and existing tests are passing.

🐢

@angeloashmore angeloashmore marked this pull request as draft October 1, 2021 21:39
@angeloashmore angeloashmore requested a review from lihbr October 2, 2021 04:08
@angeloashmore angeloashmore marked this pull request as ready for review October 2, 2021 04:08
@angeloashmore angeloashmore added enhancement New feature or request v6 Getting addressed or related to version 6 of the kit labels Oct 2, 2021
Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

Awesome job! I just think there are some minor tweaks to be made but I really like the approach you took to throttle that neatly when in ideal conditions ☺️ You can go when resolved! 🚀

test/client-getAll.test.ts Outdated Show resolved Hide resolved
test/client-getAll.test.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Outdated Show resolved Hide resolved
src/client.ts Show resolved Hide resolved
@angeloashmore
Copy link
Member Author

Thanks for the review @lihbr @srenault! I made some changes and commented on your comments.

Copy link
Member

@lihbr lihbr left a comment

Choose a reason for hiding this comment

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

I'm happy with it as-is. Just to make sure I got it right:

  • getAll is now dangerouslyGetAll;
  • A 500ms delay between each request is added;
  • That's it(?)

@angeloashmore
Copy link
Member Author

Yep, that's right! I edited the original comment to reflect the latest simplifications.

If that's good with you, I'll merge it and publish in a new beta version (along with #196).

@lihbr
Copy link
Member

lihbr commented Oct 22, 2021

Yup, good for me! @srenault is it a green light for you also?

If yes, we'll proceed to update the documentation and bring to the user more consideration before they start using these methods.

@angeloashmore angeloashmore changed the title feat: throttle getAll* methods feat: throttle getAll* methods and rename getAll to dangerouslyGetAll Oct 28, 2021
@angeloashmore angeloashmore merged commit 4efdfa0 into v6 Nov 8, 2021
@angeloashmore angeloashmore deleted the aa/throttle-get-all branch November 8, 2021 23:18
@angeloashmore
Copy link
Member Author

This was approved by @srenault outside GitHub. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v6 Getting addressed or related to version 6 of the kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants