-
-
Notifications
You must be signed in to change notification settings - Fork 583
Handle GitHub API rate limits in script #2471
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
Conversation
|
✅ Deploy Preview for openapi-ts ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
size-limit report 📦
|
fd68e4c
to
a7660f2
Compare
a7660f2
to
493eee1
Compare
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.
TY for looking into this.
I've left some comments. All optional. The change is definitely an overall improvement, so we should probably ship it :)
|
||
if (process.env.GITHUB_TOKEN) { | ||
request.headers.set('Authorization', `Bearer ${process.env.GITHUB_TOKEN}`) | ||
} |
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.
Suggestion to make this non-optional once it is set as secret (or even set the secret right away, so you don't need to touch it anymore, up to you).
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.
I think leaving this conditional might make sense just in case, since the API will remain accessible even if we don't have a token(albeit at a more constrained rate limit).
|
||
const res = await fetch(request); | ||
|
||
if (!res.ok) { |
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.
Only do the rate-limit retry if we get a 429? (res.status === 429
)?
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.
Normally I'd concur, but after reading the docs on handling rate limit errors appropriately, I think retrying in this manner should cover us.
} | ||
|
||
main(); | ||
if (import.meta.main) { |
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.
Heh :) First time I see this in JS/TS.
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.
😄 it's the ESM equivalent of require.main === module
33948b7
to
413ca37
Compare
Changes
Updated
fetchUserInfo
to appropriately handle GitHub API rate limits.Notes
This should resolve issues we've encountered in Netlify builds (example).
I'm also going to add a token to our Netlify env vars to help increase our rate limits.