[State Management] ScopedHistory support for state syncing utils (#62761

)

The needed change is to rely on history as source of truth for location instead of window.location.

btw, This makes possible to test state syncing utils integration using createMemoryHistory()

One issue was discovered after this change:
When switching from context to discover url was incorrect. history.location inside state syncing utils didn't get the last update. This happened, because history instance created in discover wasn't used in context app and when all listeners unsubscribed from it - it stopped receiving location updates. To fix this I just reused one history instance in discover, context and their kbnUrlTracker

examples/state_containers_examples/public/todo/app.tsx

@@ -20,7 +20,7 @@
 import { AppMountParameters } from 'kibana/public';
 import ReactDOM from 'react-dom';
 import React from 'react';
-import { createHashHistory, createBrowserHistory } from 'history';
+import { createHashHistory } from 'history';
 import { TodoAppPage } from './todo';
 
 export interface AppOptions {
@@ -35,13 +35,10 @@ export enum History {
 }
 
 export const renderApp = (
-  { appBasePath, element }: AppMountParameters,
+  { appBasePath, element, history: platformHistory }: AppMountParameters,
   { appInstanceId, appTitle, historyType }: AppOptions
 ) => {
-  const history =
-    historyType === History.Browser
-      ? createBrowserHistory({ basename: appBasePath })
-      : createHashHistory();
+  const history = historyType === History.Browser ? platformHistory : createHashHistory();
   ReactDOM.render(
     <TodoAppPage
       history={history}
@@ -54,8 +51,7 @@ export const renderApp = (
         const currentAppUrl = stripTrailingSlash(history.createHref(history.location));
         if (historyType === History.Browser) {
           // browser history
-          const basePath = stripTrailingSlash(appBasePath);
-          return currentAppUrl === basePath && !history.location.search && !history.location.hash;
+          return currentAppUrl === '' && !history.location.search && !history.location.hash;
         } else {
           // hashed history
           return currentAppUrl === '#' && !history.location.search;

examples/state_containers_examples/public/todo/todo.tsx

@@ -91,17 +91,20 @@ const TodoApp: React.FC<TodoAppProps> = ({ filter }) => {
   return (
     <>
       <div>
-        <Link to={{ ...location, pathname: '/' }}>
+        <Link to={{ ...location, pathname: '/' }} data-test-subj={'filterLinkAll'}>
           <EuiButton size={'s'} color={!filter ? 'primary' : 'secondary'}>
             All
           </EuiButton>
         </Link>
-        <Link to={{ ...location, pathname: '/completed' }}>
+        <Link to={{ ...location, pathname: '/completed' }} data-test-subj={'filterLinkCompleted'}>
           <EuiButton size={'s'} color={filter === 'completed' ? 'primary' : 'secondary'}>
             Completed
           </EuiButton>
         </Link>
-        <Link to={{ ...location, pathname: '/not-completed' }}>
+        <Link
+          to={{ ...location, pathname: '/not-completed' }}
+          data-test-subj={'filterLinkNotCompleted'}
+        >
           <EuiButton size={'s'} color={filter === 'not-completed' ? 'primary' : 'secondary'}>
             Not Completed
           </EuiButton>
@@ -121,6 +124,7 @@ const TodoApp: React.FC<TodoAppProps> = ({ filter }) => {
                 });
               }}
               label={todo.text}
+              data-test-subj={`todoCheckbox-${todo.id}`}
             />
             <EuiButton
               style={{ marginLeft: '8px' }}

examples/state_containers_examples/public/with_data_services/application.tsx

@@ -19,7 +19,6 @@
 
 import React from 'react';
 import ReactDOM from 'react-dom';
-import { createBrowserHistory } from 'history';
 import { AppMountParameters, CoreStart } from '../../../../src/core/public';
 import { AppPluginDependencies } from './types';
 import { StateDemoApp } from './components/app';
@@ -28,9 +27,8 @@ import { createKbnUrlStateStorage } from '../../../../src/plugins/kibana_utils/p
 export const renderApp = (
   { notifications, http }: CoreStart,
   { navigation, data }: AppPluginDependencies,
-  { appBasePath, element }: AppMountParameters
+  { appBasePath, element, history }: AppMountParameters
 ) => {
-  const history = createBrowserHistory({ basename: appBasePath });
   const kbnUrlStateStorage = createKbnUrlStateStorage({ useHash: false, history });
 
   ReactDOM.render(

src/legacy/core_plugins/kibana/public/discover/build_services.ts

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { createHashHistory, History } from 'history';
+import { History } from 'history';
 
 import {
   Capabilities,
@@ -51,7 +51,7 @@ export interface DiscoverServices {
   data: DataPublicPluginStart;
   docLinks: DocLinksStart;
   DocViewer: DocViewerComponent;
-  history: History;
+  history: () => History;
   theme: ChartsPluginStart['theme'];
   filterManager: FilterManager;
   indexPatterns: IndexPatternsContract;
@@ -67,7 +67,8 @@ export interface DiscoverServices {
 }
 export async function buildServices(
   core: CoreStart,
-  plugins: DiscoverStartPlugins
+  plugins: DiscoverStartPlugins,
+  getHistory: () => History
 ): Promise<DiscoverServices> {
   const services = {
     savedObjectsClient: core.savedObjects.client,
@@ -77,6 +78,7 @@ export async function buildServices(
     overlays: core.overlays,
   };
   const savedObjectService = createSavedSearchesLoader(services);
+
   return {
     addBasePath: core.http.basePath.prepend,
     capabilities: core.application.capabilities,
@@ -85,11 +87,11 @@ export async function buildServices(
     data: plugins.data,
     docLinks: core.docLinks,
     DocViewer: plugins.discover.docViews.DocViewer,
-    history: createHashHistory(),
     theme: plugins.charts.theme,
     filterManager: plugins.data.query.filterManager,
     getSavedSearchById: async (id: string) => savedObjectService.get(id),
     getSavedSearchUrlById: async (id: string) => savedObjectService.urlFor(id),
+    history: getHistory,
     indexPatterns: plugins.data.indexPatterns,
     inspector: plugins.inspector,
     // @ts-ignore

src/legacy/core_plugins/kibana/public/discover/kibana_services.ts

@@ -16,6 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { createHashHistory } from 'history';
 import { DiscoverServices } from './build_services';
 import { createGetterSetter } from '../../../../../plugins/kibana_utils/public';
 import { search } from '../../../../../plugins/data/public';
@@ -52,6 +53,11 @@ export const [getUrlTracker, setUrlTracker] = createGetterSetter<{
   setTrackedUrl: (url: string) => void;
 }>('urlTracker');
 
+/**
+ * Makes sure discover and context are using one instance of history
+ */
+export const getHistory = _.once(() => createHashHistory());
+
 export const { getRequestInspectorStats, getResponseInspectorStats, tabifyAggResponse } = search;
 export {
   unhashUrl,

src/legacy/core_plugins/kibana/public/discover/np_ready/angular/context.js

@@ -81,6 +81,7 @@ function ContextAppRouteController($routeParams, $scope, $route) {
     defaultStepSize: getServices().uiSettings.get('context:defaultSize'),
     timeFieldName: indexPattern.timeFieldName,
     storeInSessionStorage: getServices().uiSettings.get('state:storeInSessionStorage'),
+    history: getServices().history(),
   });
   this.state = { ...appState.getState() };
   this.anchorId = $routeParams.id;

src/legacy/core_plugins/kibana/public/discover/np_ready/angular/context_state.ts

@@ -17,7 +17,7 @@
  * under the License.
  */
 import _ from 'lodash';
-import { createBrowserHistory, History } from 'history';
+import { History } from 'history';
 import {
   createStateContainer,
   createKbnUrlStateStorage,
@@ -71,9 +71,9 @@ interface GetStateParams {
    */
   storeInSessionStorage?: boolean;
   /**
-   * Browser history used for testing
+   * History instance to use
    */
-  history?: History;
+  history: History;
 }
 
 interface GetStateReturn {
@@ -126,7 +126,7 @@ export function getState({
 }: GetStateParams): GetStateReturn {
   const stateStorage = createKbnUrlStateStorage({
     useHash: storeInSessionStorage,
-    history: history ? history : createBrowserHistory(),
+    history,
   });
 
   const globalStateInitial = stateStorage.get(GLOBAL_STATE_URL_KEY) as GlobalState;

src/legacy/core_plugins/kibana/public/discover/np_ready/angular/discover.js

@@ -57,7 +57,7 @@ const {
   core,
   chrome,
   data,
-  history,
+  history: getHistory,
   indexPatterns,
   filterManager,
   share,
@@ -116,6 +116,7 @@ app.config($routeProvider => {
     reloadOnSearch: false,
     resolve: {
       savedObjects: function($route, Promise) {
+        const history = getHistory();
         const savedSearchId = $route.current.params.id;
         return ensureDefaultIndexPattern(core, data, history).then(() => {
           const { appStateContainer } = getState({ history });
@@ -204,6 +205,8 @@ function discoverController(
     return isDefaultType($scope.indexPattern) ? $scope.indexPattern.timeFieldName : undefined;
   };
 
+  const history = getHistory();
+
   const {
     appStateContainer,
     startSync: startStateSync,

src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field.test.tsx

@@ -31,11 +31,11 @@ import { IndexPatternField } from '../../../../../../../../plugins/data/public';
 
 jest.mock('../../../kibana_services', () => ({
   getServices: () => ({
-    history: {
+    history: () => ({
       location: {
         search: '',
       },
-    },
+    }),
     capabilities: {
       visualize: {
         show: true,

src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_sidebar.test.tsx

@@ -36,11 +36,11 @@ import { SavedObject } from '../../../../../../../../core/types';
 
 jest.mock('../../../kibana_services', () => ({
   getServices: () => ({
-    history: {
+    history: () => ({
       location: {
         search: '',
       },
-    },
+    }),
     capabilities: {
       visualize: {
         show: true,

src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/lib/visualize_url_utils.ts

@@ -125,7 +125,7 @@ export function getVisualizeUrl(
   services: DiscoverServices
 ) {
   const aggsTermSize = services.uiSettings.get('discover:aggs:terms:size');
-  const urlParams = parse(services.history.location.search) as Record<string, string>;
+  const urlParams = parse(services.history().location.search) as Record<string, string>;
 
   if (
     (field.type === KBN_FIELD_TYPES.GEO_POINT || field.type === KBN_FIELD_TYPES.GEO_SHAPE) &&

src/legacy/core_plugins/kibana/public/discover/plugin.ts

@@ -31,7 +31,7 @@ import { registerFeature } from './np_ready/register_feature';
 import './kibana_services';
 import { EmbeddableStart, EmbeddableSetup } from '../../../../../plugins/embeddable/public';
 import { getInnerAngularModule, getInnerAngularModuleEmbeddable } from './get_inner_angular';
-import { setAngularModule, setServices, setUrlTracker } from './kibana_services';
+import { getHistory, setAngularModule, setServices, setUrlTracker } from './kibana_services';
 import { NavigationPublicPluginStart as NavigationStart } from '../../../../../plugins/navigation/public';
 import { ChartsPluginStart } from '../../../../../plugins/charts/public';
 import { buildServices } from './build_services';
@@ -98,6 +98,10 @@ export class DiscoverPlugin implements Plugin<void, void> {
       stop: stopUrlTracker,
       setActiveUrl: setTrackedUrl,
     } = createKbnUrlTracker({
+      // we pass getter here instead of plain `history`,
+      // so history is lazily created (when app is mounted)
+      // this prevents redundant `#` when not in discover app
+      getHistory,
       baseUrl: core.http.basePath.prepend('/app/kibana'),
       defaultSubUrl: '#/discover',
       storageKey: `lastUrl:${core.http.basePath.get()}:discover`,
@@ -174,7 +178,7 @@ export class DiscoverPlugin implements Plugin<void, void> {
       if (this.servicesInitialized) {
         return { core, plugins };
       }
-      const services = await buildServices(core, plugins);
+      const services = await buildServices(core, plugins, getHistory);
       setServices(services);
       this.servicesInitialized = true;
 

src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.test.ts

@@ -31,6 +31,7 @@ import {
   setStateToKbnUrl,
   getStateFromKbnUrl,
 } from './kbn_url_storage';
+import { ScopedHistory } from '../../../../../core/public';
 
 describe('kbn_url_storage', () => {
   describe('getStateFromUrl & setStateToUrl', () => {
@@ -187,23 +188,54 @@ describe('kbn_url_storage', () => {
       urlControls.update('/', true);
     });
 
-    const getCurrentUrl = () => window.location.href;
+    const getCurrentUrl = () => history.createHref(history.location);
 
     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');
+      expect(getCurrentUrl()).toBe('/');
+      expect(urlControls.flush()).toBe('/3');
+      expect(getCurrentUrl()).toBe('/3');
+      await Promise.all([pr1, pr2, pr3]);
+      expect(getCurrentUrl()).toBe('/3');
+    });
+
+    it('flush() should return undefined, if no url updates happened', () => {
+      expect(urlControls.flush()).toBeUndefined();
+      urlControls.updateAsync(() => '/1', false);
+      urlControls.updateAsync(() => '/', false);
+      expect(urlControls.flush()).toBeUndefined();
+    });
+  });
+
+  describe('urlControls - scoped history integration', () => {
+    let history: History;
+    let urlControls: IKbnUrlControls;
+    beforeEach(() => {
+      const parentHistory = createBrowserHistory();
+      parentHistory.replace('/app/kibana/');
+      history = new ScopedHistory(parentHistory, '/app/kibana/');
+      urlControls = createKbnUrlControls(history);
+    });
+
+    const getCurrentUrl = () => history.createHref(history.location);
+
+    it('should flush async url updates', async () => {
+      const pr1 = urlControls.updateAsync(() => '/app/kibana/1', false);
+      const pr2 = urlControls.updateAsync(() => '/app/kibana/2', false);
+      const pr3 = urlControls.updateAsync(() => '/app/kibana/3', false);
+      expect(getCurrentUrl()).toBe('/app/kibana/');
+      expect(urlControls.flush()).toBe('/app/kibana/3');
+      expect(getCurrentUrl()).toBe('/app/kibana/3');
       await Promise.all([pr1, pr2, pr3]);
-      expect(getCurrentUrl()).toBe('http://localhost/3');
+      expect(getCurrentUrl()).toBe('/app/kibana/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);
+      urlControls.updateAsync(() => '/app/kibana/1', false);
+      urlControls.updateAsync(() => '/app/kibana/', false);
       expect(urlControls.flush()).toBeUndefined();
     });
   });

src/plugins/kibana_utils/public/state_management/url/kbn_url_storage.ts

@@ -154,7 +154,7 @@ export const createKbnUrlControls = (
   let shouldReplace = true;
 
   function updateUrl(newUrl: string, replace = false): string | undefined {
-    const currentUrl = getCurrentUrl();
+    const currentUrl = getCurrentUrl(history);
     if (newUrl === currentUrl) return undefined; // skip update
 
     const historyPath = getRelativeToHistoryPath(newUrl, history);
@@ -165,7 +165,7 @@ export const createKbnUrlControls = (
       history.push(historyPath);
     }
 
-    return getCurrentUrl();
+    return getCurrentUrl(history);
   }
 
   // queue clean up
@@ -187,7 +187,10 @@ export const createKbnUrlControls = (
 
   function getPendingUrl() {
     if (updateQueue.length === 0) return undefined;
-    const resultUrl = updateQueue.reduce((url, nextUpdate) => nextUpdate(url), getCurrentUrl());
+    const resultUrl = updateQueue.reduce(
+      (url, nextUpdate) => nextUpdate(url),
+      getCurrentUrl(history)
+    );
 
     return resultUrl;
   }