From 638792a5571d79ee26744b91a047eb4857e203b0 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Mon, 20 Jan 2020 17:24:17 +0000 Subject: [PATCH 1/6] removes CTA from Task Manager info message (#55334) removes CTA from Task Manager info message --- x-pack/plugins/task_manager/server/task_manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/task_manager/server/task_manager.ts b/x-pack/plugins/task_manager/server/task_manager.ts index 93e98f33a30b0..da9640fa3e071 100644 --- a/x-pack/plugins/task_manager/server/task_manager.ts +++ b/x-pack/plugins/task_manager/server/task_manager.ts @@ -401,7 +401,7 @@ export async function claimAvailableTasks( } else { performance.mark('claimAvailableTasks.noAvailableWorkers'); logger.info( - `[Task Ownership]: Task Manager has skipped Claiming Ownership of available tasks at it has ran out Available Workers. If this happens often, consider adjusting the "xpack.task_manager.max_workers" configuration.` + `[Task Ownership]: Task Manager has skipped Claiming Ownership of available tasks at it has ran out Available Workers.` ); } return []; From cdb0021ac6bdcc162232a5453feebd1c3693c6d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Fern=C3=A1ndez?= Date: Mon, 20 Jan 2020 18:28:55 +0100 Subject: [PATCH 2/6] [Logs UI] Fix z-index of logs page toolbar (#54469) * Fix z-index of logs page toolbar * Extract `FixedDatePicker` from log setup page, and use it in the stream page * Clean unused import Co-authored-by: Elastic Machine --- .../public/components/fixed_datepicker.tsx | 25 +++++++++++++++++++ .../analysis_setup_timerange_form.tsx | 20 +-------------- .../components/logging/log_time_controls.tsx | 3 ++- 3 files changed, 28 insertions(+), 20 deletions(-) create mode 100644 x-pack/legacy/plugins/infra/public/components/fixed_datepicker.tsx diff --git a/x-pack/legacy/plugins/infra/public/components/fixed_datepicker.tsx b/x-pack/legacy/plugins/infra/public/components/fixed_datepicker.tsx new file mode 100644 index 0000000000000..aab1bcd1da873 --- /dev/null +++ b/x-pack/legacy/plugins/infra/public/components/fixed_datepicker.tsx @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; + +import { EuiDatePicker, EuiDatePickerProps } from '@elastic/eui'; +import euiStyled from '../../../../common/eui_styled_components'; + +export const FixedDatePicker = euiStyled( + ({ + className, + inputClassName, + ...datePickerProps + }: { + className?: string; + inputClassName?: string; + } & EuiDatePickerProps) => ( + + ) +)` + z-index: 3 !important; +`; diff --git a/x-pack/legacy/plugins/infra/public/components/logging/log_analysis_setup/initial_configuration_step/analysis_setup_timerange_form.tsx b/x-pack/legacy/plugins/infra/public/components/logging/log_analysis_setup/initial_configuration_step/analysis_setup_timerange_form.tsx index 4319f844b1dcc..02119fd1c09dd 100644 --- a/x-pack/legacy/plugins/infra/public/components/logging/log_analysis_setup/initial_configuration_step/analysis_setup_timerange_form.tsx +++ b/x-pack/legacy/plugins/infra/public/components/logging/log_analysis_setup/initial_configuration_step/analysis_setup_timerange_form.tsx @@ -5,8 +5,6 @@ */ import { - EuiDatePicker, - EuiDatePickerProps, EuiDescribedFormGroup, EuiFlexGroup, EuiFormControlLayout, @@ -16,8 +14,7 @@ import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import moment, { Moment } from 'moment'; import React, { useMemo } from 'react'; - -import { euiStyled } from '../../../../../../../common/eui_styled_components'; +import { FixedDatePicker } from '../../../fixed_datepicker'; const startTimeLabel = i18n.translate('xpack.infra.analysisSetup.startTimeLabel', { defaultMessage: 'Start time', @@ -138,18 +135,3 @@ export const AnalysisSetupTimerangeForm: React.FunctionComponent<{ ); }; - -const FixedDatePicker = euiStyled( - ({ - className, - inputClassName, - ...datePickerProps - }: { - className?: string; - inputClassName?: string; - } & EuiDatePickerProps) => ( - - ) -)` - z-index: 3 !important; -`; diff --git a/x-pack/legacy/plugins/infra/public/components/logging/log_time_controls.tsx b/x-pack/legacy/plugins/infra/public/components/logging/log_time_controls.tsx index 8f5705a9b9c56..5095edd4c715c 100644 --- a/x-pack/legacy/plugins/infra/public/components/logging/log_time_controls.tsx +++ b/x-pack/legacy/plugins/infra/public/components/logging/log_time_controls.tsx @@ -9,6 +9,7 @@ import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import moment, { Moment } from 'moment'; import React from 'react'; +import { FixedDatePicker } from '../fixed_datepicker'; const noop = () => undefined; @@ -56,7 +57,7 @@ export class LogTimeControls extends React.PureComponent { return ( - Date: Tue, 21 Jan 2020 14:27:51 +0530 Subject: [PATCH 3/6] [Mappings editor] Add missing max_shingle_size parameter to search_as_you_type (#55161) --- .../document_fields/field_parameters/index.ts | 2 + .../max_shingle_size_parameter.tsx | 45 +++++++++++++++++++ .../fields/field_types/search_as_you_type.tsx | 8 +++- .../constants/parameters_definition.tsx | 11 +++++ .../lib/mappings_validator.test.ts | 2 + .../app/components/mappings_editor/types.ts | 3 +- 6 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/index.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/index.ts index 9622466ad795c..b248776c884f1 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/index.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/index.ts @@ -51,3 +51,5 @@ export * from './fielddata_parameter'; export * from './split_queries_on_whitespace_parameter'; export * from './locale_parameter'; + +export * from './max_shingle_size_parameter'; diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx new file mode 100644 index 0000000000000..bc1917b2da966 --- /dev/null +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/field_parameters/max_shingle_size_parameter.tsx @@ -0,0 +1,45 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; + +import { i18n } from '@kbn/i18n'; + +import { getFieldConfig } from '../../../lib'; +import { EditFieldFormRow } from '../fields/edit_field'; +import { UseField, Field } from '../../../shared_imports'; + +interface Props { + defaultToggleValue: boolean; +} + +export const MaxShingleSizeParameter = ({ defaultToggleValue }: Props) => ( + + + +); diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/fields/field_types/search_as_you_type.tsx b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/fields/field_types/search_as_you_type.tsx index 83541ec982ee6..dafbebd24b3fa 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/fields/field_types/search_as_you_type.tsx +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/components/document_fields/fields/field_types/search_as_you_type.tsx @@ -14,6 +14,7 @@ import { NormsParameter, SimilarityParameter, TermVectorParameter, + MaxShingleSizeParameter, } from '../../field_parameters'; import { BasicParametersSection, AdvancedParametersSection } from '../edit_field'; @@ -24,7 +25,8 @@ interface Props { const getDefaultToggleValue = (param: string, field: FieldType) => { switch (param) { case 'similarity': - case 'term_vector': { + case 'term_vector': + case 'max_shingle_size': { return field[param] !== undefined && field[param] !== getFieldConfig(param).defaultValue; } case 'analyzers': { @@ -47,6 +49,10 @@ export const SearchAsYouType = React.memo(({ field }: Props) => { + + { enable_position_increments: [], depth_limit: true, dims: false, + max_shingle_size: 'string_not_allowed', }, // All the parameters in "goodField" have the correct format // and should still be there after the validation ran. @@ -307,6 +308,7 @@ describe('Properties validator', () => { enable_position_increments: true, depth_limit: 20, dims: 'abc', + max_shingle_size: 2, }, }; diff --git a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/types.ts b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/types.ts index 0fce3422344bc..b7bf4e6b112d3 100644 --- a/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/types.ts +++ b/x-pack/legacy/plugins/index_management/public/app/components/mappings_editor/types.ts @@ -119,7 +119,8 @@ export type ParameterName = | 'points_only' | 'path' | 'dims' - | 'depth_limit'; + | 'depth_limit' + | 'max_shingle_size'; export interface Parameter { fieldConfig: FieldConfig; From 4ca2fbdb114a2e43db740eb8192857aa0981ca83 Mon Sep 17 00:00:00 2001 From: Liza Katz Date: Tue, 21 Jan 2020 11:03:55 +0200 Subject: [PATCH 4/6] [BUG] Data fetching twice on discover timefilter change (#55279) * Fix bug #54887 - Filters are not only fetch once on timefilter change - Make sure that discover doesn't fetch data when a disabled filter is changed - Support compareFilters on an array of filters. - Added tests to compare filters - Exctracted sortFilters and added tests to it. * code review + FilterCompareOptions * Remove sort by Co-authored-by: Elastic Machine --- .../filter_manager/filter_state_manager.ts | 19 ++- .../discover/np_ready/angular/discover.js | 2 +- .../query/filter_manager/filter_manager.ts | 18 +-- .../lib/compare_filters.test.ts | 131 +++++++++++++++++- .../filter_manager/lib/compare_filters.ts | 78 ++++++++--- .../query/filter_manager/lib/dedup_filters.ts | 4 +- .../query/filter_manager/lib/only_disabled.ts | 5 +- .../filter_manager/lib/sort_filters.test.ts | 91 ++++++++++++ .../query/filter_manager/lib/sort_filters.ts | 42 ++++++ 9 files changed, 351 insertions(+), 39 deletions(-) create mode 100644 src/plugins/data/public/query/filter_manager/lib/sort_filters.test.ts create mode 100644 src/plugins/data/public/query/filter_manager/lib/sort_filters.ts diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts index 61821b7ad45e9..633b7e630700d 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts @@ -21,6 +21,13 @@ import _ from 'lodash'; import { State } from 'ui/state_management/state'; import { FilterManager, esFilters } from '../../../../../../plugins/data/public'; +import { + compareFilters, + COMPARE_ALL_OPTIONS, + // this whole file will soon be deprecated by new state management. + // eslint-disable-next-line @kbn/eslint/no-restricted-paths +} from '../../../../../../plugins/data/public/query/filter_manager/lib/compare_filters'; + type GetAppStateFunc = () => State | undefined | null; /** @@ -63,8 +70,16 @@ export class FilterStateManager { const globalFilters = this.globalState.filters || []; const appFilters = (appState && appState.filters) || []; - const globalFilterChanged = !_.isEqual(this.filterManager.getGlobalFilters(), globalFilters); - const appFilterChanged = !_.isEqual(this.filterManager.getAppFilters(), appFilters); + const globalFilterChanged = !compareFilters( + this.filterManager.getGlobalFilters(), + globalFilters, + COMPARE_ALL_OPTIONS + ); + const appFilterChanged = !compareFilters( + this.filterManager.getAppFilters(), + appFilters, + COMPARE_ALL_OPTIONS + ); const filterStateChanged = globalFilterChanged || appFilterChanged; if (!filterStateChanged) return; diff --git a/src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js b/src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js index 2927565e61dce..2bf554860f743 100644 --- a/src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js +++ b/src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js @@ -637,7 +637,7 @@ function discoverController( // fetch data when filters fire fetch event subscriptions.add( - subscribeWithScope($scope, filterManager.getUpdates$(), { + subscribeWithScope($scope, filterManager.getFetches$(), { next: $scope.fetch, }) ); diff --git a/src/plugins/data/public/query/filter_manager/filter_manager.ts b/src/plugins/data/public/query/filter_manager/filter_manager.ts index c25e5df2b168a..f7d0dddd5bf03 100644 --- a/src/plugins/data/public/query/filter_manager/filter_manager.ts +++ b/src/plugins/data/public/query/filter_manager/filter_manager.ts @@ -22,7 +22,8 @@ import { Subject } from 'rxjs'; import { IUiSettingsClient } from 'src/core/public'; -import { compareFilters } from './lib/compare_filters'; +import { compareFilters, COMPARE_ALL_OPTIONS } from './lib/compare_filters'; +import { sortFilters } from './lib/sort_filters'; import { mapAndFlattenFilters } from './lib/map_and_flatten_filters'; import { uniqFilters } from './lib/uniq_filters'; import { onlyDisabledFiltersChanged } from './lib/only_disabled'; @@ -76,20 +77,9 @@ export class FilterManager { } private handleStateUpdate(newFilters: esFilters.Filter[]) { - // global filters should always be first - - newFilters.sort(({ $state: a }: esFilters.Filter, { $state: b }: esFilters.Filter): number => { - if (a!.store === b!.store) { - return 0; - } else { - return a!.store === esFilters.FilterStateStore.GLOBAL_STATE && - b!.store !== esFilters.FilterStateStore.GLOBAL_STATE - ? -1 - : 1; - } - }); + newFilters.sort(sortFilters); - const filtersUpdated = !_.isEqual(this.filters, newFilters); + const filtersUpdated = !compareFilters(this.filters, newFilters, COMPARE_ALL_OPTIONS); const updatedOnlyDisabledFilters = onlyDisabledFiltersChanged(newFilters, this.filters); this.filters = newFilters; diff --git a/src/plugins/data/public/query/filter_manager/lib/compare_filters.test.ts b/src/plugins/data/public/query/filter_manager/lib/compare_filters.test.ts index 34fd662c4ba46..9cc5938750c4e 100644 --- a/src/plugins/data/public/query/filter_manager/lib/compare_filters.test.ts +++ b/src/plugins/data/public/query/filter_manager/lib/compare_filters.test.ts @@ -17,7 +17,7 @@ * under the License. */ -import { compareFilters } from './compare_filters'; +import { compareFilters, COMPARE_ALL_OPTIONS } from './compare_filters'; import { esFilters } from '../../../../common'; describe('filter manager utilities', () => { @@ -83,5 +83,134 @@ describe('filter manager utilities', () => { expect(compareFilters(f1, f2)).toBeTruthy(); }); + + test('should compare filters array to non array', () => { + const f1 = esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ); + + const f2 = esFilters.buildQueryFilter( + { _type: { match: { query: 'mochi', type: 'phrase' } } }, + 'index', + '' + ); + + expect(compareFilters([f1, f2], f1)).toBeFalsy(); + }); + + test('should compare filters array to partial array', () => { + const f1 = esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ); + + const f2 = esFilters.buildQueryFilter( + { _type: { match: { query: 'mochi', type: 'phrase' } } }, + 'index', + '' + ); + + expect(compareFilters([f1, f2], [f1])).toBeFalsy(); + }); + + test('should compare filters array to exact array', () => { + const f1 = esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ); + + const f2 = esFilters.buildQueryFilter( + { _type: { match: { query: 'mochi', type: 'phrase' } } }, + 'index', + '' + ); + + expect(compareFilters([f1, f2], [f1, f2])).toBeTruthy(); + }); + + test('should compare array of duplicates, ignoring meta attributes', () => { + const f1 = esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index1', + '' + ); + const f2 = esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index2', + '' + ); + + expect(compareFilters([f1], [f2])).toBeTruthy(); + }); + + test('should compare array of duplicates, ignoring $state attributes', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.APP_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + expect(compareFilters([f1], [f2])).toBeTruthy(); + }); + + test('should compare duplicates with COMPARE_ALL_OPTIONS should check store', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.APP_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + expect(compareFilters([f1], [f2], COMPARE_ALL_OPTIONS)).toBeFalsy(); + }); + + test('should compare duplicates with COMPARE_ALL_OPTIONS should not check key and value ', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + f2.meta.key = 'wassup'; + f2.meta.value = 'dog'; + + expect(compareFilters([f1], [f2], COMPARE_ALL_OPTIONS)).toBeTruthy(); + }); }); }); diff --git a/src/plugins/data/public/query/filter_manager/lib/compare_filters.ts b/src/plugins/data/public/query/filter_manager/lib/compare_filters.ts index 9b171ab0aacb2..218b9d492b61f 100644 --- a/src/plugins/data/public/query/filter_manager/lib/compare_filters.ts +++ b/src/plugins/data/public/query/filter_manager/lib/compare_filters.ts @@ -17,32 +17,64 @@ * under the License. */ -import { defaults, isEqual, omit } from 'lodash'; +import { defaults, isEqual, omit, map } from 'lodash'; import { esFilters } from '../../../../common'; +export interface FilterCompareOptions { + disabled?: boolean; + negate?: boolean; + state?: boolean; +} + +/** + * Include disabled, negate and store when comparing filters + */ +export const COMPARE_ALL_OPTIONS: FilterCompareOptions = { + disabled: true, + negate: true, + state: true, +}; + +const mapFilter = ( + filter: esFilters.Filter, + comparators: FilterCompareOptions, + excludedAttributes: string[] +) => { + const cleaned: esFilters.FilterMeta = omit(filter, excludedAttributes); + + if (comparators.negate) cleaned.negate = filter.meta && Boolean(filter.meta.negate); + if (comparators.disabled) cleaned.disabled = filter.meta && Boolean(filter.meta.disabled); + + return cleaned; +}; + +const mapFilterArray = ( + filters: esFilters.Filter[], + comparators: FilterCompareOptions, + excludedAttributes: string[] +) => { + return map(filters, (filter: esFilters.Filter) => + mapFilter(filter, comparators, excludedAttributes) + ); +}; + /** - * Compare two filters to see if they match + * Compare two filters or filter arrays to see if they match. + * For filter arrays, the assumption is they are sorted. * - * @param {object} first The first filter to compare - * @param {object} second The second filter to compare - * @param {object} comparatorOptions Parameters to use for comparison + * @param {esFilters.Filter | esFilters.Filter[]} first The first filter or filter array to compare + * @param {esFilters.Filter | esFilters.Filter[]} second The second filter or filter array to compare + * @param {FilterCompareOptions} comparatorOptions Parameters to use for comparison * * @returns {bool} Filters are the same */ export const compareFilters = ( - first: esFilters.Filter, - second: esFilters.Filter, - comparatorOptions: any = {} + first: esFilters.Filter | esFilters.Filter[], + second: esFilters.Filter | esFilters.Filter[], + comparatorOptions: FilterCompareOptions = {} ) => { - let comparators: any = {}; - const mapFilter = (filter: esFilters.Filter) => { - const cleaned: esFilters.FilterMeta = omit(filter, excludedAttributes); - - if (comparators.negate) cleaned.negate = filter.meta && Boolean(filter.meta.negate); - if (comparators.disabled) cleaned.disabled = filter.meta && Boolean(filter.meta.disabled); + let comparators: FilterCompareOptions = {}; - return cleaned; - }; const excludedAttributes: string[] = ['$$hashKey', 'meta']; comparators = defaults(comparatorOptions || {}, { @@ -53,5 +85,17 @@ export const compareFilters = ( if (!comparators.state) excludedAttributes.push('$state'); - return isEqual(mapFilter(first), mapFilter(second)); + if (Array.isArray(first) && Array.isArray(second)) { + return isEqual( + mapFilterArray(first, comparators, excludedAttributes), + mapFilterArray(second, comparators, excludedAttributes) + ); + } else if (!Array.isArray(first) && !Array.isArray(second)) { + return isEqual( + mapFilter(first, comparators, excludedAttributes), + mapFilter(second, comparators, excludedAttributes) + ); + } else { + return false; + } }; diff --git a/src/plugins/data/public/query/filter_manager/lib/dedup_filters.ts b/src/plugins/data/public/query/filter_manager/lib/dedup_filters.ts index 6dae14f480b4f..897a645e87b5a 100644 --- a/src/plugins/data/public/query/filter_manager/lib/dedup_filters.ts +++ b/src/plugins/data/public/query/filter_manager/lib/dedup_filters.ts @@ -18,7 +18,7 @@ */ import { filter, find } from 'lodash'; -import { compareFilters } from './compare_filters'; +import { compareFilters, FilterCompareOptions } from './compare_filters'; import { esFilters } from '../../../../common'; /** @@ -33,7 +33,7 @@ import { esFilters } from '../../../../common'; export const dedupFilters = ( existingFilters: esFilters.Filter[], filters: esFilters.Filter[], - comparatorOptions: any = {} + comparatorOptions: FilterCompareOptions = {} ) => { if (!Array.isArray(filters)) { filters = [filters]; diff --git a/src/plugins/data/public/query/filter_manager/lib/only_disabled.ts b/src/plugins/data/public/query/filter_manager/lib/only_disabled.ts index c040d2f2960c7..3f35c94a3f858 100644 --- a/src/plugins/data/public/query/filter_manager/lib/only_disabled.ts +++ b/src/plugins/data/public/query/filter_manager/lib/only_disabled.ts @@ -17,8 +17,9 @@ * under the License. */ -import { filter, isEqual } from 'lodash'; +import { filter } from 'lodash'; import { esFilters } from '../../../../common'; +import { compareFilters, COMPARE_ALL_OPTIONS } from './compare_filters'; const isEnabled = (f: esFilters.Filter) => f && f.meta && !f.meta.disabled; @@ -35,5 +36,5 @@ export const onlyDisabledFiltersChanged = ( const newEnabledFilters = filter(newFilters || [], isEnabled); const oldEnabledFilters = filter(oldFilters || [], isEnabled); - return isEqual(oldEnabledFilters, newEnabledFilters); + return compareFilters(oldEnabledFilters, newEnabledFilters, COMPARE_ALL_OPTIONS); }; diff --git a/src/plugins/data/public/query/filter_manager/lib/sort_filters.test.ts b/src/plugins/data/public/query/filter_manager/lib/sort_filters.test.ts new file mode 100644 index 0000000000000..949c57e43ce74 --- /dev/null +++ b/src/plugins/data/public/query/filter_manager/lib/sort_filters.test.ts @@ -0,0 +1,91 @@ +/* + * 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 { sortFilters } from './sort_filters'; +import { esFilters } from '../../../../common'; + +describe('sortFilters', () => { + describe('sortFilters()', () => { + test('Not sort two application level filters', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.APP_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.APP_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + const filters = [f1, f2].sort(sortFilters); + expect(filters[0]).toBe(f1); + }); + + test('Not sort two global level filters', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + const filters = [f1, f2].sort(sortFilters); + expect(filters[0]).toBe(f1); + }); + + test('Move global level filter to the beginning of the array', () => { + const f1 = { + $state: { store: esFilters.FilterStateStore.APP_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + const f2 = { + $state: { store: esFilters.FilterStateStore.GLOBAL_STATE }, + ...esFilters.buildQueryFilter( + { _type: { match: { query: 'apache', type: 'phrase' } } }, + 'index', + '' + ), + }; + + const filters = [f1, f2].sort(sortFilters); + expect(filters[0]).toBe(f2); + }); + }); +}); diff --git a/src/plugins/data/public/query/filter_manager/lib/sort_filters.ts b/src/plugins/data/public/query/filter_manager/lib/sort_filters.ts new file mode 100644 index 0000000000000..657c80fe0ccb2 --- /dev/null +++ b/src/plugins/data/public/query/filter_manager/lib/sort_filters.ts @@ -0,0 +1,42 @@ +/* + * 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 { esFilters } from '../../../../common'; + +/** + * Sort filters according to their store - global filters go first + * + * @param {object} first The first filter to compare + * @param {object} second The second filter to compare + * + * @returns {number} Sorting order of filters + */ +export const sortFilters = ( + { $state: a }: esFilters.Filter, + { $state: b }: esFilters.Filter +): number => { + if (a!.store === b!.store) { + return 0; + } else { + return a!.store === esFilters.FilterStateStore.GLOBAL_STATE && + b!.store !== esFilters.FilterStateStore.GLOBAL_STATE + ? -1 + : 1; + } +}; From 4971a2c772317ed8092ca8f35e539fe71b828b3b Mon Sep 17 00:00:00 2001 From: Mikhail Shustov Date: Tue, 21 Jan 2020 12:58:40 +0100 Subject: [PATCH 5/6] Expose fatalErrors API from the Start contract (#55300) * Expose FatalErrors from the Start contract. This API is intended to be used for runtime as well. * update docs * update data plugin snapshot to fix tests * address comments Co-authored-by: Elastic Machine --- ...ana-plugin-public.chromenavlinks.update.md | 60 +++++++++---------- ...ana-plugin-public.corestart.fatalerrors.md | 13 ++++ .../public/kibana-plugin-public.corestart.md | 1 + .../kibana-plugin-public.fatalerrorsstart.md | 13 ++++ .../core/public/kibana-plugin-public.md | 1 + src/core/public/core_system.ts | 2 + .../fatal_errors/fatal_errors_service.mock.ts | 4 ++ .../fatal_errors/fatal_errors_service.tsx | 23 ++++++- src/core/public/fatal_errors/index.ts | 2 +- src/core/public/index.ts | 5 +- src/core/public/legacy/legacy_service.test.ts | 2 + src/core/public/mocks.ts | 1 + src/core/public/plugins/plugin_context.ts | 1 + .../public/plugins/plugins_service.test.ts | 1 + src/core/public/public.api.md | 5 ++ .../query_string_input.test.tsx.snap | 24 ++++++++ 16 files changed, 123 insertions(+), 35 deletions(-) create mode 100644 docs/development/core/public/kibana-plugin-public.corestart.fatalerrors.md create mode 100644 docs/development/core/public/kibana-plugin-public.fatalerrorsstart.md diff --git a/docs/development/core/public/kibana-plugin-public.chromenavlinks.update.md b/docs/development/core/public/kibana-plugin-public.chromenavlinks.update.md index d1cd2d3b04950..155d149f334a1 100644 --- a/docs/development/core/public/kibana-plugin-public.chromenavlinks.update.md +++ b/docs/development/core/public/kibana-plugin-public.chromenavlinks.update.md @@ -1,30 +1,30 @@ - - -[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [ChromeNavLinks](./kibana-plugin-public.chromenavlinks.md) > [update](./kibana-plugin-public.chromenavlinks.update.md) - -## ChromeNavLinks.update() method - -> Warning: This API is now obsolete. -> -> Uses the [AppBase.updater$](./kibana-plugin-public.appbase.updater_.md) property when registering your application with [ApplicationSetup.register()](./kibana-plugin-public.applicationsetup.register.md) instead. -> - -Update the navlink for the given id with the updated attributes. Returns the updated navlink or `undefined` if it does not exist. - -Signature: - -```typescript -update(id: string, values: ChromeNavLinkUpdateableFields): ChromeNavLink | undefined; -``` - -## Parameters - -| Parameter | Type | Description | -| --- | --- | --- | -| id | string | | -| values | ChromeNavLinkUpdateableFields | | - -Returns: - -`ChromeNavLink | undefined` - + + +[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [ChromeNavLinks](./kibana-plugin-public.chromenavlinks.md) > [update](./kibana-plugin-public.chromenavlinks.update.md) + +## ChromeNavLinks.update() method + +> Warning: This API is now obsolete. +> +> Uses the [AppBase.updater$](./kibana-plugin-public.appbase.updater_.md) property when registering your application with [ApplicationSetup.register()](./kibana-plugin-public.applicationsetup.register.md) instead. +> + +Update the navlink for the given id with the updated attributes. Returns the updated navlink or `undefined` if it does not exist. + +Signature: + +```typescript +update(id: string, values: ChromeNavLinkUpdateableFields): ChromeNavLink | undefined; +``` + +## Parameters + +| Parameter | Type | Description | +| --- | --- | --- | +| id | string | | +| values | ChromeNavLinkUpdateableFields | | + +Returns: + +`ChromeNavLink | undefined` + diff --git a/docs/development/core/public/kibana-plugin-public.corestart.fatalerrors.md b/docs/development/core/public/kibana-plugin-public.corestart.fatalerrors.md new file mode 100644 index 0000000000000..540b17b5a6f0b --- /dev/null +++ b/docs/development/core/public/kibana-plugin-public.corestart.fatalerrors.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [CoreStart](./kibana-plugin-public.corestart.md) > [fatalErrors](./kibana-plugin-public.corestart.fatalerrors.md) + +## CoreStart.fatalErrors property + +[FatalErrorsStart](./kibana-plugin-public.fatalerrorsstart.md) + +Signature: + +```typescript +fatalErrors: FatalErrorsStart; +``` diff --git a/docs/development/core/public/kibana-plugin-public.corestart.md b/docs/development/core/public/kibana-plugin-public.corestart.md index e561ee313f100..83af82d590c36 100644 --- a/docs/development/core/public/kibana-plugin-public.corestart.md +++ b/docs/development/core/public/kibana-plugin-public.corestart.md @@ -19,6 +19,7 @@ export interface CoreStart | [application](./kibana-plugin-public.corestart.application.md) | ApplicationStart | [ApplicationStart](./kibana-plugin-public.applicationstart.md) | | [chrome](./kibana-plugin-public.corestart.chrome.md) | ChromeStart | [ChromeStart](./kibana-plugin-public.chromestart.md) | | [docLinks](./kibana-plugin-public.corestart.doclinks.md) | DocLinksStart | [DocLinksStart](./kibana-plugin-public.doclinksstart.md) | +| [fatalErrors](./kibana-plugin-public.corestart.fatalerrors.md) | FatalErrorsStart | [FatalErrorsStart](./kibana-plugin-public.fatalerrorsstart.md) | | [http](./kibana-plugin-public.corestart.http.md) | HttpStart | [HttpStart](./kibana-plugin-public.httpstart.md) | | [i18n](./kibana-plugin-public.corestart.i18n.md) | I18nStart | [I18nStart](./kibana-plugin-public.i18nstart.md) | | [injectedMetadata](./kibana-plugin-public.corestart.injectedmetadata.md) | {
getInjectedVar: (name: string, defaultValue?: any) => unknown;
} | exposed temporarily until https://github.com/elastic/kibana/issues/41990 done use \*only\* to retrieve config values. There is no way to set injected values in the new platform. Use the legacy platform API instead. | diff --git a/docs/development/core/public/kibana-plugin-public.fatalerrorsstart.md b/docs/development/core/public/kibana-plugin-public.fatalerrorsstart.md new file mode 100644 index 0000000000000..a8ece7dcb7e02 --- /dev/null +++ b/docs/development/core/public/kibana-plugin-public.fatalerrorsstart.md @@ -0,0 +1,13 @@ + + +[Home](./index.md) > [kibana-plugin-public](./kibana-plugin-public.md) > [FatalErrorsStart](./kibana-plugin-public.fatalerrorsstart.md) + +## FatalErrorsStart type + +FatalErrors stop the Kibana Public Core and displays a fatal error screen with details about the Kibana build and the error. + +Signature: + +```typescript +export declare type FatalErrorsStart = FatalErrorsSetup; +``` diff --git a/docs/development/core/public/kibana-plugin-public.md b/docs/development/core/public/kibana-plugin-public.md index 27ca9f2d9fd57..27037d46926c1 100644 --- a/docs/development/core/public/kibana-plugin-public.md +++ b/docs/development/core/public/kibana-plugin-public.md @@ -129,6 +129,7 @@ The plugin integrates with the core system via lifecycle events: `setup` | [ChromeHelpExtensionMenuGitHubLink](./kibana-plugin-public.chromehelpextensionmenugithublink.md) | | | [ChromeHelpExtensionMenuLink](./kibana-plugin-public.chromehelpextensionmenulink.md) | | | [ChromeNavLinkUpdateableFields](./kibana-plugin-public.chromenavlinkupdateablefields.md) | | +| [FatalErrorsStart](./kibana-plugin-public.fatalerrorsstart.md) | FatalErrors stop the Kibana Public Core and displays a fatal error screen with details about the Kibana build and the error. | | [HandlerContextType](./kibana-plugin-public.handlercontexttype.md) | Extracts the type of the first argument of a [HandlerFunction](./kibana-plugin-public.handlerfunction.md) to represent the type of the context. | | [HandlerFunction](./kibana-plugin-public.handlerfunction.md) | A function that accepts a context object and an optional number of additional arguments. Used for the generic types in [IContextContainer](./kibana-plugin-public.icontextcontainer.md) | | [HandlerParameters](./kibana-plugin-public.handlerparameters.md) | Extracts the types of the additional arguments of a [HandlerFunction](./kibana-plugin-public.handlerfunction.md), excluding the [HandlerContextType](./kibana-plugin-public.handlercontexttype.md). | diff --git a/src/core/public/core_system.ts b/src/core/public/core_system.ts index 5b31c740518e4..97bd49416f705 100644 --- a/src/core/public/core_system.ts +++ b/src/core/public/core_system.ts @@ -214,6 +214,7 @@ export class CoreSystem { const http = await this.http.start({ injectedMetadata, fatalErrors: this.fatalErrorsSetup! }); const savedObjects = await this.savedObjects.start({ http }); const i18n = await this.i18n.start(); + const fatalErrors = await this.fatalErrors.start(); await this.integrations.start({ uiSettings }); const coreUiTargetDomElement = document.createElement('div'); @@ -271,6 +272,7 @@ export class CoreSystem { notifications, overlays, uiSettings, + fatalErrors, }; const plugins = await this.plugins.start(core); diff --git a/src/core/public/fatal_errors/fatal_errors_service.mock.ts b/src/core/public/fatal_errors/fatal_errors_service.mock.ts index dd7702a7ee7dd..6d9876f787fa9 100644 --- a/src/core/public/fatal_errors/fatal_errors_service.mock.ts +++ b/src/core/public/fatal_errors/fatal_errors_service.mock.ts @@ -26,18 +26,22 @@ const createSetupContractMock = () => { return setupContract; }; +const createStartContractMock = createSetupContractMock; type FatalErrorsServiceContract = PublicMethodsOf; const createMock = () => { const mocked: jest.Mocked = { setup: jest.fn(), + start: jest.fn(), }; mocked.setup.mockReturnValue(createSetupContractMock()); + mocked.start.mockReturnValue(createStartContractMock()); return mocked; }; export const fatalErrorsServiceMock = { create: createMock, createSetupContract: createSetupContractMock, + createStartContract: createStartContractMock, }; diff --git a/src/core/public/fatal_errors/fatal_errors_service.tsx b/src/core/public/fatal_errors/fatal_errors_service.tsx index 5c6a7bb322ae1..309f07859ef26 100644 --- a/src/core/public/fatal_errors/fatal_errors_service.tsx +++ b/src/core/public/fatal_errors/fatal_errors_service.tsx @@ -54,9 +54,18 @@ export interface FatalErrorsSetup { get$: () => Rx.Observable; } +/** + * FatalErrors stop the Kibana Public Core and displays a fatal error screen + * with details about the Kibana build and the error. + * + * @public + */ +export type FatalErrorsStart = FatalErrorsSetup; + /** @interal */ export class FatalErrorsService { private readonly errorInfo$ = new Rx.ReplaySubject(); + private fatalErrors?: FatalErrorsSetup; /** * @@ -82,7 +91,7 @@ export class FatalErrorsService { }, }); - const fatalErrorsSetup: FatalErrorsSetup = { + this.fatalErrors = { add: (error, source?) => { const errorInfo = getErrorInfo(error, source); @@ -101,9 +110,17 @@ export class FatalErrorsService { }, }; - this.setupGlobalErrorHandlers(fatalErrorsSetup); + this.setupGlobalErrorHandlers(this.fatalErrors!); - return fatalErrorsSetup; + return this.fatalErrors!; + } + + public start() { + const { fatalErrors } = this; + if (!fatalErrors) { + throw new Error('FatalErrorsService#setup() must be invoked before start.'); + } + return fatalErrors; } private renderError(injectedMetadata: InjectedMetadataSetup, i18n: I18nStart) { diff --git a/src/core/public/fatal_errors/index.ts b/src/core/public/fatal_errors/index.ts index e37a36152cf91..c8ea1c0bccd22 100644 --- a/src/core/public/fatal_errors/index.ts +++ b/src/core/public/fatal_errors/index.ts @@ -17,5 +17,5 @@ * under the License. */ -export { FatalErrorsSetup, FatalErrorsService } from './fatal_errors_service'; +export { FatalErrorsSetup, FatalErrorsStart, FatalErrorsService } from './fatal_errors_service'; export { FatalErrorInfo } from './get_error_info'; diff --git a/src/core/public/index.ts b/src/core/public/index.ts index 5b17eccc37f8b..5e732dd05e616 100644 --- a/src/core/public/index.ts +++ b/src/core/public/index.ts @@ -55,7 +55,7 @@ import { ChromeRecentlyAccessed, ChromeRecentlyAccessedHistoryItem, } from './chrome'; -import { FatalErrorsSetup, FatalErrorInfo } from './fatal_errors'; +import { FatalErrorsSetup, FatalErrorsStart, FatalErrorInfo } from './fatal_errors'; import { HttpSetup, HttpStart } from './http'; import { I18nStart } from './i18n'; import { InjectedMetadataSetup, InjectedMetadataStart, LegacyNavLink } from './injected_metadata'; @@ -232,6 +232,8 @@ export interface CoreStart { overlays: OverlayStart; /** {@link IUiSettingsClient} */ uiSettings: IUiSettingsClient; + /** {@link FatalErrorsStart} */ + fatalErrors: FatalErrorsStart; /** * exposed temporarily until https://github.com/elastic/kibana/issues/41990 done * use *only* to retrieve config values. There is no way to set injected values @@ -302,6 +304,7 @@ export { DocLinksStart, FatalErrorInfo, FatalErrorsSetup, + FatalErrorsStart, HttpSetup, HttpStart, I18nStart, diff --git a/src/core/public/legacy/legacy_service.test.ts b/src/core/public/legacy/legacy_service.test.ts index 9dd24f9e4a7a3..d08c8b52e39c9 100644 --- a/src/core/public/legacy/legacy_service.test.ts +++ b/src/core/public/legacy/legacy_service.test.ts @@ -98,6 +98,7 @@ const notificationsStart = notificationServiceMock.createStartContract(); const overlayStart = overlayServiceMock.createStartContract(); const uiSettingsStart = uiSettingsServiceMock.createStartContract(); const savedObjectsStart = savedObjectsMock.createStartContract(); +const fatalErrorsStart = fatalErrorsServiceMock.createStartContract(); const mockStorage = { getItem: jest.fn() } as any; const defaultStartDeps = { @@ -112,6 +113,7 @@ const defaultStartDeps = { overlays: overlayStart, uiSettings: uiSettingsStart, savedObjects: savedObjectsStart, + fatalErrors: fatalErrorsStart, }, lastSubUrlStorage: mockStorage, targetDomElement: document.createElement('div'), diff --git a/src/core/public/mocks.ts b/src/core/public/mocks.ts index 43c8aa6f1d6b9..ce90d49065ad4 100644 --- a/src/core/public/mocks.ts +++ b/src/core/public/mocks.ts @@ -74,6 +74,7 @@ function createCoreStartMock({ basePath = '' } = {}) { injectedMetadata: { getInjectedVar: injectedMetadataServiceMock.createStartContract().getInjectedVar, }, + fatalErrors: fatalErrorsServiceMock.createStartContract(), }; return mock; diff --git a/src/core/public/plugins/plugin_context.ts b/src/core/public/plugins/plugin_context.ts index f146c2452868b..48100cba4f26e 100644 --- a/src/core/public/plugins/plugin_context.ts +++ b/src/core/public/plugins/plugin_context.ts @@ -151,5 +151,6 @@ export function createPluginStartContext< injectedMetadata: { getInjectedVar: deps.injectedMetadata.getInjectedVar, }, + fatalErrors: deps.fatalErrors, }; } diff --git a/src/core/public/plugins/plugins_service.test.ts b/src/core/public/plugins/plugins_service.test.ts index cafc7e5887e38..dbbcda8d60e12 100644 --- a/src/core/public/plugins/plugins_service.test.ts +++ b/src/core/public/plugins/plugins_service.test.ts @@ -111,6 +111,7 @@ describe('PluginsService', () => { overlays: overlayServiceMock.createStartContract(), uiSettings: uiSettingsServiceMock.createStartContract(), savedObjects: savedObjectsMock.createStartContract(), + fatalErrors: fatalErrorsServiceMock.createStartContract(), }; mockStartContext = { ...mockStartDeps, diff --git a/src/core/public/public.api.md b/src/core/public/public.api.md index abd39e864bd30..f7b260c68ee96 100644 --- a/src/core/public/public.api.md +++ b/src/core/public/public.api.md @@ -377,6 +377,8 @@ export interface CoreStart { // (undocumented) docLinks: DocLinksStart; // (undocumented) + fatalErrors: FatalErrorsStart; + // (undocumented) http: HttpStart; // (undocumented) i18n: I18nStart; @@ -532,6 +534,9 @@ export interface FatalErrorsSetup { get$: () => Rx.Observable; } +// @public +export type FatalErrorsStart = FatalErrorsSetup; + // @public export type HandlerContextType> = T extends HandlerFunction ? U : never; diff --git a/src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap b/src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap index bc08c87304fca..d8db53d4c6020 100644 --- a/src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap +++ b/src/plugins/data/public/ui/query_string_input/__snapshots__/query_string_input.test.tsx.snap @@ -304,6 +304,10 @@ exports[`QueryStringInput Should disable autoFocus on EuiFieldText when disableA }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -930,6 +934,10 @@ exports[`QueryStringInput Should disable autoFocus on EuiFieldText when disableA }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -1538,6 +1546,10 @@ exports[`QueryStringInput Should pass the query language to the language switche }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -2161,6 +2173,10 @@ exports[`QueryStringInput Should pass the query language to the language switche }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -2769,6 +2785,10 @@ exports[`QueryStringInput Should render the given query 1`] = ` }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { @@ -3392,6 +3412,10 @@ exports[`QueryStringInput Should render the given query 1`] = ` }, }, }, + "fatalErrors": Object { + "add": [MockFunction], + "get$": [MockFunction], + }, "http": Object { "addLoadingCountSource": [MockFunction], "anonymousPaths": Object { From 27c8a4bc25badb3ee3c1feaa7e1e68b10bab1ccd Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Tue, 21 Jan 2020 13:04:49 +0100 Subject: [PATCH 6/6] [State Management] remove AppState from Dashboard app (#54105) Removes AppState from dashboard app and replaces it with state containers and state syncing utilities. --- .../filter_manager/filter_state_manager.ts | 14 +- .../public/dashboard/__tests__/index.ts | 1 - .../kibana/public/dashboard/legacy_imports.ts | 6 - .../public/dashboard/np_ready/application.ts | 7 - .../dashboard/np_ready/dashboard_app.tsx | 16 +- .../np_ready/dashboard_app_controller.tsx | 110 ++++++--- .../np_ready/dashboard_state.test.ts | 13 +- .../np_ready/dashboard_state_manager.ts | 233 ++++++++++++------ .../public/dashboard/np_ready/legacy_app.js | 12 +- .../np_ready/lib/migrate_app_state.test.ts | 35 +-- .../np_ready/lib/migrate_app_state.ts | 15 +- .../dashboard/np_ready/lib/save_dashboard.ts | 11 +- .../np_ready/lib/update_saved_dashboard.ts | 4 +- .../kibana/public/dashboard/np_ready/types.ts | 43 ++-- src/legacy/ui/public/chrome/api/controls.ts | 2 + .../ui/public/chrome/directives/kbn_chrome.js | 10 + .../ui/public/state_management/state.js | 2 +- .../kibana_utils/public/history/index.ts | 20 ++ .../public/history/remove_query_param.test.ts | 75 ++++++ .../public/history/remove_query_param.ts} | 43 ++-- src/plugins/kibana_utils/public/index.ts | 2 + .../url/kbn_url_storage.test.ts | 54 +++- .../state_management/url/kbn_url_storage.ts | 41 ++- .../utils/diff_object.test.ts | 0 .../state_management/utils/diff_object.ts | 0 .../public/state_sync/state_sync.test.ts | 36 +++ .../public/state_sync/state_sync.ts | 14 +- .../create_kbn_url_state_storage.test.ts | 17 +- .../create_kbn_url_state_storage.ts | 17 +- .../apps/dashboard/dashboard_clone.js | 2 +- 30 files changed, 573 insertions(+), 282 deletions(-) create mode 100644 src/plugins/kibana_utils/public/history/index.ts create mode 100644 src/plugins/kibana_utils/public/history/remove_query_param.test.ts rename src/{legacy/core_plugins/kibana/public/dashboard/__tests__/get_app_state_mock.ts => plugins/kibana_utils/public/history/remove_query_param.ts} (53%) rename src/{legacy/ui => plugins/kibana_utils}/public/state_management/utils/diff_object.test.ts (100%) rename src/{legacy/ui => plugins/kibana_utils}/public/state_management/utils/diff_object.ts (100%) diff --git a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts index 633b7e630700d..ad5c91d2e19de 100644 --- a/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts +++ b/src/legacy/core_plugins/data/public/filter/filter_manager/filter_state_manager.ts @@ -18,6 +18,7 @@ */ import _ from 'lodash'; +import { Subscription } from 'rxjs'; import { State } from 'ui/state_management/state'; import { FilterManager, esFilters } from '../../../../../../plugins/data/public'; @@ -28,7 +29,7 @@ import { // eslint-disable-next-line @kbn/eslint/no-restricted-paths } from '../../../../../../plugins/data/public/query/filter_manager/lib/compare_filters'; -type GetAppStateFunc = () => State | undefined | null; +type GetAppStateFunc = () => { filters?: esFilters.Filter[]; save?: () => void } | undefined | null; /** * FilterStateManager is responsible for watching for filter changes @@ -36,10 +37,12 @@ type GetAppStateFunc = () => State | undefined | null; * back to the URL. **/ export class FilterStateManager { + private filterManagerUpdatesSubscription: Subscription; + filterManager: FilterManager; globalState: State; getAppState: GetAppStateFunc; - interval: NodeJS.Timeout | undefined; + interval: number | undefined; constructor(globalState: State, getAppState: GetAppStateFunc, filterManager: FilterManager) { this.getAppState = getAppState; @@ -48,7 +51,7 @@ export class FilterStateManager { this.watchFilterState(); - this.filterManager.getUpdates$().subscribe(() => { + this.filterManagerUpdatesSubscription = this.filterManager.getUpdates$().subscribe(() => { this.updateAppState(); }); } @@ -57,12 +60,13 @@ export class FilterStateManager { if (this.interval) { clearInterval(this.interval); } + this.filterManagerUpdatesSubscription.unsubscribe(); } private watchFilterState() { // This is a temporary solution to remove rootscope. // Moving forward, state should provide observable subscriptions. - this.interval = setInterval(() => { + this.interval = window.setInterval(() => { const appState = this.getAppState(); const stateUndefined = !appState || !this.globalState; if (stateUndefined) return; @@ -95,7 +99,7 @@ export class FilterStateManager { private saveState() { const appState = this.getAppState(); - if (appState) appState.save(); + if (appState && appState.save) appState.save(); this.globalState.save(); } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/__tests__/index.ts b/src/legacy/core_plugins/kibana/public/dashboard/__tests__/index.ts index ab8dfe81163e4..2b992f95695f3 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/__tests__/index.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/__tests__/index.ts @@ -17,6 +17,5 @@ * under the License. */ -export { getAppStateMock } from './get_app_state_mock'; export { getSavedDashboardMock } from './get_saved_dashboard_mock'; export { getEmbeddableFactoryMock } from './get_embeddable_factories_mock'; diff --git a/src/legacy/core_plugins/kibana/public/dashboard/legacy_imports.ts b/src/legacy/core_plugins/kibana/public/dashboard/legacy_imports.ts index b44d1993db23a..244a58e8a65e5 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/legacy_imports.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/legacy_imports.ts @@ -28,8 +28,6 @@ import chrome from 'ui/chrome'; export const legacyChrome = chrome; export { State } from 'ui/state_management/state'; -export { AppState } from 'ui/state_management/app_state'; -export { AppStateClass } from 'ui/state_management/app_state'; export { SavedObjectSaveOpts } from 'ui/saved_objects/types'; export { npSetup, npStart } from 'ui/new_platform'; export { IPrivate } from 'ui/private'; @@ -45,8 +43,6 @@ export { GlobalStateProvider } from 'ui/state_management/global_state'; // @ts-ignore export { StateManagementConfigProvider } from 'ui/state_management/config_provider'; // @ts-ignore -export { AppStateProvider } from 'ui/state_management/app_state'; -// @ts-ignore export { PrivateProvider } from 'ui/private/private'; // @ts-ignore export { EventsProvider } from 'ui/events'; @@ -60,9 +56,7 @@ export { KbnUrlProvider, RedirectWhenMissingProvider } from 'ui/url/index'; // @ts-ignore export { confirmModalFactory } from 'ui/modals/confirm_modal'; export { configureAppAngularModule } from 'ui/legacy_compat'; -export { stateMonitorFactory, StateMonitor } from 'ui/state_management/state_monitor_factory'; export { ensureDefaultIndexPattern } from 'ui/legacy_compat'; -export { unhashUrl } from '../../../../../plugins/kibana_utils/public'; export { IInjector } from 'ui/chrome'; export { SavedObjectLoader } from 'ui/saved_objects'; export { VISUALIZE_EMBEDDABLE_TYPE } from '../../../visualizations/public/embeddable'; diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts index 7f7bf7cf47bda..429a7f7279996 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/application.ts @@ -31,7 +31,6 @@ import { Storage } from '../../../../../../plugins/kibana_utils/public'; import { GlobalStateProvider, StateManagementConfigProvider, - AppStateProvider, PrivateProvider, EventsProvider, PersistedState, @@ -155,12 +154,6 @@ function createLocalStateModule() { 'app/dashboard/Promise', 'app/dashboard/PersistedState', ]) - .factory('AppState', function(Private: any) { - return Private(AppStateProvider); - }) - .service('getAppState', function(Private: any) { - return Private(AppStateProvider).getAppState; - }) .service('globalState', function(Private: any) { return Private(GlobalStateProvider); }); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx index e9fdc335ba572..f56990ae82e56 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app.tsx @@ -20,12 +20,7 @@ import moment from 'moment'; import { Subscription } from 'rxjs'; -import { - AppStateClass as TAppStateClass, - AppState as TAppState, - IInjector, - KbnUrl, -} from '../legacy_imports'; +import { IInjector } from '../legacy_imports'; import { ViewMode } from '../../../../embeddable_api/public/np_ready/public'; import { SavedObjectDashboard } from '../saved_dashboard/saved_dashboard'; @@ -43,7 +38,7 @@ import { RenderDeps } from './application'; export interface DashboardAppScope extends ng.IScope { dash: SavedObjectDashboard; - appState: TAppState; + appState: DashboardAppState; screenTitle: string; model: { query: Query; @@ -60,7 +55,6 @@ export interface DashboardAppScope extends ng.IScope { refreshInterval: any; panels: SavedDashboardPanel[]; indexPatterns: IIndexPattern[]; - $evalAsync: any; dashboardViewMode: ViewMode; expandedPanel?: string; getShouldShowEditHelp: () => boolean; @@ -91,8 +85,6 @@ export interface DashboardAppScope extends ng.IScope { export function initDashboardAppDirective(app: any, deps: RenderDeps) { app.directive('dashboardApp', function($injector: IInjector) { - const AppState = $injector.get>('AppState'); - const kbnUrl = $injector.get('kbnUrl'); const confirmModal = $injector.get('confirmModal'); const config = deps.uiSettings; @@ -105,17 +97,13 @@ export function initDashboardAppDirective(app: any, deps: RenderDeps) { $routeParams: { id?: string; }, - getAppState: any, globalState: any ) => new DashboardAppController({ $route, $scope, $routeParams, - getAppState, globalState, - kbnUrl, - AppStateClass: AppState, config, confirmModal, indexPatterns: deps.npDataStart.indexPatterns, diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx index 2706b588a2ec4..7da32ac97126f 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx @@ -17,57 +17,55 @@ * under the License. */ -import _ from 'lodash'; +import _, { uniq } from 'lodash'; import { i18n } from '@kbn/i18n'; import React from 'react'; import angular from 'angular'; -import { uniq } from 'lodash'; import { Subscription } from 'rxjs'; +import { createHashHistory } from 'history'; import { DashboardEmptyScreen, DashboardEmptyScreenProps } from './dashboard_empty_screen'; import { - subscribeWithScope, ConfirmationButtonTypes, - showSaveModal, - SaveResult, migrateLegacyQuery, - State, - AppStateClass as TAppStateClass, - KbnUrl, SavedObjectSaveOpts, - unhashUrl, + SaveResult, + showSaveModal, + State, + subscribeWithScope, } from '../legacy_imports'; import { FilterStateManager } from '../../../../data/public'; import { + esFilters, IndexPattern, + IndexPatternsContract, Query, SavedQuery, - IndexPatternsContract, } from '../../../../../../plugins/data/public'; import { - DashboardContainer, DASHBOARD_CONTAINER_TYPE, + DashboardContainer, DashboardContainerFactory, DashboardContainerInput, DashboardPanelState, } from '../../../../dashboard_embeddable_container/public/np_ready/public'; import { - isErrorEmbeddable, + EmbeddableFactoryNotFoundError, ErrorEmbeddable, - ViewMode, + isErrorEmbeddable, openAddPanelFlyout, - EmbeddableFactoryNotFoundError, + ViewMode, } from '../../../../embeddable_api/public/np_ready/public'; -import { DashboardAppState, NavAction, ConfirmModalFn, SavedDashboardPanel } from './types'; +import { ConfirmModalFn, NavAction, SavedDashboardPanel } from './types'; import { showOptionsPopover } from './top_nav/show_options_popover'; import { DashboardSaveModal } from './top_nav/save_modal'; import { showCloneModal } from './top_nav/show_clone_modal'; import { saveDashboard } from './lib'; import { DashboardStateManager } from './dashboard_state_manager'; -import { DashboardConstants, createDashboardEditUrl } from './dashboard_constants'; +import { createDashboardEditUrl, DashboardConstants } from './dashboard_constants'; import { getTopNavConfig } from './top_nav/get_top_nav_config'; import { TopNavIds } from './top_nav/top_nav_ids'; import { getDashboardTitle } from './dashboard_strings'; @@ -78,17 +76,15 @@ import { SavedObjectFinderProps, SavedObjectFinderUi, } from '../../../../../../plugins/kibana_react/public'; +import { removeQueryParam, unhashUrl } from '../../../../../../plugins/kibana_utils/public'; export interface DashboardAppControllerDependencies extends RenderDeps { $scope: DashboardAppScope; $route: any; $routeParams: any; - getAppState: any; globalState: State; indexPatterns: IndexPatternsContract; dashboardConfig: any; - kbnUrl: KbnUrl; - AppStateClass: TAppStateClass; config: any; confirmModal: ConfirmModalFn; } @@ -103,12 +99,9 @@ export class DashboardAppController { $scope, $route, $routeParams, - getAppState, globalState, dashboardConfig, localStorage, - kbnUrl, - AppStateClass, indexPatterns, config, confirmModal, @@ -124,7 +117,6 @@ export class DashboardAppController { }, core: { notifications, overlays, chrome, injectedMetadata, uiSettings, savedObjects, http }, }: DashboardAppControllerDependencies) { - new FilterStateManager(globalState, getAppState, filterManager); const queryFilter = filterManager; let lastReloadRequestTime = 0; @@ -134,14 +126,30 @@ export class DashboardAppController { chrome.docTitle.change(dash.title); } + const history = createHashHistory(); const dashboardStateManager = new DashboardStateManager({ savedDashboard: dash, - AppStateClass, + useHashedUrl: config.get('state:storeInSessionStorage'), hideWriteControls: dashboardConfig.getHideWriteControls(), kibanaVersion: injectedMetadata.getKibanaVersion(), + history, }); - $scope.appState = dashboardStateManager.getAppState(); + const filterStateManager = new FilterStateManager( + globalState, + () => { + // Temporary AppState replacement + return { + set filters(_filters: esFilters.Filter[]) { + dashboardStateManager.setFilters(_filters); + }, + get filters() { + return dashboardStateManager.appState.filters; + }, + }; + }, + filterManager + ); // The hash check is so we only update the time filter on dashboard open, not during // normal cross app navigation. @@ -316,8 +324,8 @@ export class DashboardAppController { dirty = true; } + dashboardStateManager.handleDashboardContainerChanges(container); $scope.$evalAsync(() => { - dashboardStateManager.handleDashboardContainerChanges(container); if (dirty) { updateState(); } @@ -337,8 +345,8 @@ export class DashboardAppController { const type = $routeParams[DashboardConstants.ADD_EMBEDDABLE_TYPE]; const id = $routeParams[DashboardConstants.ADD_EMBEDDABLE_ID]; container.addSavedObjectEmbeddable(type, id); - kbnUrl.removeParam(DashboardConstants.ADD_EMBEDDABLE_TYPE); - kbnUrl.removeParam(DashboardConstants.ADD_EMBEDDABLE_ID); + removeQueryParam(history, DashboardConstants.ADD_EMBEDDABLE_TYPE); + removeQueryParam(history, DashboardConstants.ADD_EMBEDDABLE_ID); } } @@ -409,7 +417,9 @@ export class DashboardAppController { key ]; if (!_.isEqual(containerValue, appStateValue)) { - (differences as { [key: string]: unknown })[key] = appStateValue; + // cloneDeep hack is needed, as there are multiple place, where container's input mutated, + // but values from appStateValue are deeply frozen, as they can't be mutated directly + (differences as { [key: string]: unknown })[key] = _.cloneDeep(appStateValue); } }); @@ -560,11 +570,6 @@ export class DashboardAppController { ); function updateViewMode(newMode: ViewMode) { - $scope.topNavMenu = getTopNavConfig( - newMode, - navActions, - dashboardConfig.getHideWriteControls() - ); // eslint-disable-line no-use-before-define dashboardStateManager.switchViewMode(newMode); } @@ -580,17 +585,28 @@ export class DashboardAppController { function revertChangesAndExitEditMode() { dashboardStateManager.resetState(); - kbnUrl.change( - dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL - ); // This is only necessary for new dashboards, which will default to Edit mode. updateViewMode(ViewMode.VIEW); + // Angular's $location skips this update because of history updates from syncState which happen simultaneously + // when calling kbnUrl.change() angular schedules url update and when angular finally starts to process it, + // the update is considered outdated and angular skips it + // so have to use implementation of dashboardStateManager.changeDashboardUrl, which workarounds those issues + dashboardStateManager.changeDashboardUrl( + dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL + ); + // We need to do a hard reset of the timepicker. appState will not reload like // it does on 'open' because it's been saved to the url and the getAppState.previouslyStored() check on // reload will cause it not to sync. if (dashboardStateManager.getIsTimeSavedWithDashboard()) { - dashboardStateManager.syncTimefilterWithDashboard(timefilter); + // have to use $evalAsync here until '_g' is migrated from $location to state sync utility ('history') + // When state sync utility changes url, angular's $location is missing it's own updates which happen during the same digest cycle + // temporary solution is to delay $location updates to next digest cycle + // unfortunately, these causes 2 browser history entries, but this is temporary and will be fixed after migrating '_g' to state_sync utilities + $scope.$evalAsync(() => { + dashboardStateManager.syncTimefilterWithDashboard(timefilter); + }); } } @@ -642,7 +658,11 @@ export class DashboardAppController { }); if (dash.id !== $routeParams.id) { - kbnUrl.change(createDashboardEditUrl(dash.id)); + // Angular's $location skips this update because of history updates from syncState which happen simultaneously + // when calling kbnUrl.change() angular schedules url update and when angular finally starts to process it, + // the update is considered outdated and angular skips it + // so have to use implementation of dashboardStateManager.changeDashboardUrl, which workarounds those issues + dashboardStateManager.changeDashboardUrl(createDashboardEditUrl(dash.id)); } else { chrome.docTitle.change(dash.lastSavedTitle); updateViewMode(ViewMode.VIEW); @@ -844,6 +864,15 @@ export class DashboardAppController { }); }); + dashboardStateManager.registerChangeListener(() => { + // view mode could have changed, so trigger top nav update + $scope.topNavMenu = getTopNavConfig( + dashboardStateManager.getViewMode(), + navActions, + dashboardConfig.getHideWriteControls() + ); + }); + $scope.$on('$destroy', () => { updateSubscription.unsubscribe(); visibleSubscription.unsubscribe(); @@ -859,6 +888,9 @@ export class DashboardAppController { if (dashboardContainer) { dashboardContainer.destroy(); } + if (filterStateManager) { + filterStateManager.destroy(); + } }); } } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts index d9a93b2ceedc3..8806684aab13c 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state.test.ts @@ -18,16 +18,12 @@ */ import './np_core.test.mocks'; +import { createBrowserHistory } from 'history'; import { DashboardStateManager } from './dashboard_state_manager'; -import { getAppStateMock, getSavedDashboardMock } from '../__tests__'; -import { AppStateClass } from '../legacy_imports'; -import { DashboardAppState } from './types'; -import { TimeRange, TimefilterContract, InputTimeRange } from 'src/plugins/data/public'; +import { getSavedDashboardMock } from '../__tests__'; +import { InputTimeRange, TimefilterContract, TimeRange } from 'src/plugins/data/public'; import { ViewMode } from 'src/plugins/embeddable/public'; -jest.mock('ui/state_management/state', () => ({ - State: {}, -})); jest.mock('ui/agg_types', () => ({ aggTypes: { metrics: [], @@ -52,9 +48,10 @@ describe('DashboardState', function() { function initDashboardState() { dashboardState = new DashboardStateManager({ savedDashboard, - AppStateClass: getAppStateMock() as AppStateClass, + useHashedUrl: false, hideWriteControls: false, kibanaVersion: '7.0.0', + history: createBrowserHistory(), }); } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts index 6df18757da6f5..451e7c8ff96db 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_state_manager.ts @@ -19,20 +19,16 @@ import { i18n } from '@kbn/i18n'; import _ from 'lodash'; - +import { History } from 'history'; +import { Subscription } from 'rxjs'; import { Moment } from 'moment'; import { DashboardContainer } from 'src/legacy/core_plugins/dashboard_embeddable_container/public/np_ready/public'; import { ViewMode } from '../../../../../../plugins/embeddable/public'; +import { migrateLegacyQuery } from '../legacy_imports'; import { - stateMonitorFactory, - StateMonitor, - AppStateClass as TAppStateClass, - migrateLegacyQuery, -} from '../legacy_imports'; -import { - Query, esFilters, + Query, TimefilterContract as Timefilter, } from '../../../../../../plugins/data/public'; @@ -41,7 +37,20 @@ import { convertPanelStateToSavedDashboardPanel } from './lib/embeddable_saved_o import { FilterUtils } from './lib/filter_utils'; import { SavedObjectDashboard } from '../saved_dashboard/saved_dashboard'; -import { SavedDashboardPanel, DashboardAppState, DashboardAppStateDefaults } from './types'; +import { + DashboardAppState, + DashboardAppStateDefaults, + DashboardAppStateTransitions, + SavedDashboardPanel, +} from './types'; +import { + createKbnUrlStateStorage, + createStateContainer, + IKbnUrlStateStorage, + ISyncStateRef, + ReduxLikeStateContainer, + syncState, +} from '../../../../../../plugins/kibana_utils/public'; /** * Dashboard state manager handles connecting angular and redux state between the angular and react portions of the @@ -51,7 +60,6 @@ import { SavedDashboardPanel, DashboardAppState, DashboardAppStateDefaults } fro */ export class DashboardStateManager { public savedDashboard: SavedObjectDashboard; - public appState: DashboardAppState; public lastSavedDashboardFilters: { timeTo?: string | Moment; timeFrom?: string | Moment; @@ -63,38 +71,78 @@ export class DashboardStateManager { private kibanaVersion: string; public isDirty: boolean; private changeListeners: Array<(status: { dirty: boolean }) => void>; - private stateMonitor: StateMonitor; + + public get appState(): DashboardAppState { + return this.stateContainer.get(); + } + + private readonly stateContainer: ReduxLikeStateContainer< + DashboardAppState, + DashboardAppStateTransitions + >; + private readonly stateContainerChangeSub: Subscription; + private readonly STATE_STORAGE_KEY = '_a'; + private readonly kbnUrlStateStorage: IKbnUrlStateStorage; + private readonly stateSyncRef: ISyncStateRef; + private readonly history: History; /** * * @param savedDashboard - * @param AppState The AppState class to use when instantiating a new AppState instance. * @param hideWriteControls true if write controls should be hidden. + * @param kibanaVersion current kibanaVersion + * @param */ constructor({ savedDashboard, - AppStateClass, hideWriteControls, kibanaVersion, + useHashedUrl, + history, }: { savedDashboard: SavedObjectDashboard; - AppStateClass: TAppStateClass; hideWriteControls: boolean; kibanaVersion: string; + useHashedUrl: boolean; + history: History; }) { + this.history = history; this.kibanaVersion = kibanaVersion; this.savedDashboard = savedDashboard; this.hideWriteControls = hideWriteControls; - this.stateDefaults = getAppStateDefaults(this.savedDashboard, this.hideWriteControls); + // get state defaults from saved dashboard, make sure it is migrated + this.stateDefaults = migrateAppState( + getAppStateDefaults(this.savedDashboard, this.hideWriteControls), + kibanaVersion + ); + + this.kbnUrlStateStorage = createKbnUrlStateStorage({ useHash: useHashedUrl, history }); - this.appState = new AppStateClass(this.stateDefaults); + // setup initial state by merging defaults with state from url + // also run migration, as state in url could be of older version + const initialState = migrateAppState( + { + ...this.stateDefaults, + ...this.kbnUrlStateStorage.get(this.STATE_STORAGE_KEY), + }, + kibanaVersion + ); - // Initializing appState does two things - first it translates the defaults into AppState, second it updates - // appState based on the URL (the url trumps the defaults). This means if we update the state format at all and - // want to handle BWC, we must not only migrate the data stored with saved Dashboard, but also any old state in the - // url. - migrateAppState(this.appState, kibanaVersion); + // setup state container using initial state both from defaults and from url + this.stateContainer = createStateContainer( + initialState, + { + set: state => (prop, value) => ({ ...state, [prop]: value }), + setOption: state => (option, value) => ({ + ...state, + options: { + ...state.options, + [option]: value, + }, + }), + } + ); this.isDirty = false; @@ -104,29 +152,35 @@ export class DashboardStateManager { // in the 'lose changes' warning message. this.lastSavedDashboardFilters = this.getFilterState(); - /** - * Creates a state monitor and saves it to this.stateMonitor. Used to track unsaved changes made to appState. - */ - this.stateMonitor = stateMonitorFactory.create( - this.appState, - this.stateDefaults - ); - - this.stateMonitor.ignoreProps('viewMode'); - // Filters need to be compared manually because they sometimes have a $$hashkey stored on the object. - this.stateMonitor.ignoreProps('filters'); - // Query needs to be compared manually because saved legacy queries get migrated in app state automatically - this.stateMonitor.ignoreProps('query'); + this.changeListeners = []; - this.stateMonitor.onChange((status: { dirty: boolean }) => { - this.isDirty = status.dirty; + this.stateContainerChangeSub = this.stateContainer.state$.subscribe(() => { + this.isDirty = this.checkIsDirty(); + this.changeListeners.forEach(listener => listener({ dirty: this.isDirty })); }); - this.changeListeners = []; - - this.stateMonitor.onChange((status: { dirty: boolean }) => { - this.changeListeners.forEach(listener => listener(status)); + // make sure url ('_a') matches initial state + this.kbnUrlStateStorage.set(this.STATE_STORAGE_KEY, initialState, { replace: true }); + + // setup state syncing utils. state container will be synched with url into `this.STATE_STORAGE_KEY` query param + this.stateSyncRef = syncState({ + storageKey: this.STATE_STORAGE_KEY, + stateContainer: { + ...this.stateContainer, + set: (state: DashboardAppState | null) => { + // sync state required state container to be able to handle null + // overriding set() so it could handle null coming from url + this.stateContainer.set({ + ...this.stateDefaults, + ...state, + }); + }, + }, + stateStorage: this.kbnUrlStateStorage, }); + + // actually start syncing state with container + this.stateSyncRef.start(); } public registerChangeListener(callback: (status: { dirty: boolean }) => void) { @@ -172,7 +226,7 @@ export class DashboardStateManager { }); if (dirty) { - this.appState.panels = Object.values(convertedPanelStateMap); + this.stateContainer.transitions.set('panels', Object.values(convertedPanelStateMap)); } if (input.isFullScreenMode !== this.getFullScreenMode()) { @@ -184,7 +238,6 @@ export class DashboardStateManager { } this.changeListeners.forEach(listener => listener({ dirty })); - this.saveState(); } public getFullScreenMode() { @@ -192,8 +245,11 @@ export class DashboardStateManager { } public setFullScreenMode(fullScreenMode: boolean) { - this.appState.fullScreenMode = fullScreenMode; - this.saveState(); + this.stateContainer.transitions.set('fullScreenMode', fullScreenMode); + } + + public setFilters(filters: esFilters.Filter[]) { + this.stateContainer.transitions.set('filters', filters); } /** @@ -210,7 +266,10 @@ export class DashboardStateManager { // The right way to fix this might be to ensure the defaults object stored on state is a deep // clone, but given how much code uses the state object, I determined that to be too risky of a change for // now. TODO: revisit this! - this.stateDefaults = getAppStateDefaults(this.savedDashboard, this.hideWriteControls); + this.stateDefaults = migrateAppState( + getAppStateDefaults(this.savedDashboard, this.hideWriteControls), + this.kibanaVersion + ); // The original query won't be restored by the above because the query on this.savedDashboard is applied // in place in order for it to affect the visualizations. this.stateDefaults.query = this.lastSavedDashboardFilters.query; @@ -218,9 +277,7 @@ export class DashboardStateManager { this.stateDefaults.filters = [...this.getLastSavedFilterBars()]; this.isDirty = false; - this.appState.setDefaults(this.stateDefaults); - this.appState.reset(); - this.stateMonitor.setInitialState(this.appState.toJSON()); + this.stateContainer.set(this.stateDefaults); } /** @@ -252,31 +309,28 @@ export class DashboardStateManager { } public setDescription(description: string) { - this.appState.description = description; - this.saveState(); + this.stateContainer.transitions.set('description', description); } public setTitle(title: string) { - this.appState.title = title; this.savedDashboard.title = title; - this.saveState(); + this.stateContainer.transitions.set('title', title); } public getAppState() { - return this.appState; + return this.stateContainer.get(); } public getQuery(): Query { - return migrateLegacyQuery(this.appState.query); + return migrateLegacyQuery(this.stateContainer.get().query); } public getSavedQueryId() { - return this.appState.savedQuery; + return this.stateContainer.get().savedQuery; } public setSavedQueryId(id?: string) { - this.appState.savedQuery = id; - this.saveState(); + this.stateContainer.transitions.set('savedQuery', id); } public getUseMargins() { @@ -287,8 +341,7 @@ export class DashboardStateManager { } public setUseMargins(useMargins: boolean) { - this.appState.options.useMargins = useMargins; - this.saveState(); + this.stateContainer.transitions.setOption('useMargins', useMargins); } public getHidePanelTitles() { @@ -296,8 +349,7 @@ export class DashboardStateManager { } public setHidePanelTitles(hidePanelTitles: boolean) { - this.appState.options.hidePanelTitles = hidePanelTitles; - this.saveState(); + this.stateContainer.transitions.setOption('hidePanelTitles', hidePanelTitles); } public getTimeRestore() { @@ -305,8 +357,7 @@ export class DashboardStateManager { } public setTimeRestore(timeRestore: boolean) { - this.appState.timeRestore = timeRestore; - this.saveState(); + this.stateContainer.transitions.set('timeRestore', timeRestore); } public getIsTimeSavedWithDashboard() { @@ -397,7 +448,6 @@ export class DashboardStateManager { (panel: SavedDashboardPanel) => panel.panelIndex === panelIndex ); Object.assign(foundPanel, panelAttributes); - this.saveState(); return foundPanel; } @@ -456,15 +506,37 @@ export class DashboardStateManager { } /** - * Saves the current application state to the URL. + * Synchronously writes current state to url + * returned boolean indicates whether the update happened and if history was updated */ - public saveState() { - this.appState.save(); + private saveState({ replace }: { replace: boolean }): boolean { + // schedules setting current state to url + this.kbnUrlStateStorage.set( + this.STATE_STORAGE_KEY, + this.stateContainer.get() + ); + // immediately forces scheduled updates and changes location + return this.kbnUrlStateStorage.flush({ replace }); + } + + // TODO: find nicer solution for this + // this function helps to make just 1 browser history update, when we imperatively changing the dashboard url + // It could be that there is pending *dashboardStateManager* updates, which aren't flushed yet to the url. + // So to prevent 2 browser updates: + // 1. Force flush any pending state updates (syncing state to query) + // 2. If url was updated, then apply path change with replace + public changeDashboardUrl(pathname: string) { + // synchronously persist current state to url with push() + const updated = this.saveState({ replace: false }); + // change pathname + this.history[updated ? 'replace' : 'push']({ + ...this.history.location, + pathname, + }); } public setQuery(query: Query) { - this.appState.query = query; - this.saveState(); + this.stateContainer.transitions.set('query', query); } /** @@ -472,24 +544,33 @@ export class DashboardStateManager { * @param filter An array of filter bar filters. */ public applyFilters(query: Query, filters: esFilters.Filter[]) { - this.appState.query = query; this.savedDashboard.searchSource.setField('query', query); this.savedDashboard.searchSource.setField('filter', filters); - this.saveState(); + this.stateContainer.transitions.set('query', query); } public switchViewMode(newMode: ViewMode) { - this.appState.viewMode = newMode; - this.saveState(); + this.stateContainer.transitions.set('viewMode', newMode); } /** * Destroys and cleans up this object when it's no longer used. */ public destroy() { - if (this.stateMonitor) { - this.stateMonitor.destroy(); - } + this.stateContainerChangeSub.unsubscribe(); this.savedDashboard.destroy(); + if (this.stateSyncRef) { + this.stateSyncRef.stop(); + } + } + + private checkIsDirty() { + // Filters need to be compared manually because they sometimes have a $$hashkey stored on the object. + // Query needs to be compared manually because saved legacy queries get migrated in app state automatically + const propsToIgnore: Array = ['viewMode', 'filters', 'query']; + + const initial = _.omit(this.stateDefaults, propsToIgnore); + const current = _.omit(this.stateContainer.get(), propsToIgnore); + return !_.isEqual(initial, current); } } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js index 540bfcf5aa684..7dc408ea4b801 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/legacy_app.js @@ -35,6 +35,7 @@ import { import { DashboardListing, EMPTY_FILTER } from './listing/dashboard_listing'; import { addHelpMenuToAppChrome } from './help_menu/help_menu_util'; import { syncOnMount } from './global_state_sync'; +import { createHashHistory } from 'history'; export function initDashboardApp(app, deps) { initDashboardAppDirective(app, deps); @@ -190,7 +191,7 @@ export function initDashboardApp(app, deps) { template: dashboardTemplate, controller: createNewDashboardCtrl, resolve: { - dash: function($rootScope, $route, redirectWhenMissing, kbnUrl, AppState) { + dash: function($rootScope, $route, redirectWhenMissing, kbnUrl) { const id = $route.current.params.id; return ensureDefaultIndexPattern(deps.core, deps.npDataStart, $rootScope, kbnUrl) @@ -216,8 +217,13 @@ export function initDashboardApp(app, deps) { // Preserve BWC of v5.3.0 links for new, unsaved dashboards. // See https://github.com/elastic/kibana/issues/10951 for more context. if (error instanceof SavedObjectNotFound && id === 'create') { - // Note "new AppState" is necessary so the state in the url is preserved through the redirect. - kbnUrl.redirect(DashboardConstants.CREATE_NEW_DASHBOARD_URL, {}, new AppState()); + // Note preserve querystring part is necessary so the state is preserved through the redirect. + const history = createHashHistory(); + history.replace({ + ...history.location, // preserve query, + pathname: DashboardConstants.CREATE_NEW_DASHBOARD_URL, + }); + deps.toastNotifications.addWarning( i18n.translate('kbn.dashboard.urlWasRemovedInSixZeroWarningMessage', { defaultMessage: diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.test.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.test.ts index 4aa2461bb6593..73336ec951894 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.test.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.test.ts @@ -23,7 +23,6 @@ import { SavedDashboardPanel } from '../types'; import { migrateAppState } from './migrate_app_state'; test('migrate app state from 6.0', async () => { - const mockSave = jest.fn(); const appState = { uiState: { 'P-1': { vis: { defaultColors: { '0+-+100': 'rgb(0,104,55)' } } }, @@ -39,11 +38,8 @@ test('migrate app state from 6.0', async () => { type: 'visualization', }, ], - translateHashToRison: () => 'a', - getQueryParamName: () => 'a', - save: mockSave, }; - migrateAppState(appState, '8.0'); + migrateAppState(appState as any, '8.0'); expect(appState.uiState).toBeUndefined(); const newPanel = (appState.panels[0] as unknown) as SavedDashboardPanel; @@ -54,12 +50,10 @@ test('migrate app state from 6.0', async () => { expect(newPanel.gridData.y).toBe(0); expect((newPanel.embeddableConfig as any).vis.defaultColors['0+-+100']).toBe('rgb(0,104,55)'); - expect(mockSave).toBeCalledTimes(1); }); test('migrate sort from 6.1', async () => { const TARGET_VERSION = '8.0'; - const mockSave = jest.fn(); const appState = { uiState: { 'P-1': { vis: { defaultColors: { '0+-+100': 'rgb(0,104,55)' } } }, @@ -76,12 +70,9 @@ test('migrate sort from 6.1', async () => { sort: 'sort', }, ], - translateHashToRison: () => 'a', - getQueryParamName: () => 'a', - save: mockSave, useMargins: false, }; - migrateAppState(appState, TARGET_VERSION); + migrateAppState(appState as any, TARGET_VERSION); expect(appState.uiState).toBeUndefined(); const newPanel = (appState.panels[0] as unknown) as SavedDashboardPanel; @@ -91,11 +82,9 @@ test('migrate sort from 6.1', async () => { expect((newPanel.embeddableConfig as any).sort).toBe('sort'); expect((newPanel.embeddableConfig as any).vis.defaultColors['0+-+100']).toBe('rgb(0,104,55)'); - expect(mockSave).toBeCalledTimes(1); }); test('migrates 6.0 even when uiState does not exist', async () => { - const mockSave = jest.fn(); const appState = { panels: [ { @@ -109,11 +98,8 @@ test('migrates 6.0 even when uiState does not exist', async () => { sort: 'sort', }, ], - translateHashToRison: () => 'a', - getQueryParamName: () => 'a', - save: mockSave, }; - migrateAppState(appState, '8.0'); + migrateAppState(appState as any, '8.0'); expect((appState as any).uiState).toBeUndefined(); const newPanel = (appState.panels[0] as unknown) as SavedDashboardPanel; @@ -122,11 +108,9 @@ test('migrates 6.0 even when uiState does not exist', async () => { expect((newPanel as any).sort).toBeUndefined(); expect((newPanel.embeddableConfig as any).sort).toBe('sort'); - expect(mockSave).toBeCalledTimes(1); }); test('6.2 migration adjusts w & h without margins', async () => { - const mockSave = jest.fn(); const appState = { panels: [ { @@ -143,12 +127,9 @@ test('6.2 migration adjusts w & h without margins', async () => { version: '6.2.0', }, ], - translateHashToRison: () => 'a', - getQueryParamName: () => 'a', - save: mockSave, useMargins: false, }; - migrateAppState(appState, '8.0'); + migrateAppState(appState as any, '8.0'); expect((appState as any).uiState).toBeUndefined(); const newPanel = (appState.panels[0] as unknown) as SavedDashboardPanel; @@ -159,11 +140,9 @@ test('6.2 migration adjusts w & h without margins', async () => { expect((newPanel as any).sort).toBeUndefined(); expect((newPanel.embeddableConfig as any).sort).toBe('sort'); - expect(mockSave).toBeCalledTimes(1); }); test('6.2 migration adjusts w & h with margins', async () => { - const mockSave = jest.fn(); const appState = { panels: [ { @@ -180,12 +159,9 @@ test('6.2 migration adjusts w & h with margins', async () => { version: '6.2.0', }, ], - translateHashToRison: () => 'a', - getQueryParamName: () => 'a', - save: mockSave, useMargins: true, }; - migrateAppState(appState, '8.0'); + migrateAppState(appState as any, '8.0'); expect((appState as any).uiState).toBeUndefined(); const newPanel = (appState.panels[0] as unknown) as SavedDashboardPanel; @@ -196,5 +172,4 @@ test('6.2 migration adjusts w & h with margins', async () => { expect((newPanel as any).sort).toBeUndefined(); expect((newPanel.embeddableConfig as any).sort).toBe('sort'); - expect(mockSave).toBeCalledTimes(1); }); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.ts index 4083900c7ede7..0cd958ced0eb1 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/migrate_app_state.ts @@ -28,6 +28,7 @@ import { SavedDashboardPanel630, SavedDashboardPanel640To720, SavedDashboardPanel620, + SavedDashboardPanel, } from '../types'; import { migratePanelsTo730 } from '../../migrations/migrate_to_730_panels'; @@ -37,9 +38,9 @@ import { migratePanelsTo730 } from '../../migrations/migrate_to_730_panels'; * Once we hit a major version, we can remove support for older style URLs and get rid of this logic. */ export function migrateAppState( - appState: { [key: string]: unknown } | DashboardAppState, + appState: { [key: string]: unknown } & DashboardAppState, kibanaVersion: string -) { +): DashboardAppState { if (!appState.panels) { throw new Error( i18n.translate('kbn.dashboard.panel.invalidData', { @@ -76,11 +77,11 @@ export function migrateAppState( | SavedDashboardPanel640To720 >, kibanaVersion, - appState.useMargins, - appState.uiState - ); + appState.useMargins as boolean, + appState.uiState as Record> + ) as SavedDashboardPanel[]; delete appState.uiState; - - appState.save(); } + + return appState; } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/save_dashboard.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/save_dashboard.ts index 691c87122564f..d80208ce27ffe 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/save_dashboard.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/save_dashboard.ts @@ -36,16 +36,19 @@ export function saveDashboard( dashboardStateManager: DashboardStateManager, saveOptions: SavedObjectSaveOpts ): Promise { - dashboardStateManager.saveState(); - const savedDashboard = dashboardStateManager.savedDashboard; const appState = dashboardStateManager.appState; updateSavedDashboard(savedDashboard, appState, timeFilter, toJson); return savedDashboard.save(saveOptions).then((id: string) => { - dashboardStateManager.lastSavedDashboardFilters = dashboardStateManager.getFilterState(); - dashboardStateManager.resetState(); + if (id) { + // reset state only when save() was successful + // e.g. save() could be interrupted if title is duplicated and not confirmed + dashboardStateManager.lastSavedDashboardFilters = dashboardStateManager.getFilterState(); + dashboardStateManager.resetState(); + } + return id; }); } diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/update_saved_dashboard.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/update_saved_dashboard.ts index 2072b5d4f6eb0..ec8073c0f72f7 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/update_saved_dashboard.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/lib/update_saved_dashboard.ts @@ -19,13 +19,13 @@ import _ from 'lodash'; import { RefreshInterval, TimefilterContract } from 'src/plugins/data/public'; -import { AppState } from '../../legacy_imports'; import { FilterUtils } from './filter_utils'; import { SavedObjectDashboard } from '../../saved_dashboard/saved_dashboard'; +import { DashboardAppState } from '../types'; export function updateSavedDashboard( savedDashboard: SavedObjectDashboard, - appState: AppState, + appState: DashboardAppState, timeFilter: TimefilterContract, toJson: (object: T) => string ) { diff --git a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/types.ts b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/types.ts index e3eb25a208856..3151fbf821b9f 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/np_ready/types.ts +++ b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/types.ts @@ -18,7 +18,6 @@ */ import { ViewMode } from 'src/plugins/embeddable/public'; -import { AppState } from '../legacy_imports'; import { RawSavedDashboardPanelTo60, RawSavedDashboardPanel610, @@ -93,11 +92,7 @@ export type SavedDashboardPanelTo60 = Pick< readonly type: string; }; -export type DashboardAppStateDefaults = DashboardAppStateParameters & { - description?: string; -}; - -export interface DashboardAppStateParameters { +export interface DashboardAppState { panels: SavedDashboardPanel[]; fullScreenMode: boolean; title: string; @@ -113,9 +108,24 @@ export interface DashboardAppStateParameters { savedQuery?: string; } -// This could probably be improved if we flesh out AppState more... though AppState will be going away -// so maybe not worth too much time atm. -export type DashboardAppState = DashboardAppStateParameters & AppState; +export type DashboardAppStateDefaults = DashboardAppState & { + description?: string; +}; + +export interface DashboardAppStateTransitions { + set: ( + state: DashboardAppState + ) => ( + prop: T, + value: DashboardAppState[T] + ) => DashboardAppState; + setOption: ( + state: DashboardAppState + ) => ( + prop: T, + value: DashboardAppState['options'][T] + ) => DashboardAppState; +} export interface SavedDashboardPanelMap { [key: string]: SavedDashboardPanel; @@ -139,18 +149,3 @@ export type ConfirmModalFn = ( title: string; } ) => void; - -export type AddFilterFn = ( - { - field, - value, - operator, - index, - }: { - field: string; - value: string; - operator: string; - index: string; - }, - appState: AppState -) => void; diff --git a/src/legacy/ui/public/chrome/api/controls.ts b/src/legacy/ui/public/chrome/api/controls.ts index 6dde26be20df2..a95d53ec2eab6 100644 --- a/src/legacy/ui/public/chrome/api/controls.ts +++ b/src/legacy/ui/public/chrome/api/controls.ts @@ -42,4 +42,6 @@ export function initChromeControlsApi(chrome: { [key: string]: any }) { * might be incorrect in the moments just before the UI is updated. */ chrome.getVisible = () => visible$.getValue(); + + chrome.visible$ = visible$.asObservable(); } diff --git a/src/legacy/ui/public/chrome/directives/kbn_chrome.js b/src/legacy/ui/public/chrome/directives/kbn_chrome.js index 72d26a37a60a1..4c5d7981e962a 100644 --- a/src/legacy/ui/public/chrome/directives/kbn_chrome.js +++ b/src/legacy/ui/public/chrome/directives/kbn_chrome.js @@ -30,6 +30,7 @@ import { chromeHeaderNavControlsRegistry, NavControlSide, } from '../../registry/chrome_header_nav_controls'; +import { subscribeWithScope } from '../../utils/subscribe_with_scope'; export function kbnChromeProvider(chrome, internals) { uiModules.get('kibana').directive('kbnChrome', () => { @@ -83,6 +84,15 @@ export function kbnChromeProvider(chrome, internals) { ); } + const chromeVisibility = subscribeWithScope($scope, chrome.visible$, { + next: () => { + // just makes sure change detection is triggered when chrome visibility changes + }, + }); + $scope.$on('$destroy', () => { + chromeVisibility.unsubscribe(); + }); + return chrome; }, }; diff --git a/src/legacy/ui/public/state_management/state.js b/src/legacy/ui/public/state_management/state.js index 289d4b8006cba..c2274eae59f50 100644 --- a/src/legacy/ui/public/state_management/state.js +++ b/src/legacy/ui/public/state_management/state.js @@ -29,7 +29,6 @@ import _ from 'lodash'; import { i18n } from '@kbn/i18n'; import angular from 'angular'; import rison from 'rison-node'; -import { applyDiff } from './utils/diff_object'; import { EventsProvider } from '../events'; import { fatalError, toastNotifications } from '../notify'; import './config_provider'; @@ -38,6 +37,7 @@ import { hashedItemStore, isStateHash, createStateHash, + applyDiff, } from '../../../../plugins/kibana_utils/public'; export function StateProvider( diff --git a/src/plugins/kibana_utils/public/history/index.ts b/src/plugins/kibana_utils/public/history/index.ts new file mode 100644 index 0000000000000..b4b5658c1c886 --- /dev/null +++ b/src/plugins/kibana_utils/public/history/index.ts @@ -0,0 +1,20 @@ +/* + * 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. + */ + +export { removeQueryParam } from './remove_query_param'; diff --git a/src/plugins/kibana_utils/public/history/remove_query_param.test.ts b/src/plugins/kibana_utils/public/history/remove_query_param.test.ts new file mode 100644 index 0000000000000..0b2547ae94668 --- /dev/null +++ b/src/plugins/kibana_utils/public/history/remove_query_param.test.ts @@ -0,0 +1,75 @@ +/* + * 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 { removeQueryParam } from './remove_query_param'; +import { createMemoryHistory, Location } from 'history'; + +describe('removeQueryParam', () => { + it('should remove query param from url', () => { + const startLocation: Location = { + pathname: '/dashboard/c3a76790-3134-11ea-b024-83a7b4783735', + search: "?_a=(description:'')&_b=3", + state: null, + hash: '', + }; + + const history = createMemoryHistory(); + history.push(startLocation); + removeQueryParam(history, '_a'); + + expect(history.location).toEqual( + expect.objectContaining({ + pathname: '/dashboard/c3a76790-3134-11ea-b024-83a7b4783735', + search: '?_b=3', + state: null, + hash: '', + }) + ); + }); + + it('should not fail if nothing to remove', () => { + const startLocation: Location = { + pathname: '/dashboard/c3a76790-3134-11ea-b024-83a7b4783735', + search: "?_a=(description:'')&_b=3", + state: null, + hash: '', + }; + + const history = createMemoryHistory(); + history.push(startLocation); + removeQueryParam(history, '_c'); + + expect(history.location).toEqual(expect.objectContaining(startLocation)); + }); + + it('should not fail if no search', () => { + const startLocation: Location = { + pathname: '/dashboard/c3a76790-3134-11ea-b024-83a7b4783735', + search: '', + state: null, + hash: '', + }; + + const history = createMemoryHistory(); + history.push(startLocation); + removeQueryParam(history, '_c'); + + expect(history.location).toEqual(expect.objectContaining(startLocation)); + }); +}); diff --git a/src/legacy/core_plugins/kibana/public/dashboard/__tests__/get_app_state_mock.ts b/src/plugins/kibana_utils/public/history/remove_query_param.ts similarity index 53% rename from src/legacy/core_plugins/kibana/public/dashboard/__tests__/get_app_state_mock.ts rename to src/plugins/kibana_utils/public/history/remove_query_param.ts index d9dea35a8a1c0..fbf985998b4cd 100644 --- a/src/legacy/core_plugins/kibana/public/dashboard/__tests__/get_app_state_mock.ts +++ b/src/plugins/kibana_utils/public/history/remove_query_param.ts @@ -17,32 +17,23 @@ * under the License. */ -import { AppStateClass } from '../legacy_imports'; +import { History, Location } from 'history'; +import { parse } from 'querystring'; +import { stringifyQueryString } from '../state_management/url/stringify_query_string'; // TODO: extract it to ../url -/** - * A poor excuse for a mock just to get some basic tests to run in jest without requiring the injector. - * This could be improved if we extract the appState and state classes externally of their angular providers. - * @return {AppStateMock} - */ -export function getAppStateMock(): AppStateClass { - class AppStateMock { - constructor(defaults: any) { - Object.assign(this, defaults); - } - - on() {} - off() {} - toJSON() { - return ''; - } - save() {} - translateHashToRison(stateHashOrRison: string | string[]) { - return stateHashOrRison; - } - getQueryParamName() { - return ''; - } +export function removeQueryParam(history: History, param: string, replace: boolean = true) { + const oldLocation = history.location; + const search = (oldLocation.search || '').replace(/^\?/, ''); + const query = parse(search); + delete query[param]; + const newSearch = stringifyQueryString(query); + const newLocation: Location = { + ...oldLocation, + search: newSearch, + }; + if (replace) { + history.replace(newLocation); + } else { + history.push(newLocation); } - - return AppStateMock; } diff --git a/src/plugins/kibana_utils/public/index.ts b/src/plugins/kibana_utils/public/index.ts index fa58a61e51232..00c1c95028b4d 100644 --- a/src/plugins/kibana_utils/public/index.ts +++ b/src/plugins/kibana_utils/public/index.ts @@ -58,3 +58,5 @@ export { StartSyncStateFnType, StopSyncStateFnType, } from './state_sync'; +export { removeQueryParam } from './history'; +export { applyDiff } from './state_management/utils/diff_object'; diff --git a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.test.ts b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.test.ts index f1c527d3d5309..6e4c505c62ebc 100644 --- a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.test.ts +++ b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.test.ts @@ -85,6 +85,7 @@ describe('kbn_url_storage', () => { beforeEach(() => { history = createMemoryHistory(); urlControls = createKbnUrlControls(history); + urlControls.update('/', true); }); const getCurrentUrl = () => createPath(history.location); @@ -143,17 +144,6 @@ describe('kbn_url_storage', () => { expect(cb).toHaveBeenCalledTimes(3); }); - it('should flush async url updates', async () => { - const pr1 = urlControls.updateAsync(() => '/1', false); - const pr2 = urlControls.updateAsync(() => '/2', false); - const pr3 = urlControls.updateAsync(() => '/3', false); - expect(getCurrentUrl()).toBe('/'); - urlControls.flush(); - expect(getCurrentUrl()).toBe('/3'); - await Promise.all([pr1, pr2, pr3]); - expect(getCurrentUrl()).toBe('/3'); - }); - it('flush should take priority over regular replace behaviour', async () => { const pr1 = urlControls.updateAsync(() => '/1', true); const pr2 = urlControls.updateAsync(() => '/2', false); @@ -174,6 +164,48 @@ describe('kbn_url_storage', () => { await Promise.all([pr1, pr2, pr3]); expect(getCurrentUrl()).toBe('/'); }); + + it('should retrieve pending url ', async () => { + const pr1 = urlControls.updateAsync(() => '/1', true); + const pr2 = urlControls.updateAsync(() => '/2', false); + const pr3 = urlControls.updateAsync(() => '/3', true); + expect(urlControls.getPendingUrl()).toEqual('/3'); + expect(getCurrentUrl()).toBe('/'); + await Promise.all([pr1, pr2, pr3]); + expect(getCurrentUrl()).toBe('/3'); + + expect(urlControls.getPendingUrl()).toBeUndefined(); + }); + }); + + describe('urlControls - browser history integration', () => { + let history: History; + let urlControls: IKbnUrlControls; + beforeEach(() => { + history = createBrowserHistory(); + urlControls = createKbnUrlControls(history); + urlControls.update('/', true); + }); + + const getCurrentUrl = () => window.location.href; + + it('should flush async url updates', async () => { + const pr1 = urlControls.updateAsync(() => '/1', false); + const pr2 = urlControls.updateAsync(() => '/2', false); + const pr3 = urlControls.updateAsync(() => '/3', false); + expect(getCurrentUrl()).toBe('http://localhost/'); + expect(urlControls.flush()).toBe('http://localhost/3'); + expect(getCurrentUrl()).toBe('http://localhost/3'); + await Promise.all([pr1, pr2, pr3]); + expect(getCurrentUrl()).toBe('http://localhost/3'); + }); + + it('flush() should return undefined, if no url updates happened', () => { + expect(urlControls.flush()).toBeUndefined(); + urlControls.updateAsync(() => 'http://localhost/1', false); + urlControls.updateAsync(() => 'http://localhost/', false); + expect(urlControls.flush()).toBeUndefined(); + }); }); describe('getRelativeToHistoryPath', () => { diff --git a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts index 03c136ea3d092..1dd204e717213 100644 --- a/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts +++ b/src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts @@ -107,25 +107,34 @@ export interface IKbnUrlControls { listen: (cb: () => void) => () => void; /** - * Updates url synchronously + * Updates url synchronously, if needed + * skips the update and returns undefined in case when trying to update to current url + * otherwise returns new url + * * @param url - url to update to * @param replace - use replace instead of push */ - update: (url: string, replace: boolean) => string; + update: (url: string, replace: boolean) => string | undefined; /** * Schedules url update to next microtask, * Useful to batch sync changes to url to cause only one browser history update * @param updater - fn which receives current url and should return next url to update to * @param replace - use replace instead of push + * */ - updateAsync: (updater: UrlUpdaterFnType, replace?: boolean) => Promise; + updateAsync: (updater: UrlUpdaterFnType, replace?: boolean) => Promise; /** - * Synchronously flushes scheduled url updates + * If there is a pending url update - returns url that is scheduled for update + */ + getPendingUrl: () => string | undefined; + + /** + * Synchronously flushes scheduled url updates. Returns new flushed url, if there was an update. Otherwise - undefined. * @param replace - if replace passed in, then uses it instead of push. Otherwise push or replace is picked depending on updateQueue */ - flush: (replace?: boolean) => string; + flush: (replace?: boolean) => string | undefined; /** * Cancels any pending url updates @@ -143,9 +152,9 @@ export const createKbnUrlControls = ( // if any call in a queue asked to push, then we should push let shouldReplace = true; - function updateUrl(newUrl: string, replace = false): string { + function updateUrl(newUrl: string, replace = false): string | undefined { const currentUrl = getCurrentUrl(); - if (newUrl === currentUrl) return currentUrl; // skip update + if (newUrl === currentUrl) return undefined; // skip update const historyPath = getRelativeToHistoryPath(newUrl, history); @@ -166,15 +175,22 @@ export const createKbnUrlControls = ( // runs scheduled url updates function flush(replace = shouldReplace) { - if (updateQueue.length === 0) return getCurrentUrl(); - const resultUrl = updateQueue.reduce((url, nextUpdate) => nextUpdate(url), getCurrentUrl()); + const nextUrl = getPendingUrl(); - cleanUp(); + if (!nextUrl) return; - const newUrl = updateUrl(resultUrl, replace); + cleanUp(); + const newUrl = updateUrl(nextUrl, replace); return newUrl; } + function getPendingUrl() { + if (updateQueue.length === 0) return undefined; + const resultUrl = updateQueue.reduce((url, nextUpdate) => nextUpdate(url), getCurrentUrl()); + + return resultUrl; + } + return { listen: (cb: () => void) => history.listen(() => { @@ -199,6 +215,9 @@ export const createKbnUrlControls = ( cancel: () => { cleanUp(); }, + getPendingUrl: () => { + return getPendingUrl(); + }, }; }; diff --git a/src/legacy/ui/public/state_management/utils/diff_object.test.ts b/src/plugins/kibana_utils/public/state_management/utils/diff_object.test.ts similarity index 100% rename from src/legacy/ui/public/state_management/utils/diff_object.test.ts rename to src/plugins/kibana_utils/public/state_management/utils/diff_object.test.ts diff --git a/src/legacy/ui/public/state_management/utils/diff_object.ts b/src/plugins/kibana_utils/public/state_management/utils/diff_object.ts similarity index 100% rename from src/legacy/ui/public/state_management/utils/diff_object.ts rename to src/plugins/kibana_utils/public/state_management/utils/diff_object.ts diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync.test.ts b/src/plugins/kibana_utils/public/state_sync/state_sync.test.ts index 08ad1551420d2..17f41483a0a21 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync.test.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync.test.ts @@ -291,6 +291,42 @@ describe('state_sync', () => { stop(); }); + + it("should preserve reference to unchanged state slices if them didn't change", async () => { + const otherUnchangedSlice = { a: 'test' }; + const oldState = { + todos: container.get().todos, + otherUnchangedSlice, + }; + container.set(oldState as any); + + const { stop, start } = syncStates([ + { + stateContainer: withDefaultState(container, defaultState), + storageKey: key, + stateStorage: urlSyncStrategy, + }, + ]); + await urlSyncStrategy.set('_s', container.get()); + expect(getCurrentUrl()).toMatchInlineSnapshot( + `"/#?_s=(otherUnchangedSlice:(a:test),todos:!((completed:!f,id:0,text:'Learning%20state%20containers')))"` + ); + start(); + + history.replace( + "/#?_s=(otherUnchangedSlice:(a:test),todos:!((completed:!t,id:0,text:'Learning%20state%20containers')))" + ); + + const newState = container.get(); + expect(newState.todos).toEqual([ + { id: 0, text: 'Learning state containers', completed: true }, + ]); + + // reference to unchanged slice is preserved + expect((newState as any).otherUnchangedSlice).toBe(otherUnchangedSlice); + + stop(); + }); }); }); diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync.ts b/src/plugins/kibana_utils/public/state_sync/state_sync.ts index 9c1116e5da531..28d133829e07c 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync.ts @@ -24,6 +24,7 @@ import { IStateSyncConfig } from './types'; import { IStateStorage } from './state_sync_state_storage'; import { distinctUntilChangedWithInitialValue } from '../../common'; import { BaseState } from '../state_containers'; +import { applyDiff } from '../state_management/utils/diff_object'; /** * Utility for syncing application state wrapped in state container @@ -100,7 +101,18 @@ export function syncState< const updateState = () => { const newState = stateStorage.get(storageKey); const oldState = stateContainer.get(); - if (!defaultComparator(newState, oldState)) { + if (newState) { + // apply only real differences to new state + const mergedState = { ...oldState } as State; + // merges into 'mergedState' all differences from newState, + // but leaves references if they are deeply the same + const diff = applyDiff(mergedState, newState); + + if (diff.keys.length > 0) { + stateContainer.set(mergedState); + } + } else if (oldState !== newState) { + // empty new state case stateContainer.set(newState); } }; diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts index 826122176e061..cc3f1df7c1e00 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.test.ts @@ -46,9 +46,11 @@ describe('KbnUrlStateStorage', () => { const key = '_s'; urlStateStorage.set(key, state); expect(getCurrentUrl()).toMatchInlineSnapshot(`"/"`); - urlStateStorage.flush(); + expect(urlStateStorage.flush()).toBe(true); expect(getCurrentUrl()).toMatchInlineSnapshot(`"/#?_s=(ok:1,test:test)"`); expect(urlStateStorage.get(key)).toEqual(state); + + expect(urlStateStorage.flush()).toBe(false); // nothing to flush, not update }); it('should cancel url updates', async () => { @@ -62,6 +64,19 @@ describe('KbnUrlStateStorage', () => { expect(urlStateStorage.get(key)).toEqual(null); }); + it('should cancel url updates if synchronously returned to the same state', async () => { + const state1 = { test: 'test', ok: 1 }; + const state2 = { test: 'test', ok: 2 }; + const key = '_s'; + const pr1 = urlStateStorage.set(key, state1); + await pr1; + const historyLength = history.length; + const pr2 = urlStateStorage.set(key, state2); + const pr3 = urlStateStorage.set(key, state1); + await Promise.all([pr2, pr3]); + expect(history.length).toBe(historyLength); + }); + it('should notify about url changes', async () => { expect(urlStateStorage.change$).toBeDefined(); const key = '_s'; diff --git a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts index 245006349ad55..082eaa5095ab9 100644 --- a/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts +++ b/src/plugins/kibana_utils/public/state_sync/state_sync_state_storage/create_kbn_url_state_storage.ts @@ -28,7 +28,11 @@ import { } from '../../state_management/url'; export interface IKbnUrlStateStorage extends IStateStorage { - set: (key: string, state: State, opts?: { replace: boolean }) => Promise; + set: ( + key: string, + state: State, + opts?: { replace: boolean } + ) => Promise; get: (key: string) => State | null; change$: (key: string) => Observable; @@ -36,7 +40,8 @@ export interface IKbnUrlStateStorage extends IStateStorage { cancel: () => void; // synchronously runs any pending url updates - flush: (opts?: { replace?: boolean }) => void; + // returned boolean indicates if change occurred + flush: (opts?: { replace?: boolean }) => boolean; } /** @@ -60,7 +65,11 @@ export const createKbnUrlStateStorage = ( replace ); }, - get: key => getStateFromKbnUrl(key), + get: key => { + // if there is a pending url update, then state will be extracted from that pending url, + // otherwise current url will be used to retrieve state from + return getStateFromKbnUrl(key, url.getPendingUrl()); + }, change$: (key: string) => new Observable(observer => { const unlisten = url.listen(() => { @@ -75,7 +84,7 @@ export const createKbnUrlStateStorage = ( share() ), flush: ({ replace = false }: { replace?: boolean } = {}) => { - url.flush(replace); + return !!url.flush(replace); }, cancel() { url.cancel(); diff --git a/test/functional/apps/dashboard/dashboard_clone.js b/test/functional/apps/dashboard/dashboard_clone.js index f5485c1db206e..8b7f6ba6a34dd 100644 --- a/test/functional/apps/dashboard/dashboard_clone.js +++ b/test/functional/apps/dashboard/dashboard_clone.js @@ -37,7 +37,7 @@ export default function({ getService, getPageObjects }) { await PageObjects.dashboard.addVisualizations( PageObjects.dashboard.getTestVisualizationNames() ); - await PageObjects.dashboard.enterDashboardTitleAndClickSave(dashboardName); + await PageObjects.dashboard.saveDashboard(dashboardName); await PageObjects.dashboard.clickClone(); await PageObjects.dashboard.confirmClone();