Add promise support for onRetryAttempt and shouldRetry

[atjeff]

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

• Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
• Ensure the tests and linter pass
• Code coverage does not decrease (if any source code was changed)
• Appropriate docs were updated (if necessary)

Fixes #144 🦕

[atjeff]

I didn't see any harm in allowing promises in onRetryAttempt, as we're just notifying the user we're retrying, do you think it should await the completion of the onRetryAttempt promise?

[bcoe]

This looks reasonable to me, but it would be good to also have @JustinBeckwith's thoughts.

Since retry is already async, I think you're right that this probably isn't a breaking change.

[bcoe]

is this Promise.resolve necessary, await false would just resolve false right?

[bcoe]

I'd be tempted to make this assertion something like:

accepts async onRetryAttempt handler

[atjeff]

Thanks for the review @bcoe! I was seeing a lot of tests failing without the resolve, I'll hopefully find some time to look into why tomorrow. I renamed the test

[atjeff]

Found any time @JustinBeckwith ?

[JustinBeckwith]

LGTM. Want to make sure @bcoe is happy too :)

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@f0539b7). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #223   +/-   ##
=========================================
  Coverage          ?   95.84%           
=========================================
  Files             ?        6           
  Lines             ?      602           
  Branches          ?       99           
=========================================
  Hits              ?      577           
  Misses            ?       24           
  Partials          ?        1

Impacted Files	Coverage Δ
src/retry.ts	98.68% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0539b7...dd3eead. Read the comment docs.

[bcoe]

Hey @atjeff testing out your branch, this approach worked great for me:

   // Determine if we should retry the request
   const shouldRetryFn = config.shouldRetry || shouldRetryRequest;
-  const shouldRetryFnPromise = Promise.resolve(shouldRetryFn(err));
-  if (!(await shouldRetryFnPromise)) {
+  if (!(await await shouldRetryFn(err))) {
     return {shouldRetry: false, config: err.config};
   }

What tests were failing for you prior to the resolving step? I wonder if it might have been something else biting you. If we can do without the Promise.resolve, I rather would 👌

[atjeff]

Yep, it was me not thinking to await twice. Woops! Pushing that change now.

[atjeff]

I'm not sure how I had failing tests, sorry about that. Awaiting shouldRetryFn(err) works fine. Pushed @bcoe

[atjeff]

Thanks for taking the time guys!

[wiktor-obrebski]

sorry I dig it back, but from the discussion I expected that retry will wait for both: onRetryAttempt and shouldRetry functions.
but I clearly see that the code wait only for shouldRetry function, but not for onRetryAttempt.

gaxios/src/retry.ts

Lines 75 to 77 in dd3eead

	if (config.onRetryAttempt) {
	config.onRetryAttempt(err);
	}

do I miss something?