feat: introduce base client that utilizes context #196
Conversation
|
Needs to be landed before twilio/twilio-oai-generator#301 tests will work. |
|
|
||
| func (w wrapperClient) SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values, | ||
| headers map[string]interface{}) (*http.Response, error) { | ||
| return w.SendRequest(method, rawURL, data, headers) |
There was a problem hiding this comment.
Should this be calling SendRequestWithCtx instead? Otherwise the context is swallowed.
There was a problem hiding this comment.
Sorry, there really should've been a comment over wrapperClient to make this more explicit.
wrapperClient is wrapping the older BaseClient implementation. So this method is the new interface passing though to the context-free client.
This enables the SDK to use the BaseClientWithCtx interface everywhere since we can transform the older BaseClient
There was a problem hiding this comment.
Ah, understanding better now the intent. I'm thinking it would be a little easier to understand the implications by actually having the wrapperClient send through the provided context (which is what I was expecting in the initial review).
wrapping/upgrading is adding the ability to an existing client/handler to pass a context through (by adding an implementation for SendRequestWithCtx to it). So it no longer becomes a noop that swallows the context. That way if I wanted to take an existing integration and start adding context to API calls, all I'd need to do is switch to use the context-based version of the API method (no initialization changes needed to the client/service/handler).
Thoughts?
There was a problem hiding this comment.
So there are a few different ways for folks to instantiate a twiliogo client, in all but one the *WithCtx methods will operate as expected. There's just one scenario where it will be misleading to the integration developer, but I think it's the most sophisticated setup so there's the highest likelihood they will identify this.
Integrations where the client will "just work" with *WithCtx methods:
- generic
twiliogo.NewRestClientis upgraded automatically client.Clientthat provides a*http.Clientwill automatically use the context
The only scenario where a context may be provided but not used is when the integration is using client.RequestHandler and has provided a custom BaseClient. If this is the case, there's no way for the library to pass the context in unless we introduce breaking changes, since the specified client by the integration (which is opaque to the SDK) has no method available to ingest a context.
There was a problem hiding this comment.
Ah, now I I understand. Thanks for bearing with me.
|
|
||
| func (w wrapperClient) SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values, | ||
| headers map[string]interface{}) (*http.Response, error) { | ||
| return w.SendRequest(method, rawURL, data, headers) |
There was a problem hiding this comment.
Sorry, there really should've been a comment over wrapperClient to make this more explicit.
wrapperClient is wrapping the older BaseClient implementation. So this method is the new interface passing though to the context-free client.
This enables the SDK to use the BaseClientWithCtx interface everywhere since we can transform the older BaseClient
natebrennand
force-pushed
the
natebrennand/add-context-methods-to-base-client
branch
from
1385df6
to
fa94b6c
Compare
|
|
||
| func (w wrapperClient) SendRequestWithCtx(ctx context.Context, method string, rawURL string, data url.Values, | ||
| headers map[string]interface{}) (*http.Response, error) { | ||
| return w.SendRequest(method, rawURL, data, headers) |
There was a problem hiding this comment.
Ah, now I I understand. Thanks for bearing with me.
This reverts commit b61e334.
Works on #98
This PR introduces alternative base client interfaces,
BaseClientWithCtxandRequestHandlerWithCtx, that allowcontext.Contextobjects to be provided to every API request. This will allow the Twilio SDK to support context cancellations, and for custom HTTP clients to access the context while executing the request.Checklist