blockfrost / blockfrost-go

Golang SDK for Blockfrost.io
Apache License 2.0
23 stars 9 forks source link

Allow to use a custom *http.Client #74

Closed jybp closed 9 months ago

jybp commented 9 months ago

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 commented 9 months ago

cc @slowbackspace

BTW some tests seem broken. I can rebase on https://github.com/blockfrost/blockfrost-go/pull/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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.