Skip to content
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

[RFC] ApplicationService mounting #36477

Merged
merged 5 commits into from
Jun 18, 2019
Merged

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented May 10, 2019

Summary

Related to #18843

View Rendered RFC

[skip-ci]

@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform RFC labels May 10, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@joshdover joshdover requested review from lukeelmers, cjcenizal and a team May 10, 2019 21:24
@joshdover joshdover marked this pull request as ready for review May 10, 2019 21:24
@joshdover
Copy link
Contributor Author

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.

@epixa epixa mentioned this pull request May 10, 2019
10 tasks
@jinmu03 jinmu03 requested review from kobelb and peteharverson May 10, 2019 22:38
Copy link
Contributor

@kobelb kobelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
@epixa epixa self-requested a review May 11, 2019 00:06
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
@epixa epixa mentioned this pull request May 11, 2019
@mattapperson
Copy link
Contributor

mattapperson commented May 17, 2019

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

rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
rfcs/text/0003_application_service_mounting.md Outdated Show resolved Hide resolved
@stacey-gammon
Copy link
Contributor

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.

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@jinmu03 jinmu03 requested review from spalger and azasypkin May 24, 2019 22:47
@joshdover
Copy link
Contributor Author

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"?

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)

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.

@joshdover joshdover added the non-issue Indicates to automation that a pull request should not appear in the release notes label Jun 5, 2019
@joshdover joshdover added the RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted label Jun 13, 2019
@joshdover
Copy link
Contributor Author

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.

Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit.

rfcs/text/0004_application_service_mounting.md Outdated Show resolved Hide resolved
@joshdover joshdover added the release_note:skip Skip the PR/issue when compiling release notes label Jun 13, 2019
@joshdover joshdover force-pushed the app-service/rfc branch 2 times, most recently from 0c09e4a to 523ac20 Compare June 14, 2019 22:54
/** A context type that implements the Handler Context pattern from RFC-0003 */
export interface MountContext {
core: {
chrome: ChromeStart;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Copy link
Contributor

@spalger spalger Jun 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this approach but some downsides to this:

  1. Complicates the bundling requirements for 3rd party plugins.
  2. 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.

@joshdover joshdover merged commit b842395 into elastic:master Jun 18, 2019
@joshdover joshdover deleted the app-service/rfc branch June 18, 2019 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes RFC/final-comment-period If no concerns are raised in 3 business days this RFC will be accepted RFC Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet