Skip to content

Commit

Permalink
Revert "use BehaviorSubject for subscribing to component state changes"
Browse files Browse the repository at this point in the history
This reverts commit 335a8e8.
  • Loading branch information
mattkime committed Feb 3, 2020
1 parent 832b1c4 commit 44df84b
Show file tree
Hide file tree
Showing 7 changed files with 24 additions and 75 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { Search } from './components/search';
import { Form } from './components/form';
import { AdvancedSettingsVoiceAnnouncement } from './components/advanced_settings_voice_announcement';
import { IUiSettingsClient } from '../../../../../../../core/public/';
import { RegistryComponent } from '../../../../../../../plugins/advanced_settings/public';

import { getAriaName, toEditableConfig, DEFAULT_CATEGORY } from './lib';

Expand All @@ -41,9 +40,6 @@ interface AdvancedSettingsState {
footerQueryMatched: boolean;
query: IQuery;
filteredSettings: Record<string, FieldSetting[]>;
PageTitle: RegistryComponent;
PageSubtitle: RegistryComponent;
PageFooter: RegistryComponent;
}

type GroupedSettings = Record<string, FieldSetting[]>;
Expand All @@ -66,27 +62,11 @@ export class AdvancedSettings extends Component<AdvancedSettingsProps, AdvancedS
this.categories = this.initCategories(this.groupedSettings);
this.categoryCounts = this.initCategoryCounts(this.groupedSettings);

const componentRegistry = npStart.plugins.advancedSettings.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
);

this.state = {
query: parsedQuery,
footerQueryMatched: false,
filteredSettings: this.mapSettings(Query.execute(parsedQuery, this.settings)),
PageTitle: $PageTitle.getValue(),
PageSubtitle: $PageSubtitle.getValue(),
PageFooter: $PageFooter.getValue(),
};

$PageTitle.subscribe(PageTitle => this.setState({ PageTitle }));
$PageSubtitle.subscribe(PageSubtitle => this.setState({ PageSubtitle }));
$PageFooter.subscribe(PageFooter => this.setState({ PageFooter }));
}

init(config: IUiSettingsClient) {
Expand Down Expand Up @@ -175,14 +155,14 @@ export class AdvancedSettings extends Component<AdvancedSettingsProps, AdvancedS
};

render() {
const {
filteredSettings,
query,
footerQueryMatched,
PageTitle,
PageSubtitle,
PageFooter,
} = this.state;
const { filteredSettings, query, footerQueryMatched } = this.state;
const componentRegistry = npStart.plugins.advancedSettings.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 @@ -40,7 +40,6 @@ routes.defaults(/\/management/, {
componentRegistry.register(
componentRegistry.componentType.PAGE_FOOTER_COMPONENT,
Component,
undefined,
true
);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,12 @@ describe('ComponentRegistry', () => {
registry.setup.register(
ComponentRegistry.componentType.PAGE_TITLE_COMPONENT,
anotherComponent,
undefined,
true
);

expect(
registry.start.get$(ComponentRegistry.componentType.PAGE_TITLE_COMPONENT).getValue()
).toBe(anotherComponent);
expect(registry.start.get(ComponentRegistry.componentType.PAGE_TITLE_COMPONENT)).toBe(
anotherComponent
);
});
});

Expand All @@ -65,13 +64,12 @@ describe('ComponentRegistry', () => {
const registry = new ComponentRegistry();
const component = () => <div />;
registry.setup.register(ComponentRegistry.componentType.PAGE_TITLE_COMPONENT, component);
expect(
registry.start.get$(ComponentRegistry.componentType.PAGE_TITLE_COMPONENT).getValue()
).toBe(component);
expect(registry.start.get(ComponentRegistry.componentType.PAGE_TITLE_COMPONENT)).toBe(
component
);
});
});

