-
Notifications
You must be signed in to change notification settings - Fork 86
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(client): add support for global retry configuration #380
Conversation
Soooo, @redonkulus , would it be possible to check this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with a global config. Added one comment. We should document this in the readme too.
statsCollector: options.statsCollector, | ||
unsafeAllowRetry: Boolean(options.unsafeAllowRetry), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is called unsafe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the unsafeAllowRetry
allows POST requests to be retried. In many cases, however, one would not like to have this feature enabled (since it could lead to duplicated transactions in the back-end). For me, a better name would be retryOnPost
or something like this so it's clear what is happening (or, perhaps, removing this flag completely since, right now, retry is disabled by default).
Regarding the documentation, it's already in the readme:
Line 477 in 081b3c7
**Note:** Fetchr doesn't retry POST requests for safety reasons. You can enable retries for POST requests by setting the `unsafeAllowRetry` property to `true`: |
@redonkulus @pablopalacios : Thanks to both of you for keeping this project alive and kicking. It's total bliss to use it. |
@konarkmodi thanks for the kind words. @pablopalacios do you want to update the readme, then we can get this merged? |
e973ba2
to
b87fbde
Compare
b87fbde
to
29f94aa
Compare
@redonkulus I thought it was already there. In any case, I updated the documentation to make it clearer (hopefully :D). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the docs update, its clearer now.
@redonkulus we were only able to set retry configuration per request. Now, it's possible to set some (or all) configuration globally:
This avoids repeating some configuration that (at least in our case) are always the same.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.