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

Add backoff option to pagination #1182

Merged
merged 3 commits into from
Jul 6, 2020
Merged

Conversation

jaulz
Copy link
Contributor

@jaulz jaulz commented Apr 24, 2020

This PR adds an easy possibility to respect rate limits that are enforced by some APIs. If the rate limit is 600 per hour and you know that you will most likely hit the limit you can set backoff to 100 which means that paginated requests are triggered only every 100 milliseconds. Unfortunately, I had no clever idea how to implement a test but maybe you have an idea?

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

source/create.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

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

This needs tests

@jaulz
Copy link
Contributor Author

jaulz commented Apr 24, 2020

Yep, definitely. That why I mentioned in the description that I had no clever idea how to test it 😉 Any idea how we could simulate this?

@szmarczak
Copy link
Collaborator

Faketimers (lolex)

@jaulz
Copy link
Contributor Author

jaulz commented Apr 24, 2020

@szmarczak yeah, I saw that you used it in other tests as well but I couldn't really figure out how to properly use it in this case.

@sindresorhus sindresorhus changed the title feat: add backoff option to pagination Add backoff option to pagination Apr 26, 2020
@sindresorhus sindresorhus changed the title Add backoff option to pagination Add backoff option to pagination Apr 27, 2020
@szmarczak
Copy link
Collaborator

This PR adds an easy possibility to respect rate limits that are enforced by some APIs.

Actually do we need a backoff option? got.paginate() uses the Promise API under the hood, so it has got a retry mechanism. It also respects the Retry-After header.

source/create.ts Outdated Show resolved Hide resolved
@jaulz
Copy link
Contributor Author

jaulz commented May 14, 2020

Retry means that we accept running into potential server errors whereas backoff would allow us to prevent server errors from the beginning if you know the rate limits.

@jaulz
Copy link
Contributor Author

jaulz commented May 14, 2020

I was just about to give the tests another try but after installing all dependencies and run yarn testI see these issues:

Error: Failed to load config "xo-typescript" to extend from.
Referenced from: BaseConfig

Is there anything wrong on master at the moment?

@szmarczak
Copy link
Collaborator

Looks like you're missing npm install...

@jaulz
Copy link
Contributor Author

jaulz commented May 14, 2020

Not really I think... I even removed the node_modules and did a fresh yarn.

@szmarczak
Copy link
Collaborator

What about if you use npm instead? We don't use yarn [yet?] :)

@jaulz
Copy link
Contributor Author

jaulz commented May 15, 2020

Hm, honestly I have no clue how that test system works... I would have assumed that it would work with my current approach but then again I am running into typing issues (i.e. end does not exist on t).

@szmarczak
Copy link
Collaborator

No worries, I'll fix it.

@jaulz
Copy link
Contributor Author

jaulz commented May 15, 2020

@szmarczak thanks! I'm curious how you will do it 😊 I couldn't even see that my test was executed. Is there actually a simple way to run a single test (if I use only I see lint issues) in watch mode? And is it also somehow possible to log some stuff in between for debugging?

@sindresorhus
Copy link
Owner

@jaulz Bump :)

@jaulz
Copy link
Contributor Author

jaulz commented Jul 6, 2020

@sindresorhus yeah, I would still love to see this as part of the package but unfortunately I was stuck with the test set up 😞 @szmarczak could you by any chance find some time and fix the test?

@szmarczak
Copy link
Collaborator

I'll do this today. I'm not home rn, will be in two/three hours.

@szmarczak
Copy link
Collaborator

After 30+ minutes of trying to introduce fake-timers, I'm giving up. It doesn't have clock.runUntilNextAsync which makes things extremely hard. I'll go with native setTimeout.

@szmarczak szmarczak merged commit 4be7446 into sindresorhus:master Jul 6, 2020
@jaulz
Copy link
Contributor Author

jaulz commented Jul 7, 2020

@szmarczak thanks a lot!

@jaulz jaulz deleted the patch-3 branch July 7, 2020 05:46
@szmarczak
Copy link
Collaborator

No problem, sorry I hadn't got to this before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants