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

Added rate limiting for callback or promise usage. Trello limits requ… #23

Closed
wants to merge 1 commit into from

Conversation

symi
Copy link

@symi symi commented Dec 20, 2015

I've been making a bulk uploader into trello, and hit the trello api rate limit. Thought it might be something nice to put into this project so added simple-rate-limiter module (no license and requires node v0.10.1+, project has tests). By default it is turned off, but the config is set at the token/key rate of 100 per 10 second interval. I've made it wrap both callback and promise APIs. There's some tests to check it works both rate limited and non rate limited. Plus updated the README.

If its something you want in the project, feel free to merge.

…ests to its APIs (http://help.trello.com/article/838-api-rate-limits).

By default rate limiting is turned off and is set to limit at a single API key and token rate (100 requests per a 10 second interval).
Rate limiting is toggled via a static exposed limitRate property on the wrapper object, and rate amounts can be edited via a limits object.
@norberteder
Copy link
Owner

Would you please resolve the merge conflicts?

@symi
Copy link
Author

symi commented Nov 6, 2017

It doesn't looks possible to resolve the conflicts. By the looks of it #43 has solved the same issue (but using a random delay). Implementation details differ enough that both cannot and should not exist together.

#43 is a simpler solution without a 3rd party module however doesn't guarantee ordering nor earliest possible execution time (if limit is breached). This PR I believe will guarantee order and execute requests at the earliest possible time (its been a while since I did the PR so can't remember the details 100%).

As #43 is in the project I don't mind you closing this PR if you feel that is solving the problem.

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.

None yet

2 participants