Add asResponse option to HttpService methods #52434
Conversation
joshdover
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
|
|
||
| import { IHttpResponse } from './types'; | ||
|
|
||
| export class HttpResponse<TResponseBody = any> implements IHttpResponse<TResponseBody> { |
There was a problem hiding this comment.
This object does nothing useful now, but will be extended in #43970
joshdover
force-pushed
the
np/http-asresponse
branch
from
ac584c7
to
b67f806
Compare
joshdover
force-pushed
the
np/http-asresponse
branch
from
b67f806
to
02a5006
Compare
| const capabilities = await http.post(url, { | ||
| const capabilities = await http.post<Capabilities>(url, { |
There was a problem hiding this comment.
👍 I think this is definitely more developer friendly.
| public intercept(interceptor: HttpInterceptor) { | ||
| this.interceptors.add(interceptor); | ||
| return () => this.interceptors.delete(interceptor); | ||
| } |
There was a problem hiding this comment.
NIT: addInterceptor is probably more appropriate. I'm expecting an intercept method on a fetch service to actually intercept a request.
There was a problem hiding this comment.
Agreed. This is named as such to be compatible with the ui/kfetch module as a drop in replacement. However, I think we could change this one easily since it's only used by a handful of plugins. Let me do that in a separate PR to keep the potential impact of this initial refactor minimal.
x-pack/legacy/plugins/lens/public/indexpattern_plugin/loader.test.ts
Outdated
Show resolved
Hide resolved
| return () => this.interceptors.delete(interceptor); | ||
| } | ||
|
|
||
| public removeAllInterceptors() { |
There was a problem hiding this comment.
Do we still need it? I suspect it's used in tests only. I'd prefer not to make it a part of the public API and remove from http_service contract. Not a blocker for the current PR.
There was a problem hiding this comment.
Correct, it's only used by tests at this time. I'd like to move the detailed interceptor tests out of the overall HttpService tests and into a test that only exercises this class. At that point, we can remove this completely from the public API.
However, I wanted to make sure not to change the tests too much in this initial refactor so as to avoid regressions.
There was a problem hiding this comment.
However, I wanted to make sure not to change the tests too much in this initial refactor so as to avoid regressions.
💯
joshdover
force-pushed
the
np/http-asresponse
branch
from
02a5006
to
1afa086
Compare
joshdover
force-pushed
the
np/http-asresponse
branch
from
51b855a
to
55bc0e1
Compare
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
This refactors the client-side HTTP service with a few minor changes:
asResponseoption to return a more detailed response object rather than just the response body.This refactoring does not change any tests in order to be sure that the functionality was not broken. In the future, tests could be broken out for different parts of the functionality (interceptors, fetching, etc.).
In order to support the
asResponseoption without breaking the API, I had to overload theHttpHandlertype. This causes some unexpected type errors with Jest mocks as it does not recognize the return type can be two different types. If anyone has an ideas on how to fix this, please let me know, but it does not appear to be supported well.This is not so much a problem for any code that uses the built in mocks that Core provides. I only had to manually cast in a few spots where custom mocks were used.
This change paves the way for #43970 where we can add methods to the
IHttpResponseinterface for adding Kibana-specific functionality to responses.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers