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

Advanced settings component registry ⇒ kibana platform plugin #55940

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
be0e773
advanced settings component registry to new platform
mattkime Jan 25, 2020
9e7d525
change plugin name, fix some i18n
mattkime Jan 25, 2020
f3b467b
fix karma mocks
mattkime Jan 25, 2020
380838c
fix jest tests
mattkime Jan 25, 2020
b13abeb
fix jest tests
mattkime Jan 25, 2020
ffdf36e
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
elasticmachine Jan 27, 2020
7b225aa
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
elasticmachine Jan 29, 2020
4a00882
refactor component registry
mattkime Jan 30, 2020
5ef28a5
refactor component registry
mattkime Jan 30, 2020
fdacef1
refactor component registry
mattkime Jan 30, 2020
095efaf
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Jan 30, 2020
f10a154
fix spaces test
mattkime Jan 30, 2020
ff96574
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Jan 30, 2020
863cb0f
better use of mocks
mattkime Jan 30, 2020
01689f3
fix snapshot, change component type to use strings as ids
mattkime Jan 31, 2020
f7390f4
Merge branch 'adv_settings_componenet_reg_to_plugin' of github.com:ma…
mattkime Jan 31, 2020
823a0c3
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Jan 31, 2020
5b0caf9
fix tests
mattkime Feb 1, 2020
8ddbcc7
karma fix
mattkime Feb 1, 2020
66215ef
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Feb 3, 2020
335a8e8
use BehaviorSubject for subscribing to component state changes
mattkime Feb 3, 2020
832b1c4
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Feb 3, 2020
44df84b
Revert "use BehaviorSubject for subscribing to component state changes"
mattkime Feb 3, 2020
5ae8a5e
remove comment
mattkime Feb 3, 2020
65e7b7b
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
elasticmachine Feb 4, 2020
b0b2d13
Merge branch 'master' into adv_settings_componenet_reg_to_plugin
mattkime Feb 4, 2020
ea8c1bf
Merge branch 'adv_settings_componenet_reg_to_plugin' of github.com:ma…
mattkime Feb 4, 2020
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
1 change: 1 addition & 0 deletions .i18nrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"kbn": "src/legacy/core_plugins/kibana",
"kbnDocViews": "src/legacy/core_plugins/kbn_doc_views",
"management": ["src/legacy/core_plugins/management", "src/plugins/management"],
"advancedSettings": "src/plugins/advanced_settings",
"kibana_react": "src/legacy/core_plugins/kibana_react",
"kibana-react": "src/plugins/kibana_react",
"kibana_utils": "src/plugins/kibana_utils",
Expand Down
8 changes: 0 additions & 8 deletions rfcs/text/0006_management_section_service.md
Original file line number Diff line number Diff line change
Expand Up @@ -257,15 +257,7 @@ 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.FC<any>;
registerSettingsComponent: (
id: string,
component: string | React.FC<any>,
allowOverride: boolean
) => void;
management: new ManagementSection();
MANAGEMENT_BREADCRUMB: {
text: string;
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,17 @@ function mockConfig() {
core: {
uiSettings: config,
},
plugins: {
advancedSettings: {
componentRegistry: {
get: () => <div>Hello</div>,
componentType: {
PAGE_TITLE_COMPONENT: 'page_title_component',
PAGE_SUBTITLE_COMPONENT: 'page_subtitle_component',
},
},
},
},
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,6 @@ import { getAriaName, toEditableConfig, DEFAULT_CATEGORY } from './lib';

import { FieldSetting, IQuery } from './types';

import {
registerDefaultComponents,
PAGE_TITLE_COMPONENT,
PAGE_SUBTITLE_COMPONENT,
PAGE_FOOTER_COMPONENT,
} from './components/default_component_registry';
import { getSettingsComponent } from './components/component_registry';

interface AdvancedSettingsProps {
queryText: string;
enableSaving: boolean;
Expand Down Expand Up @@ -75,8 +67,6 @@ export class AdvancedSettings extends Component<AdvancedSettingsProps, AdvancedS
footerQueryMatched: false,
filteredSettings: this.mapSettings(Query.execute(parsedQuery, this.settings)),
};

registerDefaultComponents();
}

init(config: IUiSettingsClient) {
Expand Down Expand Up @@ -166,10 +156,13 @@ export class AdvancedSettings extends Component<AdvancedSettingsProps, AdvancedS

render() {
const { filteredSettings, query, footerQueryMatched } = this.state;
const componentRegistry = npStart.plugins.advancedSettings.componentRegistry;

const PageTitle = getSettingsComponent(PAGE_TITLE_COMPONENT);
const PageSubtitle = getSettingsComponent(PAGE_SUBTITLE_COMPONENT);
const PageFooter = getSettingsComponent(PAGE_FOOTER_COMPONENT);
const PageTitle = componentRegistry.get(componentRegistry.componentType.PAGE_TITLE_COMPONENT);
const PageSubtitle = componentRegistry.get(
componentRegistry.componentType.PAGE_SUBTITLE_COMPONENT
);
const PageFooter = componentRegistry.get(componentRegistry.componentType.PAGE_FOOTER_COMPONENT);

return (
<div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@
import React from 'react';
import routes from 'ui/routes';

import { registerSettingsComponent, PAGE_FOOTER_COMPONENT } from 'ui/management';
import { npStart } from 'ui/new_platform';
import { TelemetryOptInProvider } from '../../services';
import { TelemetryForm } from '../../components';

routes.defaults(/\/management/, {
resolve: {
telemetryManagementSection: function(Private) {
const telemetryOptInProvider = Private(TelemetryOptInProvider);
const componentRegistry = npStart.plugins.advancedSettings.componentRegistry;

const Component = props => (
<TelemetryForm
Expand All @@ -36,7 +37,11 @@ routes.defaults(/\/management/, {
/>
);

registerSettingsComponent(PAGE_FOOTER_COMPONENT, Component, true);
componentRegistry.register(
componentRegistry.componentType.PAGE_FOOTER_COMPONENT,
Component,
true
);
},
},
});
8 changes: 0 additions & 8 deletions src/legacy/ui/public/management/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,7 @@
*/

declare module 'ui/management' {
export const PAGE_TITLE_COMPONENT: string;
export const PAGE_SUBTITLE_COMPONENT: string;
export const PAGE_FOOTER_COMPONENT: string;
export const SidebarNav: React.FC<any>;
export function registerSettingsComponent(
id: string,
component: string | React.FC<any>,
allowOverride: boolean
): void;
export const management: any; // TODO - properly provide types
export const MANAGEMENT_BREADCRUMB: {
text: string;
Expand Down
6 changes: 0 additions & 6 deletions src/legacy/ui/public/management/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,6 @@
* under the License.
*/

export {
PAGE_TITLE_COMPONENT,
PAGE_SUBTITLE_COMPONENT,
PAGE_FOOTER_COMPONENT,
} from '../../../core_plugins/kibana/public/management/sections/settings/components/default_component_registry';
export { registerSettingsComponent } from '../../../core_plugins/kibana/public/management/sections/settings/components/component_registry';
export { MANAGEMENT_BREADCRUMB } from './breadcrumbs';
import { npStart } from 'ui/new_platform';
export const management = npStart.plugins.management.legacy;
9 changes: 9 additions & 0 deletions src/legacy/ui/public/new_platform/new_platform.karma_mock.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ const mockCore = {
export const npSetup = {
core: mockCore,
plugins: {
advancedSettings: {
componentRegistry: {
register: sinon.fake(),
componentType: {
PAGE_TITLE_COMPONENT: 'page_title_component',
PAGE_SUBTITLE_COMPONENT: 'page_subtitle_component',
},
},
},
usageCollection: {
allowTrackUserAgent: sinon.fake(),
reportUiStats: sinon.fake(),
Expand Down
6 changes: 6 additions & 0 deletions src/legacy/ui/public/new_platform/new_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ import { KibanaLegacySetup, KibanaLegacyStart } from '../../../../plugins/kibana
import { HomePublicPluginSetup, HomePublicPluginStart } from '../../../../plugins/home/public';
import { SharePluginSetup, SharePluginStart } from '../../../../plugins/share/public';
import { ManagementStart } from '../../../../plugins/management/public';
import {
AdvancedSettingsSetup,
AdvancedSettingsStart,
} from '../../../../plugins/advanced_settings/public';
import { BfetchPublicSetup, BfetchPublicStart } from '../../../../plugins/bfetch/public';
import { UsageCollectionSetup } from '../../../../plugins/usage_collection/public';
import {
Expand All @@ -54,6 +58,7 @@ export interface PluginsSetup {
kibana_legacy: KibanaLegacySetup;
share: SharePluginSetup;
usageCollection: UsageCollectionSetup;
advancedSettings: AdvancedSettingsSetup;
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved
}

export interface PluginsStart {
Expand All @@ -70,6 +75,7 @@ export interface PluginsStart {
kibana_legacy: KibanaLegacyStart;
share: SharePluginStart;
management: ManagementStart;
advancedSettings: AdvancedSettingsStart;
}

export const npSetup = {
Expand Down
7 changes: 7 additions & 0 deletions src/plugins/advanced_settings/kibana.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"id": "advancedSettings",
"version": "kibana",
"server": false,
"ui": true,
"requiredPlugins": []
}
38 changes: 38 additions & 0 deletions src/plugins/advanced_settings/public/component_registry/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { registerSettingsComponent, getSettingsComponent } from './component_registry';
import {
registerDefaultComponents,
PAGE_TITLE_COMPONENT,
PAGE_SUBTITLE_COMPONENT,
PAGE_FOOTER_COMPONENT,
} from './default_component_registry';

registerDefaultComponents();
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved

export const componentRegistry = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

API right here

register: registerSettingsComponent,
get: getSettingsComponent,
componentType: {
PAGE_TITLE_COMPONENT,
PAGE_SUBTITLE_COMPONENT,
PAGE_FOOTER_COMPONENT,
},
Copy link
Member

Choose a reason for hiding this comment

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

It feels like these should be static exports instead of part of the runtime contract, WDYT?

Perhaps in public/index.ts as:

export const component = {
  PAGE_TITLE_COMPONENT,
  PAGE_SUBTITLE_COMPONENT,
  PAGE_FOOTER_COMPONENT,
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like thats likely one more import. Changed to an enum which seems to make code a little more compact. Past that I don't have a strong opinion.

};

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const PageTitle = () => {
return (
<EuiText>
<h1 data-test-subj="managementSettingsTitle">
<FormattedMessage id="kbn.management.settings.pageTitle" defaultMessage="Settings" />
<FormattedMessage id="advancedSettings.pageTitle" defaultMessage="Settings" />
</h1>
</EuiText>
);
Expand Down
26 changes: 26 additions & 0 deletions src/plugins/advanced_settings/public/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { PluginInitializerContext } from 'kibana/public';
import { AdvancedSettingsPlugin } from './plugin';
export { AdvancedSettingsSetup, AdvancedSettingsStart } from './types';

export function plugin(initializerContext: PluginInitializerContext) {
return new AdvancedSettingsPlugin();
}
37 changes: 37 additions & 0 deletions src/plugins/advanced_settings/public/plugin.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { CoreSetup, CoreStart, Plugin } from 'kibana/public';
import { componentRegistry } from './component_registry';
import { AdvancedSettingsSetup, AdvancedSettingsStart } from './types';

export class AdvancedSettingsPlugin
implements Plugin<AdvancedSettingsSetup, AdvancedSettingsStart> {
public setup(core: CoreSetup) {
return {
componentRegistry,
};
}

public start(core: CoreStart) {
return {
componentRegistry,
lukeelmers marked this conversation as resolved.
Show resolved Hide resolved
};
}
}
27 changes: 27 additions & 0 deletions src/plugins/advanced_settings/public/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { componentRegistry } from './component_registry';

export interface AdvancedSettingsSetup {
componentRegistry: typeof componentRegistry;
}
export interface AdvancedSettingsStart {
componentRegistry: typeof componentRegistry;
}
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want these interfaces to be the same since it is a registry, right?

i.e. on Setup maybe only register should be available, and on Start only get, so that nobody is trying to register after setup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like its sufficient to provide register access in the setup lifecycle although it doesn't seem necessary to me - but I might be missing something. That said, I'm a little uncertain how this relates to legacy code - I'd be using register after setup is complete, correct?

Copy link
Member

Choose a reason for hiding this comment

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

The reasoning behind doing it this way in other registries has been that, if you only allow registering in the setup lifecycle, by the time you get to start you can guaranteed that all items that are going to be registered are already present, and can then safely use get to retrieve anything from the registry.

This avoids the scenario where you may try to do something with the items in a registry which has not been fully populated yet.

It's a little weird in this case because I believe the only place the registry is being read from is in the advanced settings UI itself, which means new registry items will likely just re-render in React if they come in late. So I'm not sure if it would have a material impact in this particular plugin or not; I'm mostly bringing it up for the sake of consistency with how we've dealt with registries in other plugins. Examples of places where we've done this are expressions, inspector, management.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, what about license changes? How does that play with the setup / start, register / get pattern?

Loading