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

feat: add exponential backoff and retries for push/send #52

Merged
merged 4 commits into from
Oct 18, 2022

Conversation

wschurman
Copy link
Member

Why

We'll be adding rate limiting to the push/send API soon, so before that it'd be good to have this library handle 429s gracefully.

How

Add promise-retry with 2 retries and exponential backoff.

Test Plan

Run unit test.

@codecov
Copy link

codecov bot commented Sep 29, 2022

Codecov Report

Merging #52 (d1dac81) into master (0205b91) will decrease coverage by 0.57%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #52      +/-   ##
==========================================
- Coverage   99.35%   98.77%   -0.58%     
==========================================
  Files           1        1              
  Lines         154      163       +9     
  Branches       28       29       +1     
==========================================
+ Hits          153      161       +8     
  Misses          1        1              
- Partials        0        1       +1     
Flag Coverage Δ
node_10_x 98.77% <100.00%> (-0.58%) ⬇️
node_12_x 98.77% <100.00%> (-0.58%) ⬇️
node_14_x 98.77% <100.00%> (-0.58%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ExpoClient.ts 98.77% <100.00%> (-0.58%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -6,21 +6,6 @@ afterEach(() => {
(fetch as any).reset();
});

test('limits the number of concurrent requests', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

The addition of promiseRetry means that the promiseLimit is no longer the first awaited call, so this test is no longer reliable.

src/__tests__/ExpoClient-test.ts Outdated Show resolved Hide resolved
src/__tests__/ExpoClient-test.ts Outdated Show resolved Hide resolved
src/__tests__/ExpoClient-test.ts Outdated Show resolved Hide resolved
src/ExpoClient.ts Outdated Show resolved Hide resolved
@wschurman wschurman requested a review from ide October 3, 2022 17:59
@wschurman
Copy link
Member Author

Noted inline about why promiseRetry is difficult (maybe impossible) to test in an asynchronous context with jest mock timers. I had a few other ideas, but it seems unlikely that any of them will pan out:

  • mock setTimeout with custom implementation (don't think this is possible?)

@wschurman
Copy link
Member Author

I was unable to get the mock timers to work unfortunately. That being said, these seem to finish in under the default timeout, so I'm not too worried.

@ide
Copy link
Member

ide commented Oct 13, 2022

My primary concern is human time, not CI time. The unit tests are actually "fast, reliable tests" that should give feedback as fast as possible, so we never want to wait for timers with them. Another reasonable solution would be to make it so that the backoff in tests is always 0ms.

@wschurman wschurman merged commit 4dfde3b into master Oct 18, 2022
@wschurman wschurman deleted the @wschurman/retries branch October 18, 2022 22:14
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