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

labels: retry request for PR files if GitHub request fails #130

Merged
merged 1 commit into from
Apr 11, 2017

Conversation

phillipj
Copy link
Member

Trying to get around some intermittent GitHub API 404 failures, probably due to a race condition where the API hasn't yet got to update its PR cache or something similar.

Refs #88 (comment)

Trying to get around some intermittent GitHub API 404 failures, probably
due to a race condition where the API hasn't yet got to update its PR cache
or something similar.
@mscdex
Copy link
Contributor

mscdex commented Mar 23, 2017

IMHO we shouldn't pull in async for something that could be accomplished with setTimeout() and a counter.

@phillipj
Copy link
Member Author

See your point in async being overkill for one function. Though writing something similar ourself should ideally include tests for that particular code, as well as maintaining it over time. Since this is strictly backend and not frontend where we'd consider JS-bundle sizes, I personally see the benefits greater than the downsides in including a battle tested package like async.

Got any thoughts on where the threshold should be for knowing when to use a 3rd party package or write it ourself?

P.S. I really had two async use cases in mind when open this PR -> #128 / 458fdc6#diff-3b3f42a9fe8032799be4775b56e049dfR14 as well.

Fishrock123

This comment was marked as off-topic.

@phillipj phillipj merged commit 31d0d2e into nodejs:master Apr 11, 2017
@phillipj phillipj deleted the retry-on-gh-failures branch April 11, 2017 20:42
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