From 5073715438319c62c3698696e870ce3bf811e6ed Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Tue, 14 Jan 2020 12:33:02 +0100 Subject: [PATCH 1/8] Remove AppState from np/dashboard improve improve improve improve --- .../filter_manager/filter_state_manager.ts | 16 +- .../public/dashboard/__tests__/index.ts | 1 - .../dashboard/np_ready/dashboard_app.tsx | 16 +- .../np_ready/dashboard_app_controller.tsx | 106 ++++++--- .../np_ready/dashboard_state.test.ts | 12 +- .../np_ready/dashboard_state_manager.ts | 207 +++++++++++------- .../np_ready/lib/migrate_app_state.test.ts | 35 +-- .../np_ready/lib/migrate_app_state.ts | 15 +- .../dashboard/np_ready/lib/save_dashboard.ts | 2 - .../np_ready/lib/update_saved_dashboard.ts | 4 +- .../kibana/public/dashboard/np_ready/types.ts | 37 ++-- .../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 | 1 + .../url/kbn_url_storage.test.ts | 54 ++++- .../state_management/url/kbn_url_storage.ts | 41 +++- .../create_kbn_url_state_storage.test.ts | 17 +- .../create_kbn_url_state_storage.ts | 17 +- 19 files changed, 468 insertions(+), 251 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%) 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..d0f467c0a2a35 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,10 +18,11 @@ */ import _ from 'lodash'; +import { Subscription } from 'rxjs'; import { State } from 'ui/state_management/state'; import { FilterManager, esFilters } from '../../../../../../plugins/data/public'; -type GetAppStateFunc = () => State | undefined | null; +type GetAppStateFunc = () => { filters?: esFilters.Filter[]; save?: () => void } | undefined | null; /** * FilterStateManager is responsible for watching for filter changes @@ -29,6 +30,8 @@ type GetAppStateFunc = () => State | undefined | null; * back to the URL. **/ export class FilterStateManager { + private subs: Subscription[] = []; + filterManager: FilterManager; globalState: State; getAppState: GetAppStateFunc; @@ -41,15 +44,18 @@ export class FilterStateManager { this.watchFilterState(); - this.filterManager.getUpdates$().subscribe(() => { - this.updateAppState(); - }); + this.subs.push( + this.filterManager.getUpdates$().subscribe(() => { + this.updateAppState(); + }) + ); } destroy() { if (this.interval) { clearInterval(this.interval); } + this.subs.forEach(s => s.unsubscribe()); } private watchFilterState() { @@ -80,7 +86,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/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..c374fb6da3f6f 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,56 @@ * 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, + SaveResult, + showSaveModal, + State, + subscribeWithScope, unhashUrl, } 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 +77,15 @@ import { SavedObjectFinderProps, SavedObjectFinderUi, } from '../../../../../../plugins/kibana_react/public'; +import { removeQueryParam } 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 +100,9 @@ export class DashboardAppController { $scope, $route, $routeParams, - getAppState, globalState, dashboardConfig, localStorage, - kbnUrl, - AppStateClass, indexPatterns, config, confirmModal, @@ -124,7 +118,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 +127,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 +325,8 @@ export class DashboardAppController { dirty = true; } + dashboardStateManager.handleDashboardContainerChanges(container); $scope.$evalAsync(() => { - dashboardStateManager.handleDashboardContainerChanges(container); if (dirty) { updateState(); } @@ -337,8 +346,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 +418,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); } }); @@ -559,6 +570,22 @@ export class DashboardAppController { }) ); + // 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 + function changeUrl(pathname: string) { + // synchronously persist current state to url with push() + const updated = dashboardStateManager.saveState({ replace: false }); + // change pathname + history[updated ? 'replace' : 'push']({ + ...history.location, + pathname, + }); + } + function updateViewMode(newMode: ViewMode) { $scope.topNavMenu = getTopNavConfig( newMode, @@ -580,9 +607,6 @@ 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); @@ -592,6 +616,10 @@ export class DashboardAppController { if (dashboardStateManager.getIsTimeSavedWithDashboard()) { dashboardStateManager.syncTimefilterWithDashboard(timefilter); } + + changeUrl( + dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL + ); } confirmModal( @@ -642,7 +670,12 @@ export class DashboardAppController { }); if (dash.id !== $routeParams.id) { - kbnUrl.change(createDashboardEditUrl(dash.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 + + changeUrl(createDashboardEditUrl(dash.id)); } else { chrome.docTitle.change(dash.lastSavedTitle); updateViewMode(ViewMode.VIEW); @@ -844,6 +877,12 @@ export class DashboardAppController { }); }); + dashboardStateManager.registerChangeListener(() => { + // some angular dependant properties could have changed: e.g. view mode or full screen mode + // run digest cycle to be sure, we are not missing those update on UI + $scope.$applyAsync(); + }); + $scope.$on('$destroy', () => { updateSubscription.unsubscribe(); visibleSubscription.unsubscribe(); @@ -859,6 +898,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 4d5101e1f9e5f..01064695e3798 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 @@ -19,16 +19,10 @@ import './np_core.test.mocks'; 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: {}, -})); - describe('DashboardState', function() { let dashboardState: DashboardStateManager; const savedDashboard = getSavedDashboardMock(); @@ -46,7 +40,7 @@ describe('DashboardState', function() { function initDashboardState() { dashboardState = new DashboardStateManager({ savedDashboard, - AppStateClass: getAppStateMock() as AppStateClass, + useHashedUrl: false, hideWriteControls: false, kibanaVersion: '7.0.0', }); 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..934fbdaa04da6 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,76 @@ 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; /** * * @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.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.appState = new AppStateClass(this.stateDefaults); + this.kbnUrlStateStorage = createKbnUrlStateStorage({ useHash: useHashedUrl, history }); + + // 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 +150,32 @@ 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)); + // 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 +221,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 +233,6 @@ export class DashboardStateManager { } this.changeListeners.forEach(listener => listener({ dirty })); - this.saveState(); } public getFullScreenMode() { @@ -192,8 +240,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); } /** @@ -218,9 +269,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 +301,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 +333,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 +341,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 +349,7 @@ export class DashboardStateManager { } public setTimeRestore(timeRestore: boolean) { - this.appState.timeRestore = timeRestore; - this.saveState(); + this.stateContainer.transitions.set('timeRestore', timeRestore); } public getIsTimeSavedWithDashboard() { @@ -397,7 +440,6 @@ export class DashboardStateManager { (panel: SavedDashboardPanel) => panel.panelIndex === panelIndex ); Object.assign(foundPanel, panelAttributes); - this.saveState(); return foundPanel; } @@ -456,15 +498,21 @@ 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(); + public saveState({ replace = false }: { replace: boolean } = { replace: false }): 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 }); } public setQuery(query: Query) { - this.appState.query = query; - this.saveState(); + this.stateContainer.transitions.set('query', query); } /** @@ -472,24 +520,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/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..3f1b6e546fece 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,8 +36,6 @@ export function saveDashboard( dashboardStateManager: DashboardStateManager, saveOptions: SavedObjectSaveOpts ): Promise { - dashboardStateManager.saveState(); - const savedDashboard = dashboardStateManager.savedDashboard; const appState = dashboardStateManager.appState; 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..baf2a41573fec 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,11 @@ export type SavedDashboardPanelTo60 = Pick< readonly type: string; }; -export type DashboardAppStateDefaults = DashboardAppStateParameters & { +export type DashboardAppStateDefaults = DashboardAppState & { description?: string; }; -export interface DashboardAppStateParameters { +export interface DashboardAppState { panels: SavedDashboardPanel[]; fullScreenMode: boolean; title: string; @@ -113,9 +112,20 @@ 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 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/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 0ba444c4e9395..8bb8c6f209aa7 100644 --- a/src/plugins/kibana_utils/public/index.ts +++ b/src/plugins/kibana_utils/public/index.ts @@ -58,3 +58,4 @@ export { StartSyncStateFnType, StopSyncStateFnType, } from './state_sync'; +export { removeQueryParam } from './history'; 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/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(); From 5b042dbefbaa3f51e83f7ad6fc3a63117147158b Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Wed, 15 Jan 2020 11:53:28 +0100 Subject: [PATCH 2/8] fix hard reseting time filters --- .../np_ready/dashboard_app_controller.tsx | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) 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 c374fb6da3f6f..dc1019b7d1aa2 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 @@ -610,16 +610,22 @@ export class DashboardAppController { // This is only necessary for new dashboards, which will default to Edit mode. updateViewMode(ViewMode.VIEW); + changeUrl( + 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); + }); } - - changeUrl( - dash.id ? createDashboardEditUrl(dash.id) : DashboardConstants.CREATE_NEW_DASHBOARD_URL - ); } confirmModal( From fb47fc801e9756e2c18cff6d10c739c965395297 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Wed, 15 Jan 2020 12:37:32 +0100 Subject: [PATCH 3/8] make sure url ('_a') matches initial state --- .../public/dashboard/np_ready/dashboard_state_manager.ts | 3 +++ 1 file changed, 3 insertions(+) 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 934fbdaa04da6..b7762134391a7 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 @@ -157,6 +157,9 @@ export class DashboardStateManager { this.changeListeners.forEach(listener => listener({ dirty: this.isDirty })); }); + // 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, From d15cd9bdc71f5a2110bd019eb1dc7b5396bedf4d Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Fri, 17 Jan 2020 18:03:16 +0100 Subject: [PATCH 4/8] improvements according to review --- .../filter_manager/filter_state_manager.ts | 16 +++---- .../kibana/public/dashboard/legacy_imports.ts | 6 --- .../np_ready/dashboard_app_controller.tsx | 44 ++++++------------- .../np_ready/dashboard_state.test.ts | 2 + .../np_ready/dashboard_state_manager.ts | 27 ++++++++++-- .../public/dashboard/np_ready/legacy_app.js | 12 +++-- src/legacy/ui/public/chrome/api/controls.ts | 2 + .../ui/public/chrome/directives/kbn_chrome.js | 10 +++++ .../ui/public/state_management/state.js | 2 +- src/plugins/kibana_utils/public/index.ts | 1 + .../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 +++++- 14 files changed, 119 insertions(+), 53 deletions(-) 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 d0f467c0a2a35..05f225543be8e 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 @@ -30,12 +30,12 @@ type GetAppStateFunc = () => { filters?: esFilters.Filter[]; save?: () => void } * back to the URL. **/ export class FilterStateManager { - private subs: Subscription[] = []; + 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; @@ -44,24 +44,22 @@ export class FilterStateManager { this.watchFilterState(); - this.subs.push( - this.filterManager.getUpdates$().subscribe(() => { - this.updateAppState(); - }) - ); + this.filterManagerUpdatesSubscription = this.filterManager.getUpdates$().subscribe(() => { + this.updateAppState(); + }); } destroy() { if (this.interval) { clearInterval(this.interval); } - this.subs.forEach(s => s.unsubscribe()); + 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; 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/dashboard_app_controller.tsx b/src/legacy/core_plugins/kibana/public/dashboard/np_ready/dashboard_app_controller.tsx index dc1019b7d1aa2..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 @@ -34,7 +34,6 @@ import { showSaveModal, State, subscribeWithScope, - unhashUrl, } from '../legacy_imports'; import { FilterStateManager } from '../../../../data/public'; import { @@ -77,7 +76,7 @@ import { SavedObjectFinderProps, SavedObjectFinderUi, } from '../../../../../../plugins/kibana_react/public'; -import { removeQueryParam } from '../../../../../../plugins/kibana_utils/public'; +import { removeQueryParam, unhashUrl } from '../../../../../../plugins/kibana_utils/public'; export interface DashboardAppControllerDependencies extends RenderDeps { $scope: DashboardAppScope; @@ -570,28 +569,7 @@ export class DashboardAppController { }) ); - // 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 - function changeUrl(pathname: string) { - // synchronously persist current state to url with push() - const updated = dashboardStateManager.saveState({ replace: false }); - // change pathname - history[updated ? 'replace' : 'push']({ - ...history.location, - pathname, - }); - } - function updateViewMode(newMode: ViewMode) { - $scope.topNavMenu = getTopNavConfig( - newMode, - navActions, - dashboardConfig.getHideWriteControls() - ); // eslint-disable-line no-use-before-define dashboardStateManager.switchViewMode(newMode); } @@ -610,7 +588,11 @@ export class DashboardAppController { // This is only necessary for new dashboards, which will default to Edit mode. updateViewMode(ViewMode.VIEW); - changeUrl( + // 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 ); @@ -676,12 +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 - - changeUrl(createDashboardEditUrl(dash.id)); + // 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); @@ -884,9 +865,12 @@ export class DashboardAppController { }); dashboardStateManager.registerChangeListener(() => { - // some angular dependant properties could have changed: e.g. view mode or full screen mode - // run digest cycle to be sure, we are not missing those update on UI - $scope.$applyAsync(); + // view mode could have changed, so trigger top nav update + $scope.topNavMenu = getTopNavConfig( + dashboardStateManager.getViewMode(), + navActions, + dashboardConfig.getHideWriteControls() + ); }); $scope.$on('$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 9bc5a456e0dc7..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,6 +18,7 @@ */ import './np_core.test.mocks'; +import { createBrowserHistory } from 'history'; import { DashboardStateManager } from './dashboard_state_manager'; import { getSavedDashboardMock } from '../__tests__'; import { InputTimeRange, TimefilterContract, TimeRange } from 'src/plugins/data/public'; @@ -50,6 +51,7 @@ describe('DashboardState', function() { 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 b7762134391a7..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 @@ -84,6 +84,7 @@ export class DashboardStateManager { private readonly STATE_STORAGE_KEY = '_a'; private readonly kbnUrlStateStorage: IKbnUrlStateStorage; private readonly stateSyncRef: ISyncStateRef; + private readonly history: History; /** * @@ -103,8 +104,9 @@ export class DashboardStateManager { hideWriteControls: boolean; kibanaVersion: string; useHashedUrl: boolean; - history?: History; + history: History; }) { + this.history = history; this.kibanaVersion = kibanaVersion; this.savedDashboard = savedDashboard; this.hideWriteControls = hideWriteControls; @@ -264,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; @@ -504,7 +509,7 @@ export class DashboardStateManager { * Synchronously writes current state to url * returned boolean indicates whether the update happened and if history was updated */ - public saveState({ replace = false }: { replace: boolean } = { replace: false }): boolean { + private saveState({ replace }: { replace: boolean }): boolean { // schedules setting current state to url this.kbnUrlStateStorage.set( this.STATE_STORAGE_KEY, @@ -514,6 +519,22 @@ export class DashboardStateManager { 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.stateContainer.transitions.set('query', query); } 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/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/index.ts b/src/plugins/kibana_utils/public/index.ts index 8b5da3fc98ceb..00c1c95028b4d 100644 --- a/src/plugins/kibana_utils/public/index.ts +++ b/src/plugins/kibana_utils/public/index.ts @@ -59,3 +59,4 @@ export { StopSyncStateFnType, } from './state_sync'; export { removeQueryParam } from './history'; +export { applyDiff } from './state_management/utils/diff_object'; 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); } }; From 10fe7772a3297f9e5b03591dd9d47a5b8ca9be02 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Fri, 17 Jan 2020 18:09:05 +0100 Subject: [PATCH 5/8] clean up AppState imports --- .../kibana/public/dashboard/np_ready/application.ts | 7 ------- 1 file changed, 7 deletions(-) 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); }); From 1257fb1fd181e1195372adde350b2b66e62d251e Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Mon, 20 Jan 2020 11:04:23 +0100 Subject: [PATCH 6/8] fix used before declare --- .../kibana/public/dashboard/np_ready/types.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 baf2a41573fec..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 @@ -92,10 +92,6 @@ export type SavedDashboardPanelTo60 = Pick< readonly type: string; }; -export type DashboardAppStateDefaults = DashboardAppState & { - description?: string; -}; - export interface DashboardAppState { panels: SavedDashboardPanel[]; fullScreenMode: boolean; @@ -112,6 +108,10 @@ export interface DashboardAppState { savedQuery?: string; } +export type DashboardAppStateDefaults = DashboardAppState & { + description?: string; +}; + export interface DashboardAppStateTransitions { set: ( state: DashboardAppState From 6e71ffbd44c86435b9f9df26cc7ca740768c8d4e Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Mon, 20 Jan 2020 12:43:07 +0100 Subject: [PATCH 7/8] fixing test --- .../public/dashboard/np_ready/lib/save_dashboard.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 3f1b6e546fece..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 @@ -42,8 +42,13 @@ export function saveDashboard( 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; }); } From 23845a58d6a7434572a413d871cec7026c290df8 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Mon, 20 Jan 2020 13:37:14 +0100 Subject: [PATCH 8/8] improve stability of dashboard clone test --- test/functional/apps/dashboard/dashboard_clone.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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();