[RFC] ApplicationService mounting#36477
Conversation
|
Pinging @elastic/kibana-platform |
2eda4ae to
fd609b2
Compare
|
I've requested review on this from a few people I know who are interested / may have concerns about how the ApplicationService works. If you know of anyone else, please add them as a reviewer to this PR. |
kobelb
left a comment
There was a problem hiding this comment.
This is potentially out of scope for this RFC, if so, please feel free to ignore me. The "kibana" application currently consists of: Discover, Visualize, Dashboard, Home, Management, etc. and I believe as part of the effort in moving to the new platform, we'd like to move away from this approach. In doing so, are we concerned about breaking existing URLs, and if so, should anything change with the current proposal to accommodate for "redirects"?
lukeelmers
left a comment
There was a problem hiding this comment.
Just did an initial read, will give it more thought and come back with additional comments if anything comes to mind.
I think @kobelb makes a good point about redirects, that might be a valuable functionality to have (or to introduce in the near term as an enhancement). The current idea for dealing with the shared kibana app routes is to separate those apps out into separate plugins, and (temporarily) maintain an empty “wrapper” kibana app that has routes mounting those other apps. Part of the concern is breaking existing routes, and part is around performance (navigating between kibana routes currently doesn’t require a refresh)
Not sure exactly how that would look given the proposed design here, but @joshdover would be curious to hear your thoughts.
|
I really like this, but feel this and the embeddable api effort should work in tandem to ensure that embeddable are a top level concept within the platform for DX reasons |
I actually don't think these overlap, nor necessarily that embeddables should be a top level concept. They aren't going to be in core, at least not in phase 1, but a plugin. A Kibana developer could create a plugin and not use embeddables. I think maybe @mattapperson you are primarily concerned with how embeddables will work with the URL, e.g. making it easy to preserve changes. I do think the embeddables effort could improve this but I still don't think it needs to be a part of the platform effort, or core, at least not now. The way I see us taking these baby steps is starting by keeping embeddables as a separate plugin, and we can always migrate to core later. More difficult to take something we decided to be in core and rip it out. |
cjcenizal
left a comment
There was a problem hiding this comment.
I reviewed the pattern used by the ES UI team for mounting and unmounting the Index Management, CCR, and Remote Clusters apps, and this looks like it will be a great replacement. Nice work!
|
For anyone following, I'm currently working on polishing up the Handler RFC (#36509) which impacts the design of the ApplicationService before moving forward with this RFC. |
fd609b2 to
e3bf7f7
Compare
I'm treating this as a separate issue for the time being. Plugins that are migrating should consider setting up routes that serve as simple "redirect" apps until 8.0. As pieces of the AppService land, full-page refreshes will go away so the performance concern is negligible. |
|
This RFC is now in the final comment period. If you have any comments about a fundamental problem with this proposal, make them now. Otherwise this will be accepted on Monday, June 17th. |
0c09e4a to
523ac20
Compare
There was a problem hiding this comment.
This isn't necessarily critical for this RFC, but in the spirit of the RFC demonstrating real world examples, I figured I'd mention it. I leave it to you whether you change that here or not.
I don't think the application mounting function should get access to the start contracts of plugins. In fact, I suspect the application mount handler would get access to more things than would be exposed via plugin start. We expose basically everything we can to the plugin lifecycle hooks today, but with the introduction of the handler proposal we should be able to scale back the capabilities exposed at a top level in favor of those exposed directly to handlers.
There was a problem hiding this comment.
Agreed, many of these services do not make sense outside the scope of rendering (for example, overlays). I was expecting to move these out of CoreStart as well. I'll go ahead and update these to not reference *Start types since those will not be present in the future, making this RFC confusing.
523ac20 to
d0c318f
Compare
| from `ui/chrome` | ||
| - Making Kibana a single page application may lead to problems if applications | ||
| do not clean themselves up properly when unmounted | ||
| - Application `mount` functions will have access to *setup* via the closure. We |
There was a problem hiding this comment.
We might be able to work around this by using this instead:
class Plugin {
start(core) {
core.application.register({
id: 'foo',
load: () => import(...)
})
}
}We could even use eslint to validate that this function is doing nothing other than importing the module and returning the promise (maybe we would want a more unique/identifiable name to assist with this). The application service would then need to call the mount/renderApp function exported by the app module, and since the module is completely outside the scope of the closure it can't reuse the start context.
There was a problem hiding this comment.
I considered this approach but some downsides to this:
- Complicates the bundling requirements for 3rd party plugins.
- Forces an unnecessary additional request overhead if your app is tiny.
As the API surface of setup shrinks to only registration APIs, I'm less concerned about this closure issue. I'm going to proceed as is, but open to update this RFC before 8.0 if we start running into issues here.
Summary
Related to #18843
View Rendered RFC
[skip-ci]