-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[APM] Replace ui/kfetch with core.http #47635
[APM] Replace ui/kfetch with core.http #47635
Conversation
💚 Build Succeeded |
443205d
to
ce00084
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit on the docs for HttpFetchError
but otherwise Platform changes LGTM
src/core/public/public.api.md
Outdated
@@ -435,6 +433,19 @@ export interface HttpErrorResponse { | |||
response?: Response; | |||
} | |||
|
|||
// Warning: (ae-missing-release-tag) "HttpFetchError" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an block comment above the definition of HttpFetchError
like:
/*
* @public
*/
That will make this warning go away and mark the type as part of the public API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. That comment format specifically seemingly did not work, but /** @public */
did.
💚 Build Succeeded |
💔 Build Failed |
x-pack/legacy/plugins/apm/public/components/app/ServiceNodeOverview/index.tsx
Show resolved
Hide resolved
5c48d93
to
bec10e2
Compare
💚 Build Succeeded |
docs/development/core/public/kibana-plugin-public.httpfetcherror._constructor_.md
Show resolved
Hide resolved
x-pack/legacy/plugins/apm/public/components/app/ServiceNodeOverview/index.tsx
Show resolved
Hide resolved
environments: [] | ||
} | ||
] | ||
} as never); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why never
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason TS expects never
as a type. I'm probably doing something wrong, but I couldn't figure it out and didn't want to spend too much time fixing types for a test. Do you happen to know what's going on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about:
function getCoreMock(res: HttpBody) {
return ({
http: {
basePath: {
prepend: (path: string) => `/basepath${path}`
},
get: jest.fn(res)
},
notifications: {
toasts: {
addWarning: () => {}
}
}
} as unknown) as LegacyCoreStart & {
http: { get: jest.SpyInstance<LegacyCoreStart['http']['get'], any> };
};
}
This way the spies are not modified, and the compile error disappears
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my type was just wrong. Simplified to { get: jest.Mock<any,any> }
and that works as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
bec10e2
to
20c93f2
Compare
ed336a1
to
9aad918
Compare
@sqren I've merged in your changes and did some rudimentary checks that the agent configuration things that you merged in still working, but can you do some surface checks as well just to be sure? |
💚 Build Succeeded |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Did some manual testing of agent configs and still works as expected 👍
Closes #46548.
This replaces the static import
ui/kfetch
with the injectedhttp
module fromcore
. To accommodate this, several changes were made:callApi
's signature from( options )
to( http, options )
.useCallApi
hook that usesuseKibanaCore
to gethttp
from context.callApmApi
to a factory function that needshttp
. I could not get a similar(http, options)
signature to work because I could not extract the type ofoptions
fromcallApmApi
, presumably because it is a generic.useCallApmApi
hook, similar touseCallApi
.callApmApi
as the first argument to the callback passed touseFetcher
. I figured this is a little better than using a separate hook because it a) removes the need for an extra import/hook b) avoids the dependency dance foruseFetcher
.useFetcher
inuseApmIndexPattern