Skip to content
Closed
Show file tree
Hide file tree
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
4 changes: 4 additions & 0 deletions src/legacy/core_plugins/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export default function DataPlugin(kibana: any) {
}).default();
},
init: (server: Legacy.Server) => ({}),
uiExports: {
injectDefaultVars: () => ({}),
styleSheetPaths: resolve(__dirname, 'public/index.scss'),
},
};

return new kibana.Plugin(config);
Expand Down
4 changes: 4 additions & 0 deletions src/legacy/core_plugins/data/public/index.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
@import 'src/legacy/ui/public/styles/styling_constants';

@import './query_bar/index';

13 changes: 12 additions & 1 deletion src/legacy/core_plugins/data/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,32 @@
*/

import { IndexPatternsService } from './index_patterns';
import { SearchBarService } from './search_bar';
import { QueryBarService } from './query_bar';

class DataService {
private readonly indexPatterns: IndexPatternsService;
private readonly searchBar: SearchBarService;
private readonly queryBar: QueryBarService;

constructor() {
this.indexPatterns = new IndexPatternsService();
this.searchBar = new SearchBarService();
this.queryBar = new QueryBarService();
}

public setup() {
return {
indexPatterns: this.indexPatterns.setup(),
...this.searchBar.setup(),
...this.queryBar.setup(),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should have clear namespaces in the contract to make it clear what services own what methods:

search: this.searchBar.setup(),
query: this.queryBar.setup(),

This would prevent confusion for users because instead of importing like this:

import { data } from 'plugins/data';
const { toUser, fromUser } = data.helpers; // helpers for what?

They could instead do:

import { data } from 'plugins/data';
const { toUser, fromUser } = data.queryBar.helpers;

And if we are concerned about a dev's experience in having to destructure services just to get a React component, we could create a components.ts file at the top level that re-exports components for devs to import:

// plugins/data/public/components.ts
export { QueryBar } from './query_bar/components/query_bar';
export { SearchBar } from './search_bar/components/search_bar';

// plugins/foo-plugin/public/setup.ts
import { QueryBar, SearchBar } from 'plugins/data/components';

};
}

public stop() {
this.indexPatterns.stop();
this.searchBar.stop();
this.queryBar.stop();
}
}

Expand All @@ -43,7 +53,8 @@ class DataService {
* the data that will eventually be injected by the new platform.
*/
// eslint-disable-next-line import/no-default-export
export default new DataService().setup();
const data = new DataService().setup();
export { data };

/** @public */
export type DataSetup = ReturnType<DataService['setup']>;
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import {
} from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import React, { Component } from 'react';
import { documentationLinks } from '../../documentation_links/documentation_links';
import { documentationLinks } from 'ui/documentation_links/documentation_links';

const kueryQuerySyntaxDocs = documentationLinks.query.kueryQuerySyntax;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,13 @@ import { EuiSuperUpdateButton } from '@elastic/eui';
import { FormattedMessage, InjectedIntl, injectI18n } from '@kbn/i18n/react';
import { documentationLinks } from 'ui/documentation_links';
import { Toast, toastNotifications } from 'ui/notify';

import {
AutocompleteSuggestion,
AutocompleteSuggestionType,
getAutocompleteProvider,
} from '../../autocomplete_providers';
import chrome from '../../chrome';
} from 'ui/autocomplete_providers';
import chrome from 'ui/chrome';

import { fromUser, matchPairs, toUser } from '../lib';
import { QueryLanguageSwitcher } from './language_switcher';
import { SuggestionsComponent } from './typeahead/suggestions_component';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,20 @@

import 'ngreact';
import { wrapInI18nContext } from 'ui/i18n';
import { uiModules } from '../../modules';
import { uiModules } from 'ui/modules';
import { QueryBar } from '../components';

const app = uiModules.get('app/kibana', ['react']);
const app = uiModules.get('app/data', ['react']);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know a lot about ui/modules, but if we are already updating the module names, would it make sense to scope them in a more granular fashion so it's easier to know what-depends-on-what later? e.g. data/query, data/search, etc instead of having them all use the same module name

we probably also don't need app in the module name, doesn't make much sense in the context of this particular plugin

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'm not sure it's worth investing time into angular code that will be removed soon anyway. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I guess what I meant was, will it make it easier to remove angular code later if it is scoped more granularly now, especially since we are already changing these names now?

Definitely not worth investing the time if it won't help us later -- I was only suggesting it if you think it will save us time trying to figure out what's up with all of the various app/data references later.