// todo, look at these and add tests for enable / disabling state
it('should set a displayName for the component if one does not exist', () => {
const component: React.ComponentType = () => <div />;
// component.displayName = ComponentRegistry.componentType.PAGE_TITLE_COMPONENT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

import { ComponentType } from 'react';
import { BehaviorSubject } from 'rxjs';
import { PageTitle } from './page_title';
import { PageSubtitle } from './page_subtitle';
import { PageFooter } from './page_footer';
Expand All @@ -34,14 +33,7 @@ const componentType: { [key: string]: Id } = {
PAGE_FOOTER_COMPONENT: 'advanced_settings_page_footer' as Id,
};

export type RegistryComponent = ComponentType<Record<string, any> | undefined>;

type ComponentStatus = boolean;

interface RegistryComponentWithStatus {
component: RegistryComponent;
$updater: BehaviorSubject<boolean>;
}
type RegistryComponent = ComponentType<Record<string, any> | undefined>;

export class ComponentRegistry {
static readonly componentType = componentType;
Expand All @@ -51,7 +43,7 @@ export class ComponentRegistry {
advanced_settings_page_footer: PageFooter,
};

registry: { [key in Id]?: RegistryComponentWithStatus } = {};
registry: { [key in Id]?: RegistryComponent } = {};

/**
* Attempts to register the provided component, with the ability to optionally allow
Expand All @@ -63,13 +55,7 @@ export class ComponentRegistry {
* @param {*} component the component
* @param {*} allowOverride (default: false) - optional flag to allow this component to override a previously registered component
*/
private register(
id: Id,
component: RegistryComponent,
$updater: BehaviorSubject<ComponentStatus> = new BehaviorSubject(true as ComponentStatus),
allowOverride = false
) {
// todo allow registration of observable
private register(id: Id, component: RegistryComponent, allowOverride = false) {
if (!allowOverride && id in this.registry) {
throw new Error(`Component with id ${id} is already registered.`);
}
Expand All @@ -80,29 +66,17 @@ export class ComponentRegistry {
component.displayName = id;
}

this.registry[id] = { component, $updater };
this.registry[id] = component;
}

/**
* Retrieve a registered component by its ID.
* If the component does not exist, then an exception is thrown.
*
* @param {*} id the ID of the component to retrieve
*/
private $get(id: Id): BehaviorSubject<RegistryComponent> {
const defaultComponent = ComponentRegistry.defaultRegistry[id];
const componentWithStatus = this.registry[id];
const isRegistered = !!componentWithStatus;
const getComponent = () => {
return isRegistered && componentWithStatus!.$updater.getValue()
? componentWithStatus!.component
: defaultComponent;
};

const subj = new BehaviorSubject(getComponent());
if (isRegistered) {
componentWithStatus!.$updater.subscribe(() => subj.next(getComponent()));
}
return subj;
private get(id: Id): RegistryComponent {
return this.registry[id] || ComponentRegistry.defaultRegistry[id];
}

setup = {
Expand All @@ -112,6 +86,6 @@ export class ComponentRegistry {

start = {
componentType: ComponentRegistry.componentType,
get$: this.$get.bind(this),
get: this.get.bind(this),
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,4 @@
* under the License.
*/

export { ComponentRegistry, RegistryComponent } from './component_registry';
export { ComponentRegistry } from './component_registry';
2 changes: 1 addition & 1 deletion src/plugins/advanced_settings/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { PluginInitializerContext } from 'kibana/public';
import { AdvancedSettingsPlugin } from './plugin';
export { AdvancedSettingsSetup, AdvancedSettingsStart } from './types';
export { ComponentRegistry, RegistryComponent } from './component_registry';
export { ComponentRegistry } from './component_registry';

export function plugin(initializerContext: PluginInitializerContext) {
return new AdvancedSettingsPlugin();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,11 @@ export class AdvancedSettingsService {
componentRegistry.register(
componentRegistry.componentType.PAGE_TITLE_COMPONENT,
PageTitle,
undefined,
true
);
componentRegistry.register(
componentRegistry.componentType.PAGE_SUBTITLE_COMPONENT,
SubTitle,
undefined,
true
);
}
Expand Down

0 comments on commit 44df84b

Please sign in to comment.