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 access to Provider client from the provider #413

Closed
wants to merge 1 commit into from

Conversation

nathenapse
Copy link

This is helpful when we want to call Exchange with the client we provided to the Provider

This is helpful when we want to call Exchange with the client we
provided to the Provider
@ericchiang
Copy link
Collaborator

I don't see a strong reason the Provider struct should allow accessing it's internal fields like this. If you need to use the same http.Client as the Provider, use the http.Client you passed to the Provider in the first place?

@nathenapse
Copy link
Author

Let me explain my reasoning behind it. When passing the *http.client to the NewProvider() I expected it to also affect the Exchange() but that was not the case. So if we also need to pass the *http.client to the Exchange() it would be better if we override the Exchange() method to pass the http.client found in Provider. Or have a way to access it and pass it to Exchange(). Clearly I chose the second method but the first option is also something I can work on. What do you think?

@ericchiang
Copy link
Collaborator

Ah. Would you take a look at my comment here #402 (comment). That goes over some other unexpected behavior with passing config through contexts.

This is unfortunate, but I think the answer remains that you should hold on to the http.Client configuration and reuse it between calls. Having multiple ways to get at the client doesn't seem like it's adding much at the cost of more API surface

@nathenapse
Copy link
Author

That means if we have a custom *http.client we have to remember to pass it everytime?
Wouldn't that be prone to errors? and sometimes tls configurations are production specific.

@ericchiang
Copy link
Collaborator

This is the case for all values passed as context keys. If you use a context, it needs to be configured correctly before passing it to your method. E.g. if you use golang.org/x/oauth2, you also need to do the same.

Again, as I mentioned in #402, if we're going to reduce confusion, we should allow HTTP client configuration without using contexts. I don't think this PR is the correct way to improve things, so going to close out

@ericchiang ericchiang closed this Feb 23, 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.

2 participants