Skip to content

Handle Github rate-limiting response in download-experimental-build.js #24573

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

Merged
merged 1 commit into from
May 18, 2022
Merged

Handle Github rate-limiting response in download-experimental-build.js #24573

merged 1 commit into from
May 18, 2022

Conversation

blakef
Copy link
Contributor

@blakef blakef commented May 18, 2022

Summary

When running the download-experimental-build.js, fetching the build-id for the commit uses a Github API. The code currently assumes that failure is due to the commit not existing. I hit rate limiting (working over a VPN), but the error reported the commit could not be found for main.

This change surfaces the API error message should it not be a 404, in which case the previous error message worked well:

rate-limit

How did you test this change?

Ran the change while I was being rate limited:

❯ ./download-experimental-build.js --commit=main
Error: API rate limit exceeded for XXX.XXX.XXX.XXX. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)
        https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting

Confirmed that I was actually being rate limited:

❯ curl -I https://github.com/api/users/octocat 2>&1 | grep remaining
x-ratelimit-remaining: 0

Ran the change when I wasn't being rate limited:

❯ ./download-experimental-build.js --commit=foobar
Error: Could not find commit for: foobar

Ran the change on my local machine while I was being rate limited. Waited for the

@sizebot
Copy link

sizebot commented May 18, 2022

Comparing: 357a613...f5611e9

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 131.49 kB 131.49 kB = 42.15 kB 42.15 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 136.73 kB 136.73 kB = 43.69 kB 43.69 kB
facebook-www/ReactDOM-prod.classic.js = 441.13 kB 441.13 kB = 80.41 kB 80.41 kB
facebook-www/ReactDOM-prod.modern.js = 426.34 kB 426.34 kB = 78.23 kB 78.23 kB
facebook-www/ReactDOMForked-prod.classic.js = 441.13 kB 441.13 kB = 80.42 kB 80.42 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against f5611e9

Make the error messages clearer when the API doesn't respond with 200.
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

Seems fine, thanks!

@acdlite acdlite merged commit 835d9c9 into facebook:main May 18, 2022
@blakef blakef deleted the github_error_message branch May 18, 2022 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants