[APM] Shim new platform #34531
[APM] Shim new platform #34531
Conversation
sorenlouv
force-pushed
the
apm-shim-new-platform
branch
from
7b30d19
to
04a296a
Compare
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
| import { CoreSetup } from 'src/core/public'; | ||
|
|
||
| export let basePath = { | ||
| addToPath: path => `/kbn-basepath${path}` |
There was a problem hiding this comment.
Do we want this function to have this default?
| // Also, non-react functions will not be able to access context - therefore singletons might not be the worst idea | ||
| export function _setCoreDependencies(core: CoreSetup) { | ||
| basePath = core.basePath; | ||
| } |
There was a problem hiding this comment.
In the long run, I wonder if it'd be "more React-like" for us to work on converting imperative helper methods into declarative components that would then be able to use hooks to get the values from context.
Out of curiosity, how many imperative helpers do we currently have, and how many of them need access to these core deps? I imagine it's mostly the url helper stuff needing basePath, etc.
There was a problem hiding this comment.
The more I think about this, the more I think the core services should work the same as location -- seems weird to have location be accessible via context + hook (which also means you can't directly access it from imperative code) and then have these values cached in a singleton. Curious how you all are feeling about it, though?
There was a problem hiding this comment.
I think it makes sense to pass stateful objects (like location) down via context. But I find it a little odd to do the same with methods. But that's probably subjective.
core services should work the same as location
So should we use the singleton approach for everything, or React Context? If we use the latter, how should callApi access kfetch?
There was a problem hiding this comment.
Hm, yeah. My first thought is that if kfetch is no longer globally stateful (i.e. the platform isn't setting things on it before providing it to the plugin), then it should just be a separate package we import via npm/yarn. Anything else that's just a stateless method should work the same way, imo, then the rest would be handled via React context. I think we'll need to talk to the platform folks about this...
@weltenwort what is your team thinking wrt to how to handle things like this in the new platform?
There was a problem hiding this comment.
I'd definitely importing it statically if that's an option!
There was a problem hiding this comment.
While I can't speak for the whole infra monitoring team, I've started to write hooks for such external services (e.g. useKibanaUiSettings('mySetting')) in https://github.com/elastic/kibana/pull/34615/files#diff-5c1aef9186a64f50ff1ef32ee57a92ccR29) to abstract these platform details away.
These hooks can then either get the reference from the context if it needs to be injected or statically import it if possible.
There was a problem hiding this comment.
That way the plugin dependencies could be injected with one platformDependencies provider, but fine-grained hooks hide the fact that we're extracting the individual component from one larger object. Not sure if I'm making sense, let me know if you want to discuss it in person after I had my morning tea. 😉
sorenlouv
force-pushed
the
apm-shim-new-platform
branch
from
f696df1
to
23bb71d
Compare
sorenlouv
force-pushed
the
apm-shim-new-platform
branch
from
23bb71d
to
d3dc466
Compare
💚 Build Succeeded |
Closes #34533
Following the steps in the Migration guide to add shims for new platform