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

Allow to use a custom *http.Client #74

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

jybp
Copy link
Contributor

@jybp jybp commented Dec 9, 2023

Closes #71

I was very tempted to remove the github.com/hashicorp/go-retryablehttp dependency. There's no reason it should be used by default.
It's not the responsibility of such library to handle HTTP retries, it should be left at the discretion of consumers.
If I get a go-ahead, I will create a new PR to remove it altogether.

Could a new tag be introduced in this repository once merged? v0.2.0 should be fine since it's still in the v0 realm.

@jybp jybp marked this pull request as ready for review December 9, 2023 04:20
@jybp
Copy link
Contributor Author

jybp commented Dec 9, 2023

cc @slowbackspace

BTW some tests seem broken. I can rebase on #70 if you wish.

All tests pass except:

--- FAIL: TestResourceAddressTransactions (0.04s)
--- FAIL: TestResourceAssetTransactionIntegration (0.05s)
--- FAIL: TestResourceAssetddressesIntegration (0.04s)
--- FAIL: TestEpochStakeDistributionByPoolIntegration (0.08s)
--- FAIL: TestTransactionIntegration (0.04s)
--- FAIL: TestTransactionUTXOs (0.05s)
--- FAIL: TestResourceMetricsEndpointsIntegration (0.07s)
--- FAIL: TestResourceMetricsIntegration (0.10s)
--- FAIL: TestResourceInfoIntegration (0.07s)

@slowbackspace
Copy link
Collaborator

Thanks for the PR @jybp! Don't worry about rebasing right now. I would like to merge this first and then I'll rebase the huge PR.

As for the go-retryablehttp I think that from the start, one of the requirements for the SDK was to automatically handle retries so users don't have to. We also have it in Node.js SDK https://github.com/blockfrost/blockfrost-js/blob/master/src/utils/index.ts#L65-L86. So that's probably why it is there.

Though, it would be great if user could be opt-out of this behaviour, maybe it could even be configurable just like in Node.js SDK (we allow passing retrySettings object as param while creating class instance). Do I understand correctly that with your implementation user can basically opt out of retry behaviour by passing its own httpClient and that the default client still does retry failed reqs?

@jybp
Copy link
Contributor Author

jybp commented Dec 11, 2023

Do I understand correctly that with your implementation user can basically opt out of retry behaviour by passing its own httpClient

Yes you are correct. This PR allows to implicitly opt-out of that behaviour by setting a options.Client.

and that the default client still does retry failed reqs?

Yes that's also correct. It still uses the default configuration by default (when options.Client == nil).
It's now using a standard *http.Client to make the codebase simpler.
See: https://github.com/hashicorp/go-retryablehttp#getting-a-stdlib-httpclient-with-retries.
Nothing changes.

@jybp
Copy link
Contributor Author

jybp commented Dec 11, 2023

Don't worry about rebasing right now. I would like to merge this first and then I'll rebase the huge PR.

Cool! Whenever that's possible, would also be great to create a new tagged version. That way we can easily update the dependency.

Copy link
Collaborator

@slowbackspace slowbackspace left a comment

Choose a reason for hiding this comment

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

lgtm

@slowbackspace slowbackspace merged commit dd587e8 into blockfrost:master Dec 14, 2023
1 check failed
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.

Support passing a custom *http.Client
2 participants