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

add: retriesCount property to the callback #344

Merged
merged 2 commits into from
Jan 9, 2024

Conversation

MikePresman
Copy link
Contributor

Initially the custom retry handler was put in place to give the client true freedom of handling retries. I quickly saw this comes at a cost of making the simple mistake (which I had done !) of infinitely retrying.

Now there are two approaches to this.

  1. Respect the underlying spirit of custom retries and having the client handle the retries (which is what this PR is proposing by passing back the retriesCount).
  2. Limiting it on the library and not the client level with a conditional.

I have no belief that one is greater than the other but would like to respect the spirit of the initial PR.

So whats the point of this PR at all. To avoid hardcoding and passing values. This is simply cleaner.

Checklist

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

I dont see any harm in the implementation.

Btw. It is very clean PR.Thank you for your effort.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 1cc42b2 into fastify:master Jan 9, 2024
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