Add optional timeoutMs parameter to makeApiClient #2430
Conversation
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jannikschaper The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @jannikschaper! |
k8s-ci-robot
added
size/M
|
|
||
| const resultPromise = fetch(request.getUrl(), { | ||
| method: method, | ||
| body: body as any, |
There was a problem hiding this comment.
| body: body as any, | |
| body: body as BodyInit, |
If we do this typecast we should at least cast into the correct type.
There was a problem hiding this comment.
Could do that. Would lose the pure symmetry with the original code then. If you're aware that I deliberately copied the send implementation verbatim, trying to stay close to the original code, but still prefer to have the more correct type cast, I can do that!
| binary: () => resp.buffer(), | ||
| }; | ||
| return new ResponseContext(resp.status, headers, body); | ||
| }); |
There was a problem hiding this comment.
Not 100% sure but do we need a catch here as well or will this be catched by the client code?
There was a problem hiding this comment.
This code, as is (without a catch) should behave identically to the status quo. I copied the send implementation verbatim from ./gen/http/isomorphic-fetch.ts (as noted above); only adding the timeout: options?.timeoutMs, line. Since the original code didn't need a catch, I argue this code must not need it either
| // This HTTP API implementation is a copy of ./gen/http/isomorphic-fetch.ts, | ||
| // extended with the timeout pass-through code. | ||
| httpApi: { | ||
| send(request: RequestContext): Observable<ResponseContext> { |
There was a problem hiding this comment.
I don't love pulling this full implementation into this library. Is there an easier way to inject the timeout?
There was a problem hiding this comment.
I thought that too, it would be much nicer to inject the extra field without having to rip out the entire function. But I believe there's no other way. The auto-generated code passes the configuration object (without any timeout value, not even passed through) directly into fetch. Not even a dependency-injected fetch or anything that might be overwriteable, but a fetch directly imported from node-fetch.
Expand code screenshot
Maybe it's still possible to reach into the code via JavaScript black magic to overwrite the fetch implementation and amend the timeout parameter, but I have a hunch such code will be even uglier than copying the 22 lines of code
There was a problem hiding this comment.
What do you think about changing the code generator itself? They're usually pretty good about merging changes quickly into the generator.
Fixes #1613
I followed @brendandburns comments from the linked issue: