-
Notifications
You must be signed in to change notification settings - Fork 158
Add option to ignore limit check, use headers for limit check #152
Conversation
- Running prettier on Windows caused all files to change from CRLF to LF - npm scripts didn't work on Windows - Also fixed syntax in contributing.md Fixed list syntax in contributing.md
- Added `checkLimit` option to ignore API limit check
Realized that the |
Awesome PR @AustinLeeGordon . Really appreciated! I think I had confirmed with HubSpot that the checkLimit call does not count towards the API limit but I see the need for making it less chatty. |
Commit 76e937e uses response headers from each request to update the current usage. @pcothenet You're right - tested I left in The request doesn't do a check on the first call since |
@pcothenet I checked and the Check daily API usage endpoint does actually count against the daily and burst (per second) limit. I'm going to look into whether or not we can safely relax that restriction. In general, I recommend using the Disclaimer: I work for HubSpot I ran this experiment against an isolated account and I'm sure this was the only API traffic in flight at the time.
|
@chrisbaldauf Thanks for checking into that! For some reason it seemed to count for mine in some cases but didn't in others. Might have just missed something. The latest few commits in this thread change it to check from the |
Thanks @chrisbaldauf @AustinLeeGordon . I'll be able to review and hopefully GTM this weekend. |
@chrisbaldauf can you also shed some light on why the headers are not returned when using OAuth? Seems like a surprising design decision, no? |
Hey @pcothenet! Any update on this? Let me know if there's anything I can do to help! |
@AustinLeeGordon I'm still quite confused by @chrisbaldauf comment about "please be aware that those headers will be absent when using OAuth tokens". I know that in my use case, I was relying on this check to not blow up the API limit (and we're using OAuth) |
But all things considered, I think this is an improvement. Merging. |
@pcothenet Glad to see it go through! I haven’t used it with OAuth, but does it typically count toward the api count whenever using OAuth? If not, then this shouldn’t add any unnecessary calls, it just wouldn’t check the headers for it. |
Why
Currently uses up 2 API calls for each call. This adds a simple option to the constructor to ignore it:
A more elegant solution needs to be put in place for rate limit checking (something involving the API response headers), so this will be removed in the future.
This change addresses the need by:
Also