app.directive('queryBar', (reactDirective, localStorage) => {
return reactDirective(
wrapInI18nContext(QueryBar),
undefined,
{},
{
store: localStorage,
}
);
});
export function setupDirective() {
app.directive('queryBar', (reactDirective, localStorage) => {
return reactDirective(
wrapInI18nContext(QueryBar),
undefined,
{},
{
store: localStorage,
}
);
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,6 @@
* under the License.
*/

export { QueryBar } from './components';
export { QueryBarService } from './query_bar_service';

export { QueryBar } from './components';
50 changes: 50 additions & 0 deletions src/legacy/core_plugins/data/public/query_bar/query_bar_service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* 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 { QueryBar } from './components/query_bar';
import { fromUser } from './lib/from_user';
import { toUser } from './lib/to_user';

// @ts-ignore
import { setupDirective } from './directive';

/**
* Search Bar Service
*
* @internal
*/
export class QueryBarService {
public setup() {
setupDirective();
return {
helpers: {
fromUser,
toUser,
},
QueryBar,
};
}

public stop() {
// nothing to do here yet
}
}

/** @public */
export type QueryBarSetup = ReturnType<QueryBarService['setup']>;
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ import React, { Component } from 'react';
import ResizeObserver from 'resize-observer-polyfill';
import { FilterBar } from 'ui/filter_bar';
import { IndexPattern } from 'ui/index_patterns';
import { QueryBar } from 'ui/query_bar';
import { Storage } from 'ui/storage';

import { QueryBar } from '../../query_bar';

interface Query {
query: string;
language: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@

import 'ngreact';
import { wrapInI18nContext } from 'ui/i18n';
import { uiModules } from '../../modules';
import { uiModules } from 'ui/modules';
import { SearchBar } from '../components';

const app = uiModules.get('app/kibana', ['react']);
const app = uiModules.get('app/data', ['react']);

app.directive('searchBar', (reactDirective, localStorage) => {
return reactDirective(
wrapInI18nContext(SearchBar),
undefined,
{},
{
store: localStorage,
}
);
});
export function setupDirective() {
app.directive('searchBar', (reactDirective, localStorage) => {
return reactDirective(
wrapInI18nContext(SearchBar),
undefined,
{},
{
store: localStorage,
}
);
});
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,4 @@
* under the License.
*/

import './directive';

export { SearchBar } from './components';
export { SearchBarService } from './search_bar_service';
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* 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 { SearchBar } from './components/search_bar';

// @ts-ignore
import { setupDirective } from './directive';

/**
* Search Bar Service
*
* @internal
*/
export class SearchBarService {
public setup() {
setupDirective();
return {
SearchBar,
};
}

public stop() {
// nothing to do here yet
}
}

/** @public */
export type SearchBarSetup = ReturnType<SearchBarService['setup']>;
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import { wrapInI18nContext } from 'ui/i18n';
import { toastNotifications } from 'ui/notify';

import 'ui/listen';
import 'ui/search_bar';
import 'ui/apply_filters';

import { panelActionsStore } from './store/panel_actions_store';
Expand Down Expand Up @@ -59,6 +58,8 @@ import { getUnhashableStatesProvider } from 'ui/state_management/state_hashing';

import { DashboardViewportProvider } from './viewport/dashboard_viewport_provider';

import 'plugins/data';
Copy link
Member

Choose a reason for hiding this comment

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

So I assume these imports are here because this code needs the directives that are wrapped in setupDirective to have been called already?

In making this change we are losing the context that search_bar is actually what's required by this particular app. This could make life more difficult later when we go to tear out Angular, as it won't be as obvious what exactly the dependencies are... it could be anything in plugins/data. This is a case I hadn't considered before.

Thinking out loud; what if, instead of calling setupDirective() from within the particular service, we exported it as a function from each service (and only allowed it to be called once)?

// search_bar_service.ts
setup() {
  return {
    SearchBar,
    loadLegacyDirectives: _.once(setupDirective)
  }
}

// dashboard_app.js
import { data } from 'plugins/data';

data.search.loadLegacyDirectives();

The first module that calls loadLegacyDirectives() would actually ensure setupDirective() is executed, and subsequent calls would do nothing; just serve the purpose of making it clear exactly what the module depends on.

There are other ways we could go about this too -- I am mostly interested in what you think about how we can make these dependencies more explicit.

Copy link
Contributor Author

@lizozom lizozom Apr 21, 2019

Choose a reason for hiding this comment

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

I was also bothered by the implicit import here.
Love your suggestion!

However, there are going to be multiple loadLegacyDirectives in many files.
What do you think about a convenience method on the service itself?
It's still implicit about which plugins are initialized, but at least the initialization itself is explicit.

import { data } from 'plugins/data';
data.loadLegacyDirectives();

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that would work too - the only downside I can think of is that by making the services implicit, we'll just have to reverse-engineer all references to data.loadLegacyDirectives() later to figure out exactly what services' directives a particular module is relying on.

If we are already doing the work today of pulling directives into specific services and updating downstream imports, keeping everything explicit might save us from more work later (avoiding an "autoload" type situation where you don't know what a module's true dependencies are).

But I'd defer to you on this -- you've spent much more time dealing with various legacy angular directives at this point 😃

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 found a nice (and necessary) middle ground, which you can see in the filter bar PR
https://github.com/elastic/kibana/pull/35460/files#diff-23034bf5cdc67a7321dbfb230edacd84


const app = uiModules.get('app/dashboard', [
'elasticsearch',
'ngRoute',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import 'ui/fixed_scroll';
import 'ui/index_patterns';
import 'ui/state_management/app_state';
import { timefilter } from 'ui/timefilter';
import 'ui/search_bar';
import { hasSearchStategyForIndexPattern, isDefaultTypeIndexPattern } from 'ui/courier';
import { toastNotifications } from 'ui/notify';
import { VisProvider } from 'ui/vis';
Expand Down Expand Up @@ -70,6 +69,9 @@ import { getRootBreadcrumbs, getSavedSearchBreadcrumbs } from '../breadcrumbs';
import { buildVislibDimensions } from 'ui/visualize/loader/pipeline_helpers/build_pipeline';
import 'ui/capabilities/route_setup';

import 'plugins/data';


const fetchStatuses = {
UNINITIALIZED: 'uninitialized',
LOADING: 'loading',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@ import './visualization_editor';
import 'ui/vis/editors/default/sidebar';
import 'ui/visualize';
import 'ui/collapsible_sidebar';
import 'ui/query_bar';
import { uiCapabilities } from 'ui/capabilities';
import 'ui/search_bar';
import 'ui/apply_filters';
import 'ui/listen';
import chrome from 'ui/chrome';
Expand Down Expand Up @@ -55,6 +53,8 @@ import { showSaveModal } from 'ui/saved_objects/show_saved_object_save_modal';
import { SavedObjectSaveModal } from 'ui/saved_objects/components/saved_object_save_modal';
import { getEditBreadcrumbs, getCreateBreadcrumbs } from '../breadcrumbs';

import 'plugins/data';

uiRoutes
.when(VisualizeConstants.CREATE_PATH, {
template: editorTemplate,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { Server } from '../../server/kbn_server';
export type InitPluginFunction = (server: Server) => void;
export interface UiExports {
injectDefaultVars: (server: Server) => { [key: string]: any };
styleSheetPaths?: string;
}

export interface PluginSpecOptions {
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/ui/public/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
// @import './timepicker/index';
@import './markdown/index';
@import './notify/index';
@import './query_bar/index';
// @import './query_bar/index';
@import './share/index';
@import './filter_bar/index';
@import './style_compile/index';
Expand Down
3 changes: 2 additions & 1 deletion src/legacy/ui/public/agg_types/directives/parse_query.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
* under the License.
*/

import { toUser, fromUser } from '../../query_bar/lib';
import { data } from 'plugins/data';
const { toUser, fromUser } = data.helpers;
import { uiModules } from '../../modules';

uiModules
Expand Down
2 changes: 1 addition & 1 deletion src/legacy/ui/public/filter_bar/filter_bar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ import classNames from 'classnames';
import React, { Component } from 'react';
import chrome from 'ui/chrome';
import { IndexPattern } from 'ui/index_patterns';
import { FilterOptions } from 'ui/search_bar/components/filter_options';
import { FilterEditor } from './filter_editor';
import { FilterItem } from './filter_item';
import { FilterOptions } from './filter_options';

const config = chrome.getUiSettingsClient();

Expand Down
3 changes: 2 additions & 1 deletion tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@
"kibana": ["./kibana"],
"kibana/public": ["src/core/public"],
"kibana/server": ["src/core/server"],
"plugins/*": ["src/legacy/core_plugins/*/public/"],
"ui/*": [
"src/legacy/ui/public/*"
],
"test_utils/*": [
"src/test_utils/public/*"
]
],
},
// Support .tsx files and transform JSX into calls to React.createElement
"jsx": "react",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ import {

import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import { QueryBar } from 'ui/query_bar';
import { indexPatternService } from '../../../kibana_services';
import { Storage } from 'ui/storage';

import { data } from 'plugins/data';
const { QueryBar } = data;

const settings = chrome.getUiSettingsClient();
const localStorage = new Storage(window.localStorage);

Expand Down
Loading