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.

Sign up for GitHub

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

Jump to bottom

Fix agg type shims and move paginated table to kibana_legacy #57695

Closed
wants to merge 6 commits into from
Closed

Fix agg type shims and move paginated table to kibana_legacy #57695

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7eed77e
fix agg type shims and move paginated table to kibana_legacy
flash1293 Feb 14, 2020
3e992c2
Merge remote-tracking branch 'upstream/master' into fix-visualize-shims
flash1293 Feb 17, 2020
f880e2d
fix types
flash1293 Feb 17, 2020
7abddc4
fix i18n ids
flash1293 Feb 17, 2020
555e19e
fix unit tests
flash1293 Feb 17, 2020
d70fac3
Merge remote-tracking branch 'upstream/master' into fix-visualize-shims
flash1293 Feb 18, 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
2 changes: 2 additions & 0 deletions src/legacy/core_plugins/kibana/public/visualize/kibana_services.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@ import { VisualizationsStart } from '../../../visualizations/public';
import { SavedVisualizations } from './np_ready/types';
import { UsageCollectionSetup } from '../../../../../plugins/usage_collection/public';
import { KibanaLegacyStart } from '../../../../../plugins/kibana_legacy/public';
import { DataStart } from '../../../data/public';

export interface VisualizeKibanaServices {
pluginInitializerContext: PluginInitializerContext;
addBasePath: (url: string) => string;
chrome: ChromeStart;
core: CoreStart;
data: DataPublicPluginStart;
dataShim: DataStart;
embeddable: IEmbeddableStart;
getBasePath: () => string;
indexPatterns: IndexPatternsContract;
Expand Down
2 changes: 2 additions & 0 deletions src/legacy/core_plugins/kibana/public/visualize/legacy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@ import { PluginInitializerContext } from 'kibana/public';
import { npSetup, npStart } from 'ui/new_platform';
import { start as visualizations } from '../../../visualizations/public/np_ready/public/legacy';
import { plugin } from './index';
import { start as dataShim } from '../../../data/public/legacy';

const instance = plugin({
env: npSetup.plugins.kibanaLegacy.env,
} as PluginInitializerContext);
instance.setup(npSetup.core, npSetup.plugins);
instance.start(npStart.core, {
...npStart.plugins,
dataShim,
visualizations,
});
1 change: 1 addition & 0 deletions src/legacy/core_plugins/kibana/public/visualize/np_ready/editor/visualization_editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export function initVisEditorDirective(app, deps) {
editor.render({
core: deps.core,
data: deps.data,
dataShim: deps.dataShim,
embeddable: deps.embeddable,
uiState: $scope.uiState,
timeRange: $scope.timeRange,
Expand Down
2 changes: 2 additions & 0 deletions src/legacy/core_plugins/kibana/public/visualize/np_ready/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ import { TimeRange, Query, Filter, DataPublicPluginStart } from 'src/plugins/dat
import { IEmbeddableStart } from 'src/plugins/embeddable/public';
import { LegacyCoreStart } from 'kibana/public';
import { VisSavedObject, AppState, PersistedState } from '../legacy_imports';
import { DataStart } from '../../../../data/public';

export interface EditorRenderProps {
appState: AppState;
core: LegacyCoreStart;
data: DataPublicPluginStart;
dataShim: DataStart;
embeddable: IEmbeddableStart;
filters: Filter[];
uiState: PersistedState;
Expand Down
15 changes: 14 additions & 1 deletion src/legacy/core_plugins/kibana/public/visualize/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,11 @@ import {
HomePublicPluginSetup,
} from '../../../../../plugins/home/public';
import { UsageCollectionSetup } from '../../../../../plugins/usage_collection/public';
import { DataStart } from '../../../data/public';

export interface VisualizePluginStartDependencies {
data: DataPublicPluginStart;
dataShim: DataStart;
embeddable: IEmbeddableStart;
navigation: NavigationStart;
share: SharePluginStart;
Expand All @@ -69,6 +71,7 @@ export interface VisualizePluginSetupDependencies {
export class VisualizePlugin implements Plugin {
private startDependencies: {
data: DataPublicPluginStart;
dataShim: DataStart;
embeddable: IEmbeddableStart;
navigation: NavigationStart;
savedObjectsClient: SavedObjectsClientContract;
Expand Down Expand Up @@ -123,6 +126,7 @@ export class VisualizePlugin implements Plugin {
embeddable,
navigation,
visualizations,
dataShim,
data: dataStart,
share,
} = this.startDependencies;
Expand All @@ -132,6 +136,7 @@ export class VisualizePlugin implements Plugin {
addBasePath: coreStart.http.basePath.prepend,
core: coreStart,
chrome: coreStart.chrome,
dataShim,
data: dataStart,
embeddable,
getBasePath: core.http.basePath.get,
Expand Down Expand Up @@ -178,10 +183,18 @@ export class VisualizePlugin implements Plugin {

public start(
core: CoreStart,
{ embeddable, navigation, data, share, visualizations }: VisualizePluginStartDependencies
{
embeddable,
navigation,
data,
dataShim,
share,
visualizations,
}: VisualizePluginStartDependencies
) {
this.startDependencies = {
data,
dataShim,
embeddable,
navigation,
savedObjectsClient: core.savedObjects.client,
Expand Down
17 changes: 17 additions & 0 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,23 @@ jest.mock('./agg_select', () => ({
jest.mock('./agg_param', () => ({
DefaultEditorAggParam: () => null,
}));
jest.mock('../../../../../plugins/kibana_react/public', () => ({
useKibana: () => ({
services: {
dataShim: {
search: {
aggs: {
types: {},
aggTypeFieldFilters: {
filter: () => [],
},
},
},
},
},
}),
withKibana: (x: any) => x,
}));

describe('DefaultEditorAggParams component', () => {
let setAggParamValue: jest.Mock;
Expand Down
30 changes: 19 additions & 11 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_params.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import {
} from './agg_params_state';
import { DefaultEditorCommonProps } from './agg_common_props';
import { EditorParamConfig, TimeIntervalParam, FixedParam, getEditorConfig } from './utils';
import { useKibana } from '../../../../../plugins/kibana_react/public';
import { VisDefaultEditorKibanaServices } from '../types';

const FIXED_VALUE_PROP = 'fixedValue';
const DEFAULT_PROP = 'default';
Expand Down Expand Up @@ -76,11 +78,19 @@ function DefaultEditorAggParams({
setTouched,
setValidity,
}: DefaultEditorAggParamsProps) {
const groupedAggTypeOptions = useMemo(() => getAggTypeOptions(agg, indexPattern, groupName), [
agg,
indexPattern,
groupName,
]);
const {
services: {
dataShim: {
search: {
aggs: { types: aggTypes, aggTypeFieldFilters },
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about getting rid of aggTypeFieldFilters in the data search services?
The only thing it is responsible for is adding a filter in rollup and using it in the default editor.
This is pretty the same thing as we've already got rid of editorConfig registry in #56501
This is maybe a question to @lukeelmers

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 agree, this would simplify the setup as more stuff becomes static. We still need a stateful plugin for types to keep it extensible, but it takes away complexity. Also cc @ppisljar

Have there been thoughts about this already?

Copy link
Member

Choose a reason for hiding this comment

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

I'm currently working out the long term interface for agg types in #57064, which I'm hoping should be ready to merge by next week.

So far the items we will for sure be keeping in the lifecycle contract are types (registry of agg types) and createAggConfigs (a function that returns a new AggConfigs(), which will make the AggConfigs class itself internal to the data plugin).

Everything else is temporarily under a __LEGACY namespace while work out whether they are required or if they can be relocated/removed. For example, aggTypeFieldFilters is currently data.search.aggs.__LEGACY.aggTypeFieldFilters, but I agree @sulemanof, it may not be necessary to keep at all.

#57064 will be the best place to follow along, as many of these details should be sorted out in the coming days. Would welcome any input you have on that @sulemanof @flash1293 ... I'll probably add you as reviewers once I get it wrapped up 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it's not urgent I will postpone work on this PR till @lukeelmer's is merged. @kertal I will ping you once it becomes ready again :)

Copy link
Member

Choose a reason for hiding this comment

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

Just as an update here: #58462 and #58805 have both been merged this week, and I'm actively working now on the PR to cutover agg types to the new platform.

Since doing the cutover is the highest priority in terms of unblocking teams, we decided to postpone sorting out aggTypeFieldFilters and some of the other items under __LEGACY as it should be possible to address them after the service is in the new platform, since they are no longer relying on legacy world functionality.

As we clean up those APIs we will upstream affected downstream applications accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, as there are some parts that definitely will stay stateful anyway it's a very small change to make later on.

},
},
},
} = useKibana<VisDefaultEditorKibanaServices>();
const groupedAggTypeOptions = useMemo(
() => getAggTypeOptions(aggTypes, agg, indexPattern, groupName),
[agg, indexPattern, groupName, aggTypes]
);
const error = aggIsTooLow
? i18n.translate('visDefaultEditor.aggParams.errors.aggWrongRunOrderErrorMessage', {
defaultMessage: '"{schema}" aggs must run before all other buckets!',
Expand All @@ -94,12 +104,10 @@ function DefaultEditorAggParams({
aggTypeName,
fieldName,
]);
const params = useMemo(() => getAggParamsToRender({ agg, editorConfig, metricAggs, state }), [
agg,
editorConfig,
metricAggs,
state,
]);
const params = useMemo(
() => getAggParamsToRender({ agg, editorConfig, metricAggs, state }, aggTypeFieldFilters),
[agg, editorConfig, metricAggs, state, aggTypeFieldFilters]
);
const allParams = [...params.basic, ...params.advanced];
const [paramsState, onChangeParamsState] = useReducer(
aggParamsReducer,
Expand Down
20 changes: 13 additions & 7 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { IndexPattern } from 'src/plugins/data/public';
import { IndexPattern, IndexPatternField } from 'src/plugins/data/public';
import { VisState } from 'src/legacy/core_plugins/visualizations/public';
import { IAggConfig, IAggType, AggGroupNames, BUCKET_TYPES } from '../legacy_imports';
import {
Expand All @@ -34,6 +34,12 @@ jest.mock('../utils', () => ({

jest.mock('ui/new_platform');

const mockFilter = {
filter(fields: IndexPatternField[]): IndexPatternField[] {
return fields;
},
};

describe('DefaultEditorAggParams helpers', () => {
describe('getAggParamsToRender', () => {
let agg: IAggConfig;
Expand All @@ -52,14 +58,14 @@ describe('DefaultEditorAggParams helpers', () => {
},
schema: {},
} as IAggConfig;
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state });
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state }, mockFilter);

expect(params).toEqual(emptyParams);
});

it('should not create any param if there is no agg type', () => {
agg = {} as IAggConfig;
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state });
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state }, mockFilter);

expect(params).toEqual(emptyParams);
});
Expand All @@ -75,7 +81,7 @@ describe('DefaultEditorAggParams helpers', () => {
hidden: true,
},
};
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state });
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state }, mockFilter);

expect(params).toEqual(emptyParams);
});
Expand All @@ -89,7 +95,7 @@ describe('DefaultEditorAggParams helpers', () => {
hideCustomLabel: true,
},
} as IAggConfig;
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state });
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state }, mockFilter);

expect(params).toEqual(emptyParams);
});
Expand Down Expand Up @@ -128,7 +134,7 @@ describe('DefaultEditorAggParams helpers', () => {
field: 'field',
},
} as any) as IAggConfig;
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state });
const params = getAggParamsToRender({ agg, editorConfig, metricAggs, state }, mockFilter);

expect(params).toEqual({
basic: [
Expand Down Expand Up @@ -162,7 +168,7 @@ describe('DefaultEditorAggParams helpers', () => {
describe('getAggTypeOptions', () => {
it('should return agg type options grouped by subtype', () => {
const indexPattern = {} as IndexPattern;
const aggs = getAggTypeOptions({} as IAggConfig, indexPattern, 'metrics');
const aggs = getAggTypeOptions({ metrics: [] }, {} as IAggConfig, indexPattern, 'metrics');

expect(aggs).toEqual(['indexedFields']);
});
Expand Down
19 changes: 9 additions & 10 deletions src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,9 @@ import { groupAndSortBy, ComboBoxGroupedOptions } from '../utils';
import { AggTypeState, AggParamsState } from './agg_params_state';
import { AggParamEditorProps } from './agg_param_props';
import { aggParamsMap } from './agg_params_map';
import {
aggTypeFilters,
aggTypeFieldFilters,
aggTypes,
IAggConfig,
AggParam,
IFieldParamType,
IAggType,
} from '../legacy_imports';
import { aggTypeFilters, IAggConfig, AggParam, IFieldParamType, IAggType } from '../legacy_imports';
import { EditorConfig } from './utils';
import { AggConfig } from '../../../data/public/search/aggs';

interface ParamInstanceBase {
agg: IAggConfig;
Expand All @@ -50,7 +43,12 @@ export interface ParamInstance extends ParamInstanceBase {
value: unknown;
}

function getAggParamsToRender({ agg, editorConfig, metricAggs, state }: ParamInstanceBase) {
function getAggParamsToRender(
{ agg, editorConfig, metricAggs, state }: ParamInstanceBase,
aggTypeFieldFilters: {
filter(fields: IndexPatternField[], aggConfig: AggConfig): IndexPatternField[];
}
) {
const params = {
basic: [] as ParamInstance[],
advanced: [] as ParamInstance[],
Expand Down Expand Up @@ -117,6 +115,7 @@ function getAggParamsToRender({ agg, editorConfig, metricAggs, state }: ParamIns
}

function getAggTypeOptions(
aggTypes: any,
agg: IAggConfig,
indexPattern: IndexPattern,
groupName: string
Expand Down
5 changes: 4 additions & 1 deletion src/legacy/core_plugins/vis_default_editor/public/components/sidebar/state/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,15 @@ import { Vis, VisState, VisParams } from 'src/legacy/core_plugins/visualizations
import { editorStateReducer, initEditorState } from './reducers';
import { EditorStateActionTypes } from './constants';
import { EditorAction, updateStateParams } from './actions';
import { useKibana } from '../../../../../../../plugins/kibana_react/public';
import { VisDefaultEditorKibanaServices } from '../../../types';

export * from './editor_form_state';
export * from './actions';

export function useEditorReducer(vis: Vis): [VisState, React.Dispatch<EditorAction>] {
const [state, dispatch] = useReducer(editorStateReducer, vis, initEditorState);
const { services } = useKibana<VisDefaultEditorKibanaServices>();
const [state, dispatch] = useReducer(editorStateReducer(services.dataShim), vis, initEditorState);

useEffect(() => {
const handleVisUpdate = (params: VisParams) => {
Expand Down
11 changes: 8 additions & 3 deletions src/legacy/core_plugins/vis_default_editor/public/components/sidebar/state/reducers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,21 @@
import { cloneDeep } from 'lodash';

import { Vis, VisState } from 'src/legacy/core_plugins/visualizations/public';
import { AggConfigs, IAggConfig, AggGroupNames } from '../../../legacy_imports';
import { IAggConfig, AggGroupNames } from '../../../legacy_imports';
import { EditorStateActionTypes } from './constants';
import { getEnabledMetricAggsCount } from '../../agg_group_helper';
import { EditorAction } from './actions';
import { DataStart } from '../../../../../data/public';

function initEditorState(vis: Vis) {
return vis.copyCurrentState(true);
}

function editorStateReducer(state: VisState, action: EditorAction): VisState {
const editorStateReducer = ({
search: {
aggs: { AggConfigs },
},
}: DataStart) => (state: VisState, action: EditorAction): VisState => {
switch (action.type) {
case EditorStateActionTypes.ADD_NEW_AGG: {
const aggConfig = state.aggs.createAggConfig(action.payload as IAggConfig, {
Expand Down Expand Up @@ -176,6 +181,6 @@ function editorStateReducer(state: VisState, action: EditorAction): VisState {
};
}
}
}
};

export { editorStateReducer, initEditorState };
2 changes: 1 addition & 1 deletion src/legacy/core_plugins/vis_default_editor/public/default_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function DefaultEditor({
appState,
optionTabs,
query,
}: DefaultEditorControllerState & Omit<EditorRenderProps, 'data' | 'core'>) {
}: DefaultEditorControllerState & Omit<EditorRenderProps, 'data' | 'dataShim' | 'core'>) {
const visRef = useRef<HTMLDivElement>(null);
const visHandler = useRef<VisualizeEmbeddable | null>(null);
const [isCollapsed, setIsCollapsed] = useState(false);
Expand Down
3 changes: 2 additions & 1 deletion src/legacy/core_plugins/vis_default_editor/public/default_editor_controller.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,15 @@ class DefaultEditorController {
};
}

render({ data, core, ...props }: EditorRenderProps) {
render({ data, dataShim, core, ...props }: EditorRenderProps) {
render(
<I18nProvider>
<KibanaContextProvider
services={{
appName: 'vis_default_editor',
storage: localStorage,
data,
dataShim,
...core,
}}
>
Expand Down
Loading