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

Refactor Retry #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor Retry #13

wants to merge 2 commits into from

Conversation

axdg
Copy link
Contributor

@axdg axdg commented Jun 17, 2019

WIP

@elyobo, the below signature is my suggested simplification of the API that we've incremented toward.

fn is wrapped as per the logic given in curve. Curve is expected to be a callback which will be called at each failed iteration with err (the error thrown by the wrapped function) and count which is the count of total attempts to resolve - it should either throw (should no more retries be required) or return an integer dictating the number of milliseconds to sleep before re-attempting resolution.

So, for example, the wrapped function below would retry a maximum of 12 times with exponential (base 2) back off.

const curve = function (err, count) {
  if (count > 12) throw err
  return (2 ** pow) * 1000
}

const wrapped = retry(fn, curve)

We can overload the sig so that it can instead take an object containing options as a second argument, although this wouldn't help much with determining which errors we don't want to retry on (unless the generated function was wrapped again).

What do you think?

@axdg axdg requested a review from elyobo June 17, 2019 13:10
Copy link
Member

@elyobo elyobo left a comment

Choose a reason for hiding this comment

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

Seems like a cleaner API. It's still possible to opt out of retries, just have your curve throw if you want to stop retrying.

Breaking change.

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.

None yet

2 participants