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] Kibana Management Section Service #43631

Merged
merged 8 commits into from
Oct 3, 2019
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
332 changes: 332 additions & 0 deletions rfcs/text/0006_management_section_service.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,332 @@
- Start Date: 2019-08-20
- RFC PR: TBD
- Kibana Issue: [#43499](https://github.com/elastic/kibana/issues/43499)

# Summary
Management is one of the four primary "domains" covered by @elastic/kibana-app-arch (along with Data, Embeddables, and Visualizations). There are two main purposes for this service:

1. Own the management "framework" -- the UI that displays the management sidebar nav, the landing page, and handles rendering each of the sections
2. Expose a registry for other plugins to add their own registry sections to the UI and add nested links to them in the sidebar.

The purpose of this RFC is to consider item 2 above -- the service for registering sections to the nav & loading them up.

# Motivation

## Why now?
The main driver for considering this now is that the Management API moving to the new platform is going to block other teams from completing migration, so we need to have an answer to what the new platform version of the API looks like as soon as possible in `7.x`.

## Why not just keep the current API and redesign later?
The answer to that has to do with the items that are currently used in the management implementation which must be removed in order to migrate to NP: the framework currently registers a `uiExport`, and relies on `IndexedArray`, `uiRegistry`, and `ui/routes`.

This means that we will basically need to rebuild the service anyway in order to migrate to the new platform. So if we are going to invest that time, we might as well invest it in building the API the way we want it to be longer term, rather than creating more work for ourselves later.

## Technical goals
- Remove another usage of `IndexedArray` & `uiRegistry` (required for migration)
- Remove dependency on `ui/routes` (required for migration)
- Remove management section `uiExport` (required for migration)
- Simple API that is designed in keeping with new platform principles
- This includes being rendering-framework-agnostic... You should be able to build your management section UI however you'd like
- Clear separation of app/UI code and service code, even if both live within the same plugin
- Flexibility to potentially support alternate layouts in the future (see mockups in [reference section](#reference) below)

# Basic example
This API is influenced heavily by the [application service mounting RFC](https://github.com/elastic/kibana/blob/master/rfcs/text/0004_application_service_mounting.md). The intent is to make the experience consistent with that service; the Management section is basically one big app with a bunch of registered "subapps".

```ts
// my_plugin/public/plugin.ts

export class MyPlugin {
setup(core, { management }) {
// Registering a new app to a new section
const mySection = management.sections.register({
id: 'my-section',
title: 'My Main Section', // display name
order: 10,
euiIconType: 'iconName',
});
mySection.registerApp({
id: 'my-management-app',
title: 'My Management App', // display name
order: 20,
async mount(context, params) {
const { renderApp } = await import('./my-section');
return renderApp(context, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand the Management App will be written in React and all sections will be written in React, too. Maybe this should receive a React component, instead of unmounting React and mounting it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this - tighter integration with react vs staying platform agnostic.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tentatively proposed tighter integration with React here, too: #36425 (comment).

Practically speaking, anyone who builds an app will use EUI which is React. So I'm just not sure if it's helpful to design our interfaces without React in mind -- it might even end up hindering consumers.

Copy link
Contributor

@streamich streamich Aug 26, 2019

Choose a reason for hiding this comment

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

Anyone who does not want to use React can easily render using any other technology by wrapping it in a 5-line React component:

const AppSection: React.FC = () => {
  const ref = useRef();
  useLayoutEffect(() => {
    if (ref.current) renderWhateverOtherTechnology(ref.current);
  }, [ref.current]);
  return <div ref={ref} />;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Counterpoint: Any plugin that wants to register a section without using React would have to bundle React in their plugin just do these 5 lines.

I like the consistency this has with the ApplicationService, personally. It makes it one less thing to learn.

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the consistency this has with the ApplicationService, personally. It makes it one less thing to learn.

I agree with @joshdover on this... in all likelihood most people who are using this service are also going to be registering an app of some kind, which has an identical mount function. So they will already be familiar with this pattern and the couple of lines of code that are required to mount an app.

And registering a management section is really no different than registering an application. It's just an app nested inside of another app. The user clicks a nav link, a route is hit, and the app is loaded up and does whatever it wants.

Viewed that way, it makes sense that the experience would be consistent with what the core ApplicationService is providing. I don't think it would be a very good DX if someone found out that they could build their Kibana app however they want, unless they intend to register a management section, in which case they now have to bundle React with their plugin.

Copy link
Contributor

@streamich streamich Aug 28, 2019

Choose a reason for hiding this comment

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

Counterpoint: Any plugin that wants to register a section without using React would have to bundle React in their plugin just do these 5 lines.

Counter-counterpoint: React will be provided globally to everyone using Webpack Externals, as you cannot have more than one version of React in a single React tree.

Copy link
Contributor

@streamich streamich Aug 31, 2019

Choose a reason for hiding this comment

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

I suggest we look at this topic from technical perspective—instead of blindly copying what Core does for application mounting and "I like"—let's look at what we gain and lose by re-mounting React.

  • This React re-mounting adds unnecessary code: (1) un-mount React; (2) mount it back; (3) add tests for all of that—which needs to be written and then maintained. Then you need to (4) re-attach all your Error Boundaries; (5) re-attach all your React contexts (which I don't think is possible, in a non-trivial cases); (6) all your React Suspense handlers will be probably broken (I'm not aware if it is possible to re-connect them).
  • Re-mounting React does not make us less dependent on React: in both cases the Management app is written in React and all the sections are written in React. The only difference is, if we do React re-mounting, we need to add extra code, which makes the API harder to use and maintain (and is slower and breaks React Context and breaks React Error Boundaries and breaks React Suspense).
    • Management app and all its sections are written in React, by adding a few % of code that re-mounts React does not make us any less React dependent and those few % of code are not needed in the first place.
  • Un-mounting React and then mounting it back breaks React Context. (cc @ppisljar @stacey-gammon)
    • For example, if we ever had dynamic theming in EUI, where theme is stored in React context, in Kibana we would not be able to use it, at least in a sensible way, because of this React context breaking. (cc @elastic/kibana-design)
    • Similar goes for i18n—i18n context has to be re-attached if you want to use injectI18n() HOC. (cc @elastic/kibana-stack-services)
    • It is relevant not just for theming, but any context providers stop working when React is un-mounted.
    • I believe also React Error Boundaries and React Suspense stop working when you re-mount React.
  • Remounting React is slower performance-wise than just rendering a React component.
  • As mentioned above, React will anyways be bundled globally once for everyone, so a plugin will not need to "bundle React just to render a management section"; and if somebody wants to render a non-React section, it can be achieved by this 5-line function.

You can still have the "handler pattern", but in the idiomatic React way using render-props:

// section/index.tsx
export default () => <div>My Section!</div>;

// section/lazy.tsx
export default React.lazy(() => import('.'));

// plugin.tsx
import MySection from './section/lazy';

management.sections.registerLink('my-section', {
  id: 'my-link',
  order: 20,
  renderTitle: () => <>My Link<sup>beta</sup></>,
  renderDescription: () => <p>hello, not used in current UI, but possibly in future</p>,
  render: async (context, params) => <MySection context={context} params={params} />,
});
  • Above the mount function is simply renamed to render function and returns a React.ReactNode. And you can see it uses the standard React code-splitting pattern with React.lazy, so the section is loaded dynamically on-demand.
  • This way also allows you to use React.Suspense and gives ability to the section to control what is displayed while the section is being loaded or to suspend switching for some milliseconds from one section to another while the other section is being loaded; so that navigation from one section to another looks seamless to the user.
  • The above pattern is way more powerful, as you can see renderTitle and renderDescription easily allow you to render anything in those places.
  • It is also more powerful because the Management app can call the render function with updated (or the same) arguments as many times it wants and React will handle it gracefully by diff-ing the HTML and re-rendering only what needs to be re-rendered (if anything). But the proposed mount function in this RFC cannot easily be used for re-rendering, first you would need to call the callback it returns to unmount mount()() which would wipe the whole DOM tree of that section, and only then call mount() to rebuild all of that section again.

About dependence on React. Providing render prop

{
  render: async (context, params) => <MySection context={context} params={params} />;
}

instead of DOM mounting callback

{
  mount: async function mount(context, params) {
    const { renderApp } = await import('./my-section');
    return renderApp(context, params);
  }
}

does not make us more dependent on React. In the future, we can still independently remove React from the Management app and independently remove React from the individual sections, regardless if it is render rendering function or mount handler.

Copy link
Contributor

@joshdover joshdover Sep 4, 2019

Choose a reason for hiding this comment

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

Before I begin, thanks @streamich for this thorough analysis. Getting all these thoughts down "on paper" makes this go way smoother.

This React re-mounting adds unnecessary code: (1) un-mount React; (2) mount it back; (3) add tests for all of that—which needs to be written and then maintained.

I could be missing something, but from what I can tell the only different code that is added here is calling ReactDOM.render. I'm skeptical this needs to be tested beyond a simple functional test that makes sure you can access the section (which we should already have).

Then you need to (4) re-attach all your Error Boundaries; (5) re-attach all your React contexts (which I don't think is possible, in a non-trivial cases); (6) all your React Suspense handlers will be probably broken (I'm not aware if it is possible to re-connect them).

I'm not sure I understand what you mean by "re-attach" or why that would break. Yes, each call to the mount function would involve re-rendering a React tree. If the UI needed to subscribe to Observables (eg. the UI theme) these would be re-set-up on each call, but that should be a trivial operation. Maybe there's more code here than I realize.

It should be noted, that if a value provided in context needs to change it should be provided as an Observable so the React app can subscribe + update when it changes. We should not be unmounting and re-mounting to handle a context value changing.

I'm not sure why React Suspense handlers would break. Some details on this would help.

Remounting React is slower performance-wise than just rendering a React component.

True, but we only need to do this when navigating to a new section. In that case we're about to render a completely new UI anyways. I don't think there's much performance penalty here but maybe we should experiment and actually test that assumption.

As mentioned above, React will anyways be bundled globally once for everyone, so a plugin will not need to "bundle React just to render a management section"; and if somebody wants to render a non-React section, it can be achieved by this 5-line function.

Good point, but the answer here is still it depends. We plan to provide some out of the box webpack configurations, but it's still entirely likely that 3rd party plugin developers may use a different bundler or config that doesn't know React is already available on the page. In this case, the developer would end up bundling a separate copy of React. Though they could work around this, it'd be nice if we made the right thing easy out of the box.


All this to say, I think Vadim's approach is acceptable, it's just that I prefer making Kibana easy to learn over squeaking out a few milliseconds out of the UI.

Too many times in Kibana's history have we invented multiple interfaces & mechanisms for doing very similar things. This has lead to the proliferation of different solutions to the same problem in our source code. It makes learning Kibana development unnecessarily hard.

We need to make Kibana development exceedingly easy. This is as much an internal strategy as it is an external one. Scaling the Kibana & Solution teams at Elastic and growing the plugin community around us is a huge lever in making Kibana an indispensable tool for thousands of users.

Now, is this one thing going to stop someone from picking up Kibana development? Probably not, but the more inconsistencies we add, the worse it becomes. Death by 1000 cuts has to start somewhere.

Copy link
Contributor

@mattkime mattkime Sep 5, 2019

Choose a reason for hiding this comment

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

just thought I'd point out that @streamich is out on leave for a bit which is too bad for the sake of this conversation but good for him.

At risk of speaking without a thorough understanding of all the technical details (which are critically important) - is there a way for us to support framework agnostic and react first ways of doing things? I think we're all in agreement that we expect React to be used the vast majority of the time and our primary reasons for not assuming React usage are -

  1. We should avoid assuming 3rd party contributors have a particular knowledge base
  2. We wish to avoid more technology coupling than absolutely necessary
  3. Tech changes

Counterpoint - IF there aren't any functional differences, only syntactic sugar to providing a react interface, are we trying to over optimize a bootstrap procedure?

IMO - these sort of questions become much simpler when we define and agree upon goals.

}
});

// Registering a new app to an existing section
const kibanaSection = management.sections.get('kibana');
kibanaSection.registerApp({ id: 'my-kibana-management-app', ... });
}

start(core, { management }) {
// access all registered sections, filtered based on capabilities
const sections = management.sections.getAvailable();
sections.forEach(section => console.log(`${section.id} - ${section.title}`));
// automatically navigate to any app by id
management.sections.navigateToApp('my-kibana-management-app');
}
}

// my_plugin/public/my-section.tsx

export function renderApp(context, { sectionBasePath, element }) {
ReactDOM.render(
// `sectionBasePath` would be `/app/management/my-section/my-management-app`
<MyApp basename={sectionBasePath} />,
element
);

// return value must be a function that unmounts (just like Core Application Service)
return () => ReactDOM.unmountComponentAtNode(element);
}
```

We can also create a utility in `kibana_react` to make it easy for folks to `mount` a React app:
```ts
// src/plugins/kibana_react/public/mount_with_react.tsx
import { KibanaContextProvider } from './context';

export const mountWithReact = (
Component: React.ComponentType<{ basename: string }>,
context: AppMountContext,
params: ManagementSectionMountParams,
) => {
ReactDOM.render(
(
<KibanaContextProvider services={{ ...context }}>
<Component basename={params.sectionBasePath} />
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the significance of basename?

Copy link
Contributor

Choose a reason for hiding this comment

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

Its the url base path. This is the name used with the application reg api. I think line 75 does a good job demonstrating but I'm happy to amend it if you have suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for the explanation! I feel like basePath is a fairly well understood concept in Kibana. I think of it as "the app's namespace inside of the URI". If we changed basename to basePath would that carry the connotations? Just throwing it out here, feel free to disregard.

</KibanaContextProvider>
),
params.element
);

return () => ReactDOM.unmountComponentAtNode(params.element);
}

// my_plugin/public/plugin.ts
import { mountWithReact } from 'src/plugins/kibana_react/public';

export class MyPlugin {
setup(core, { management }) {
const kibanaSection = management.sections.get('kibana');
kibanaSection.registerApp({
id: 'my-other-kibana-management-app',
...,
async mount(context, params) {
const { MySection } = await import('./components/my-section');
const unmountCallback = mountWithReact(MySection, context, params);
return () => unmountCallback();
}
});
}
}
```

# Detailed design

```ts
interface ManagementSetup {
sections: SectionsServiceSetup;
}

interface ManagementStart {
sections: SectionsServiceStart;
}

interface SectionsServiceSetup {
get: (sectionId: string) => Section;
getAvailable: () => Section[]; // filtered based on capabilities
register: RegisterSection;
}

interface SectionsServiceStart {
getAvailable: () => Array<Omit<Section, 'registerApp'>>; // filtered based on capabilities
// uses `core.application.navigateToApp` under the hood, automatically prepending the `path` for the link
navigateToApp: (appId: string, options?: { path?: string; state?: any }) => void;
}

type RegisterSection = (
id: string,
title: string,
order?: number,
euiIconType?: string, // takes precedence over `icon` property.
icon?: string, // URL to image file; fallback if no `euiIconType`
) => Section;

type RegisterManagementApp = (
id: string;
title: string;
order?: number;
mount: ManagementSectionMount;
) => ManagementApp;

type Unmount = () => Promise<void> | void;

interface ManagementSectionMountParams {
sectionBasePath: string; // base path for setting up your router
element: HTMLElement; // element the section should render into
}

type ManagementSectionMount = (
context: AppMountContext, // provided by core.ApplicationService
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this is not entirely true from a technical point of view. Contexts have different responsibilities and (in theory) different interfaces.
Anyway it's okay for a while.

params: ManagementSectionMountParams,
) => Unmount | Promise<Unmount>;

interface ManagementApp {
id: string;
title: string;
basePath: string;
sectionId: string;
order?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think id will be used for the url

}

interface Section {
id: string;
title: string;
apps: ManagementApp[];
registerApp: RegisterManagementApp;
order?: number;
euiIconType?: string;
icon?: string;
}
```

# Legacy service (what this would be replacing)

Example of how this looks today:
```js
// myplugin/index
new Kibana.Plugin({
uiExports: {
managementSections: ['myplugin/management'],
}
});

// myplugin/public/management
import { management } from 'ui/management';

// completely new section
const newSection = management.register('mypluginsection', {
name: 'mypluginsection',
order: 10,
display: 'My Plugin',
icon: 'iconName',
});
newSection.register('mypluginlink', {
name: 'mypluginlink',
order: 10,
display: 'My sublink',
url: `#/management/myplugin`,
});

// new link in existing section
const kibanaSection = management.getSection('kibana');
kibanaSection.register('mypluginlink', {
name: 'mypluginlink',
order: 10,
display: 'My sublink',
url: `#/management/myplugin`,
});

// use ui/routes to render component
import routes from 'ui/routes';

const renderReact = (elem) => {
render(<MyApp />, elem);
};

routes.when('management/myplugin', {
controller($scope, $http, kbnUrl) {
$scope.$on('$destroy', () => {
const elem = document.getElementById('usersReactRoot');
if (elem) unmountComponentAtNode(elem);
});
$scope.$$postDigest(() => {
const elem = document.getElementById('usersReactRoot');
const changeUrl = (url) => {
kbnUrl.change(url);
$scope.$apply();
};
renderReact(elem, $http, changeUrl);
});
},
});
```
Current public contracts owned by the legacy service:
```js
// ui/management/index
interface API {
PAGE_TITLE_COMPONENT: string; // actually related to advanced settings?
PAGE_SUBTITLE_COMPONENT: string; // actually related to advanced settings?
PAGE_FOOTER_COMPONENT: string; // actually related to advanced settings?
SidebarNav: React.SFC<any>;
registerSettingsComponent: (
id: string,
component: string | React.SFC<any>,
allowOverride: boolean
) => void;
management: new ManagementSection();
MANAGEMENT_BREADCRUMB: {
text: string;
href: string;
};
}

// ui/management/section
class ManagementSection {
get visibleItems,
addListener: (fn: function) => void,
register: (id: string, options: Options) => ManagementSection,
deregister: (id: string) => void,
hasItem: (id: string) => boolean,
getSection: (id: string) => ManagementSection,
hide: () => void,
show: () => void,
disable: () => void,
enable: () => void,
}

interface Options {
order: number | null;
display: string | null; // defaults to id
url: string | null; // defaults to ''
visible: boolean | null; // defaults to true
disabled: boolean | null; // defaults to false
tooltip: string | null; // defaults to ''
icon: string | null; // defaults to ''
}
```

# Notes

- The hide/show/disable/enable options were dropped with the assumption that we will be working with uiCapabilities to determine this instead... so people shouldn't need to manage it manually as they can look up a pre-filtered list of sections.
- This was updated to add flexibility for custom (non-EUI) icons as outlined in [#32661](https://github.com/elastic/kibana/issues/32661). Much like the Core Application Service, you either choose an EUI icon, or provide a URL to an icon.

# Drawbacks

- This removes the ability to infinitely nest sections within each other by making a distinction between a section header and a nav link.
- So far we didn't seem to be using this feature anyway, but would like feedback on any use cases for it.

# Reference

- Issues about Global vs Spaces-based management sections: https://github.com/elastic/kibana/issues/37285 https://github.com/elastic/kibana/issues/37283
- Mockups related to above issues: https://marvelapp.com/52b8616/screen/57582729

# Alternatives

An alternative design would be making everything React-specific and simply requiring consumers of the service to provide a React component to render when a route is hit, or giving them a react-router instance to work with.

This would require slightly less work for folks using the service as it would eliminate the need for a `mount` function. However, it comes at the cost of forcing folks into a specific rendering framework, which ultimately provides less flexibility.
Copy link
Contributor

@mshustov mshustov Sep 4, 2019

Choose a reason for hiding this comment

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

  • How many registries/extension points are extendable by 3rd party plugins? In the sake of consistency, they should follow the same convention.
  • Do we have an agreement that non-core API should be library-agnostic? I haven't heard about it so far.
  • Could we declare a management section to be a part of an application? As I understand the list of the links is the only something that all management sections have in common. We do not create a separate app for NavDrawer, but do for management.

Copy link
Contributor

@cjcenizal cjcenizal Sep 4, 2019

Choose a reason for hiding this comment

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

How many registries/extension points are extendable by 3rd party plugins? In the sake of consistency, they should follow the same convention.

Index Management allows other plugins to extend its UI. Here's an example of these extension points being consumed. These extension points are tightly coupled to both React and EUI.

Copy link
Contributor

Choose a reason for hiding this comment

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

These extension points are tightly coupled to both React and EUI.

Some of them are coupled, some use POJO to setup content. Depending on the agreement for management section we can adjust (or not) this API as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it’s possible to adjust this API but it becomes difficult for us (the ES UI team that owns this code) to justify time spent on refactoring without a clear benefit, since we built these extension points for our own use, not use by third parties.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no problem with some of these very specific extension points being React. I just see the overall Management app is more generic and I think that should be decoupled from React. I think that the other ES UI extension are very unlikely to be consumed by 3rd parties.


# Adoption strategy

Our strategy for implementing this should be to build the service entirely in the new platform in a `management` plugin, so that plugins can gradually cut over to the new service as they prepare to migrate to the new platform.

One thing we would need to figure out is how to bridge the gap between the new plugin and the legacy `ui/management` service. Ideally we would find a way to integrate the two, such that the management nav could display items registered via both services. This is a strategy we'd need to work out in more detail as we got closer to implementation.

# How we teach this

The hope is that this will already feel familiar to Kibana application developers, as most will have already been exposed to the Core Application Service and how it handles mounting.

A guide could also be added to the "Management" section of the Kibana docs (the legacy service is not even formally documented).