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

Move Storage ⇒ NP #49448

Merged
merged 10 commits into from
Oct 29, 2019
Merged
Show file tree
Hide file tree
Changes from 7 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
7 changes: 1 addition & 6 deletions src/legacy/core_plugins/data/public/legacy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,13 @@
*/

import { npSetup, npStart } from 'ui/new_platform';
import { LegacyDependenciesPlugin } from './shim/legacy_dependencies_plugin';
import { plugin } from '.';

const dataPlugin = plugin();
const legacyPlugin = new LegacyDependenciesPlugin();

export const setup = dataPlugin.setup(npSetup.core, {
__LEGACY: legacyPlugin.setup(),
});
export const setup = dataPlugin.setup(npSetup.core);

export const start = dataPlugin.start(npStart.core, {
data: npStart.plugins.data,
uiActions: npSetup.plugins.uiActions,
__LEGACY: legacyPlugin.start(),
});
33 changes: 9 additions & 24 deletions src/legacy/core_plugins/data/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,7 @@ import { QueryService, QuerySetup } from './query';
import { FilterService, FilterSetup, FilterStart } from './filter';
import { TimefilterService, TimefilterSetup } from './timefilter';
import { IndexPatternsService, IndexPatternsSetup, IndexPatternsStart } from './index_patterns';
import {
LegacyDependenciesPluginSetup,
LegacyDependenciesPluginStart,
} from './shim/legacy_dependencies_plugin';
import { Storage, IStorageWrapper } from '../../../../../src/plugins/kibana_utils/public';
import { DataPublicPluginStart } from '../../../../plugins/data/public';
import { initLegacyModule } from './shim/legacy_module';
import { IUiActionsSetup } from '../../../../plugins/ui_actions/public';
Expand All @@ -36,19 +33,9 @@ import {
} from './filter/action/apply_filter_action';
import { APPLY_FILTER_TRIGGER } from '../../../../plugins/embeddable/public';

/**
* Interface for any dependencies on other plugins' `setup` contracts.
*
* @internal
*/
export interface DataPluginSetupDependencies {
__LEGACY: LegacyDependenciesPluginSetup;
}

export interface DataPluginStartDependencies {
data: DataPublicPluginStart;
uiActions: IUiActionsSetup;
__LEGACY: LegacyDependenciesPluginStart;
}

/**
Expand Down Expand Up @@ -90,9 +77,7 @@ export interface DataStart {
* in the setup/start interfaces. The remaining items exported here are either types,
* or static code.
*/
export class DataPlugin
implements
Plugin<DataSetup, DataStart, DataPluginSetupDependencies, DataPluginStartDependencies> {
export class DataPlugin implements Plugin<DataSetup, DataStart, {}, DataPluginStartDependencies> {
// Exposed services, sorted alphabetically
private readonly filter: FilterService = new FilterService();
private readonly indexPatterns: IndexPatternsService = new IndexPatternsService();
Expand All @@ -101,13 +86,16 @@ export class DataPlugin
private readonly timefilter: TimefilterService = new TimefilterService();

private setupApi!: DataSetup;
private storage!: IStorageWrapper;

public setup(core: CoreSetup, { __LEGACY }: DataPluginSetupDependencies): DataSetup {
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 a little scared about the changes on the way the storage is now accessed. What I mean is that before, the storage access was provided by a setup dependency (DataPluginSetupDependencies), and now the storage can be accessed basically from anywhere in the code base with a simple new Storage(window.localStorage), which may makes the migration more difficult when we'll implement #41982, as more calls may be added in places where there is no proper access to core APIs.

@joshdover WDYT ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet I agree that the way that storage was and is handled right now is not great. Even before this PR, apps would randomly create their own storage representation and pass it around. And this PR doesn't change the way this is handled (checkout x-pack/legacy/plugins/graph/public/app.js or x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx for example).

The __LEGACY wrapper was there only to mark the fact that Storage was imported from ui/public and not from NP. Now that it's moved to NP, we don't need that any more.

What we should do is, once platform team offers a core storage access service, is replace every occurrence of new Storage with that service.

public setup(core: CoreSetup): DataSetup {
const { uiSettings } = core;

this.storage = new Storage(window.localStorage);

const timefilterService = this.timefilter.setup({
uiSettings,
store: __LEGACY.storage,
storage: this.storage,
});
const filterService = this.filter.setup({
uiSettings,
Expand All @@ -122,10 +110,7 @@ export class DataPlugin
return this.setupApi;
}

public start(
core: CoreStart,
{ __LEGACY, data, uiActions }: DataPluginStartDependencies
): DataStart {
public start(core: CoreStart, { data, uiActions }: DataPluginStartDependencies): DataStart {
const { uiSettings, http, notifications, savedObjects } = core;

const indexPatternsService = this.indexPatterns.start({
Expand All @@ -140,7 +125,7 @@ export class DataPlugin
const SearchBar = createSearchBar({
core,
data,
store: __LEGACY.storage,
storage: this.storage,
timefilter: this.setupApi.timefilter,
filterManager: this.setupApi.filter.filterManager,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import _ from 'lodash';
import * as Rx from 'rxjs';
import { map } from 'rxjs/operators';
import { Storage } from '../../types';
import { IStorageWrapper } from 'src/plugins/kibana_utils/public';

const defaultIsDuplicate = (oldItem: any, newItem: any) => {
return _.isEqual(oldItem, newItem);
Expand All @@ -37,12 +37,12 @@ export class PersistedLog<T = any> {
public maxLength?: number;
public filterDuplicates?: boolean;
public isDuplicate: (oldItem: T, newItem: T) => boolean;
public storage: Storage;
public storage: IStorageWrapper;
public items: T[];

private update$ = new Rx.BehaviorSubject(undefined);

constructor(name: string, options: PersistedLogOptions<T> = {}, storage: Storage) {
constructor(name: string, options: PersistedLogOptions<T> = {}, storage: IStorageWrapper) {
this.name = name;
this.maxLength =
typeof options.maxLength === '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 @@ -58,7 +58,7 @@ const createMockWebStorage = () => ({
});

const createMockStorage = () => ({
store: createMockWebStorage(),
storage: createMockWebStorage(),
get: jest.fn(),
set: jest.fn(),
remove: jest.fn(),
Expand All @@ -80,7 +80,7 @@ const mockIndexPattern = {
],
} as IndexPattern;

function wrapQueryBarInputInContext(testProps: any, store?: any) {
function wrapQueryBarInputInContext(testProps: any, storage?: any) {
const defaultOptions = {
screenTitle: 'Another Screen',
intl: null as any,
Expand All @@ -89,7 +89,7 @@ function wrapQueryBarInputInContext(testProps: any, store?: any) {
const services = {
...startMock,
appName: testProps.appName || 'test',
store: store || createMockStorage(),
storage: storage || createMockStorage(),
};

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ export class QueryBarInputUI extends Component<Props, State> {
body: JSON.stringify({ opt_in: language === 'kuery' }),
});

this.services.store.set('kibana.userQueryLanguage', language);
this.services.storage.set('kibana.userQueryLanguage', language);

const newQuery = { query: '', language };
this.onChange(newQuery);
Expand All @@ -387,10 +387,10 @@ export class QueryBarInputUI extends Component<Props, State> {
};

private initPersistedLog = () => {
const { uiSettings, store, appName } = this.services;
const { uiSettings, storage, appName } = this.services;
this.persistedLog = this.props.persistedLog
? this.props.persistedLog
: getQueryLog(uiSettings, store, appName, this.props.query.language);
: getQueryLog(uiSettings, storage, appName, this.props.query.language);
};

public onMouseEnterSuggestion = (index: number) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const createMockWebStorage = () => ({
});

const createMockStorage = () => ({
store: createMockWebStorage(),
storage: createMockWebStorage(),
get: jest.fn(),
set: jest.fn(),
remove: jest.fn(),
Expand Down Expand Up @@ -112,7 +112,7 @@ function wrapQueryBarTopRowInContext(testProps: any) {
const services = {
...startMock,
appName: 'discover',
store: createMockStorage(),
storage: createMockStorage(),
};

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ function QueryBarTopRowUI(props: Props) {
const [isDateRangeInvalid, setIsDateRangeInvalid] = useState(false);

const kibana = useKibana<IDataPluginServices>();
const { uiSettings, notifications, store, appName, docLinks } = kibana.services;
const { uiSettings, notifications, storage, appName, docLinks } = kibana.services;

const kueryQuerySyntaxLink: string = docLinks!.links.query.kueryQuerySyntax;

Expand All @@ -82,7 +82,7 @@ function QueryBarTopRowUI(props: Props) {

useEffect(() => {
if (!props.query) return;
persistedLog = getQueryLog(uiSettings!, store, appName, props.query.language);
persistedLog = getQueryLog(uiSettings!, storage, appName, props.query.language);
}, [queryLanguage]);

function onClickSubmitButton(event: React.MouseEvent<HTMLButtonElement>) {
Expand Down Expand Up @@ -211,7 +211,7 @@ function QueryBarTopRowUI(props: Props) {
}

function shouldRenderQueryInput(): boolean {
return Boolean(props.showQueryInput && props.indexPatterns && props.query && store);
return Boolean(props.showQueryInput && props.indexPatterns && props.query && storage);
}

function renderUpdateButton() {
Expand Down Expand Up @@ -293,7 +293,7 @@ function QueryBarTopRowUI(props: Props) {
if (
language === 'kuery' &&
typeof query === 'string' &&
(!store || !store.get('kibana.luceneSyntaxWarningOptOut')) &&
(!storage || !storage.get('kibana.luceneSyntaxWarningOptOut')) &&
doesKueryExpressionHaveLuceneSyntaxError(query)
) {
const toast = notifications!.toasts.addWarning({
Expand Down Expand Up @@ -337,8 +337,8 @@ function QueryBarTopRowUI(props: Props) {
}

function onLuceneSyntaxWarningOptOut(toast: Toast) {
if (!store) return;
store.set('kibana.luceneSyntaxWarningOptOut', true);
if (!storage) return;
storage.set('kibana.luceneSyntaxWarningOptOut', true);
notifications!.toasts.remove(toast);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@
*/

import { UiSettingsClientContract } from 'src/core/public';
import { IStorageWrapper } from 'src/plugins/kibana_utils/public';
import { PersistedLog } from '../../persisted_log';
import { Storage } from '../../../types';

export function getQueryLog(
uiSettings: UiSettingsClientContract,
store: Storage,
storage: IStorageWrapper,
appName: string,
language: string
) {
Expand All @@ -33,6 +33,6 @@ export function getQueryLog(
maxLength: uiSettings.get('history:limit'),
filterDuplicates: true,
},
store
storage
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { Subscription } from 'rxjs';
import { Filter } from '@kbn/es-query';
import { CoreStart } from 'src/core/public';
import { DataPublicPluginStart } from 'src/plugins/data/public';
import { Storage } from '../../../types';
import { IStorageWrapper } from 'src/plugins/kibana_utils/public';
import { KibanaContextProvider } from '../../../../../../../../src/plugins/kibana_react/public';
import { TimefilterSetup } from '../../../timefilter';
import { FilterManager, SearchBar } from '../../../';
Expand All @@ -31,7 +31,7 @@ import { SearchBarOwnProps } from '.';
interface StatefulSearchBarDeps {
core: CoreStart;
data: DataPublicPluginStart;
store: Storage;
storage: IStorageWrapper;
timefilter: TimefilterSetup;
filterManager: FilterManager;
}
Expand All @@ -57,7 +57,7 @@ const defaultOnRefreshChange = (timefilter: TimefilterSetup) => {

export function createSearchBar({
core,
store,
storage,
timefilter,
filterManager,
data,
Expand Down Expand Up @@ -113,7 +113,7 @@ export function createSearchBar({
services={{
appName: props.appName,
data,
store,
storage,
...core,
}}
>
Expand Down
Loading