[APM] Make sure URL hooks are in sync with history.location (#36676) (#…

…37158)

* [APM] Make sure URL hooks are in sync with history.location

Previously, the `useUrlParams` and `useLocation` hooks were possibly running out of sync with history.location, as they maintained their own state. By listening to history changes, they would re-render their context providers with the updated values. However, the context providers are wrapped by the `Router` component which re-renders the context providers when the location changes. This means that the initial render after a location change would have the updated values in `history.location`, but not in the values returned from the `useUrlParams` and `useLocation` hooks. These hooks would then (both) queue another render with the updated values being passed to the context.

This change lets the hooks piggyback on the re-renders from the `Router` component, and uses `history.location` to pass their derived values to the context. This ensures that the values returned from the hooks and `history.location` are always in sync. It should also remove a few unnecessary renders.

Fix tests

- Passes history as a prop to URLParamsProvider, easier to test and consistent with usage of LocationProvider
- Do not test behaviour of history.replace & re-renders, only whether history.replace was called with the right Location values. This is because re-rendering is no longer a concern of URLParamsProvider, we leave that up to the Router component now.

Address review feedback:

- Use history/location from Router so we have one source of truth for routing info
- For UrlParamsContext, avoid use of history.replace. The behaviour w/ history.replace is incorrect. We now use useRef and trick React into re-rendering with an "empty" state update.
- Unit test for whether useUrlParams does not queue an unnecessary render.

Memoize context value of UrlParamsProvider

* Move infra items back to the top again

Piggybacking on this change to fix an order issue in TransactionActionMenu that was introduced w/ 2e3fbd2 (woops).

x-pack/plugins/apm/public/components/shared/DatePicker/__test__/DatePicker.test.tsx

@@ -16,6 +16,7 @@ import { IUrlParams } from '../../../../context/UrlParamsContext/types';
 import { history } from '../../../../utils/history';
 import { mount } from 'enzyme';
 import { EuiSuperDatePicker } from '@elastic/eui';
+import { MemoryRouter } from 'react-router-dom';
 
 const mockHistoryPush = jest.spyOn(history, 'push');
 const mockRefreshTimeRange = jest.fn();
@@ -34,11 +35,13 @@ const MockUrlParamsProvider: React.FC<{
 
 function mountDatePicker(params?: IUrlParams) {
   return mount(
-    <LocationProvider history={history}>
-      <MockUrlParamsProvider params={params}>
-        <DatePicker />
-      </MockUrlParamsProvider>
-    </LocationProvider>
+    <MemoryRouter initialEntries={[history.location]}>
+      <LocationProvider>
+        <MockUrlParamsProvider params={params}>
+          <DatePicker />
+        </MockUrlParamsProvider>
+      </LocationProvider>
+    </MemoryRouter>
   );
 }
 

x-pack/plugins/apm/public/components/shared/TransactionActionMenu/TransactionActionMenu.tsx

@@ -166,6 +166,7 @@ export const TransactionActionMenu: FunctionComponent<Props> = (
   });
 
   const menuItems = [
+    ...infraItems,
     {
       icon: 'discoverApp',
       key: 'discover-transaction',
@@ -181,7 +182,6 @@ export const TransactionActionMenu: FunctionComponent<Props> = (
         </DiscoverTransactionLink>
       )
     },
-    ...infraItems,
     {
       icon: 'uptimeApp',
       key: 'uptime',

x-pack/plugins/apm/public/context/LocationContext.tsx

@@ -4,28 +4,17 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import { History, Location } from 'history';
-import React, { createContext, useState, useEffect } from 'react';
-
-interface Props {
-  history: History;
-}
+import { Location } from 'history';
+import React, { createContext } from 'react';
+import { withRouter } from 'react-router-dom';
 
 const initialLocation = {} as Location;
 
 const LocationContext = createContext(initialLocation);
-const LocationProvider: React.FC<Props> = ({ history, children }) => {
-  const [location, setLocation] = useState(history.location);
-
-  useEffect(() => {
-    const unlisten = history.listen(updatedLocation => {
-      setLocation(updatedLocation);
-    });
-
-    return unlisten;
-  }, []);
-
-  return <LocationContext.Provider children={children} value={location} />;
-};
+const LocationProvider: React.ComponentClass<{}> = withRouter(
+  ({ location, children }) => {
+    return <LocationContext.Provider children={children} value={location} />;
+  }
+);
 
 export { LocationContext, LocationProvider };

x-pack/plugins/apm/public/context/UrlParamsContext/__tests__/UrlParamsContext.test.tsx

@@ -7,19 +7,22 @@
 import * as React from 'react';
 import { UrlParamsContext, UrlParamsProvider } from '..';
 import { mount } from 'enzyme';
-import * as hooks from '../../../hooks/useLocation';
-import { Location } from 'history';
+import { Location, History } from 'history';
+import { MemoryRouter, Router } from 'react-router-dom';
 import { IUrlParams } from '../types';
+import { tick } from '../../../utils/testHelpers';
 
-function mountParams() {
+function mountParams(location: Location) {
   return mount(
-    <UrlParamsProvider>
-      <UrlParamsContext.Consumer>
-        {({ urlParams }: { urlParams: IUrlParams }) => (
-          <span id="data">{JSON.stringify(urlParams, null, 2)}</span>
-        )}
-      </UrlParamsContext.Consumer>
-    </UrlParamsProvider>
+    <MemoryRouter initialEntries={[location]}>
+      <UrlParamsProvider>
+        <UrlParamsContext.Consumer>
+          {({ urlParams }: { urlParams: IUrlParams }) => (
+            <span id="data">{JSON.stringify(urlParams, null, 2)}</span>
+          )}
+        </UrlParamsContext.Consumer>
+      </UrlParamsProvider>
+    </MemoryRouter>
   );
 }
 
@@ -28,22 +31,13 @@ function getDataFromOutput(wrapper: ReturnType<typeof mount>) {
 }
 
 describe('UrlParamsContext', () => {
-  let mockLocation: Location;
-
-  beforeEach(() => {
-    mockLocation = { pathname: '/test/pathname' } as Location;
-    jest.spyOn(hooks, 'useLocation').mockImplementation(() => mockLocation);
-  });
-
-  afterEach(() => {
-    jest.restoreAllMocks();
-  });
-
   it('should have default params', () => {
+    const location = { pathname: '/test/pathname' } as Location;
+
     jest
       .spyOn(Date, 'now')
       .mockImplementation(() => new Date('2000-06-15T12:00:00Z').getTime());
-    const wrapper = mountParams();
+    const wrapper = mountParams(location);
     const params = getDataFromOutput(wrapper);
 
     expect(params).toEqual({
@@ -60,53 +54,80 @@ describe('UrlParamsContext', () => {
   });
 
   it('should read values in from location', () => {
-    mockLocation.search =
-      '?rangeFrom=2010-03-15T12:00:00Z&rangeTo=2010-04-10T12:00:00Z&transactionId=123abc';
-    const wrapper = mountParams();
+    const location = {
+      pathname: '/test/pathname',
+      search:
+        '?rangeFrom=2010-03-15T12:00:00Z&rangeTo=2010-04-10T12:00:00Z&transactionId=123abc'
+    } as Location;
+
+    const wrapper = mountParams(location);
     const params = getDataFromOutput(wrapper);
     expect(params.start).toEqual('2010-03-15T12:00:00.000Z');
     expect(params.end).toEqual('2010-04-10T12:00:00.000Z');
   });
 
   it('should update param values if location has changed', () => {
-    const wrapper = mountParams();
-    mockLocation = {
+    const location = {
       pathname: '/test/updated',
       search:
         '?rangeFrom=2009-03-15T12:00:00Z&rangeTo=2009-04-10T12:00:00Z&transactionId=UPDATED'
     } as Location;
+
+    const wrapper = mountParams(location);
+
     // force an update
     wrapper.setProps({ abc: 123 });
     const params = getDataFromOutput(wrapper);
     expect(params.start).toEqual('2009-03-15T12:00:00.000Z');
     expect(params.end).toEqual('2009-04-10T12:00:00.000Z');
   });
 
-  it('should refresh the time range with new values', () => {
+  it('should refresh the time range with new values', async () => {
+    const calls = [];
+    const history = ({
+      location: {
+        pathname: '/test'
+      },
+      listen: jest.fn()
+    } as unknown) as History;
+
     const wrapper = mount(
-      <UrlParamsProvider>
-        <UrlParamsContext.Consumer>
-          {({ urlParams, refreshTimeRange }) => {
-            return (
-              <React.Fragment>
-                <span id="data">{JSON.stringify(urlParams, null, 2)}</span>
-                <button
-                  onClick={() =>
-                    refreshTimeRange({
-                      rangeFrom: '2005-09-20T12:00:00Z',
-                      rangeTo: '2005-10-21T12:00:00Z'
-                    })
-                  }
-                />
-              </React.Fragment>
-            );
-          }}
-        </UrlParamsContext.Consumer>
-      </UrlParamsProvider>
+      <Router history={history}>
+        <UrlParamsProvider>
+          <UrlParamsContext.Consumer>
+            {({ urlParams, refreshTimeRange }) => {
+              calls.push({ urlParams });
+              return (
+                <React.Fragment>
+                  <span id="data">{JSON.stringify(urlParams, null, 2)}</span>
+                  <button
+                    onClick={() =>
+                      refreshTimeRange({
+                        rangeFrom: '2005-09-20T12:00:00Z',
+                        rangeTo: '2005-10-21T12:00:00Z'
+                      })
+                    }
+                  />
+                </React.Fragment>
+              );
+            }}
+          </UrlParamsContext.Consumer>
+        </UrlParamsProvider>
+      </Router>
     );
+
+    await tick();
+
+    expect(calls.length).toBe(1);
+
     wrapper.find('button').simulate('click');
-    const data = getDataFromOutput(wrapper);
-    expect(data.start).toEqual('2005-09-20T12:00:00.000Z');
-    expect(data.end).toEqual('2005-10-21T12:00:00.000Z');
+
+    await tick();
+
+    expect(calls.length).toBe(2);
+
+    const params = getDataFromOutput(wrapper);
+    expect(params.start).toEqual('2005-09-20T12:00:00.000Z');
+    expect(params.end).toEqual('2005-10-21T12:00:00.000Z');
   });
 });

x-pack/plugins/apm/public/context/UrlParamsContext/index.tsx

@@ -4,11 +4,16 @@
  * you may not use this file except in compliance with the Elastic License.
  */
 
-import React, { createContext, useReducer, useEffect, useMemo } from 'react';
-import { Location } from 'history';
-import { useLocation } from '../../hooks/useLocation';
+import React, {
+  createContext,
+  useMemo,
+  useCallback,
+  useRef,
+  useState
+} from 'react';
+import { withRouter } from 'react-router-dom';
+import { uniqueId } from 'lodash';
 import { IUrlParams } from './types';
-import { LOCATION_UPDATE, TIME_RANGE_REFRESH } from './constants';
 import { getParsedDate } from './helpers';
 import { resolveUrlParams } from './resolveUrlParams';
 import { UIFilters } from '../../../typings/ui-filters';
@@ -18,73 +23,67 @@ interface TimeRange {
   rangeTo: string;
 }
 
-interface LocationAction {
-  type: typeof LOCATION_UPDATE;
-  location: Location;
-}
-
-interface TimeRangeRefreshAction {
-  type: typeof TIME_RANGE_REFRESH;
-  time: TimeRange;
-}
-
 function useUiFilters({ kuery, environment }: IUrlParams): UIFilters {
   return useMemo(() => ({ kuery, environment }), [kuery, environment]);
 }
 
 const defaultRefresh = (time: TimeRange) => {};
 
-export function urlParamsReducer(
-  state: IUrlParams = {},
-  action: LocationAction | TimeRangeRefreshAction
-): IUrlParams {
-  switch (action.type) {
-    case LOCATION_UPDATE: {
-      return resolveUrlParams(action.location, state);
-    }
-
-    case TIME_RANGE_REFRESH:
-      return {
-        ...state,
-        start: getParsedDate(action.time.rangeFrom),
-        end: getParsedDate(action.time.rangeTo)
-      };
-
-    default:
-      return state;
-  }
-}
-
 const UrlParamsContext = createContext({
   urlParams: {} as IUrlParams,
   refreshTimeRange: defaultRefresh,
   uiFilters: {} as UIFilters
 });
 
-const UrlParamsProvider: React.FC<{}> = ({ children }) => {
-  const location = useLocation();
-  const [urlParams, dispatch] = useReducer(
-    urlParamsReducer,
-    resolveUrlParams(location, {})
-  );
-  const uiFilters = useUiFilters(urlParams);
-  const contextValue = React.useMemo(
-    () => ({ urlParams, refreshTimeRange, uiFilters }),
-    [urlParams]
-  );
+const UrlParamsProvider: React.ComponentClass<{}> = withRouter(
+  ({ location, children }) => {
+    const refUrlParams = useRef(resolveUrlParams(location, {}));
 
-  function refreshTimeRange(time: TimeRange) {
-    dispatch({ type: TIME_RANGE_REFRESH, time });
-  }
+    const [, forceUpdate] = useState('');
+
+    const urlParams = useMemo(
+      () =>
+        resolveUrlParams(location, {
+          start: refUrlParams.current.start,
+          end: refUrlParams.current.end,
+          rangeFrom: refUrlParams.current.rangeFrom,
+          rangeTo: refUrlParams.current.rangeTo
+        }),
+      [location, refUrlParams.current]
+    );
+
+    refUrlParams.current = urlParams;
 
-  useEffect(
-    () => {
-      dispatch({ type: LOCATION_UPDATE, location });
-    },
-    [location]
-  );
+    const refreshTimeRange = useCallback(
+      (timeRange: TimeRange) => {
+        refUrlParams.current = {
+          ...refUrlParams.current,
+          start: getParsedDate(timeRange.rangeFrom),
+          end: getParsedDate(timeRange.rangeTo)
+        };
 
-  return <UrlParamsContext.Provider children={children} value={contextValue} />;
-};
+        forceUpdate(uniqueId());
+      },
+      [forceUpdate]
+    );
+
+    const uiFilters = useUiFilters(urlParams);
+
+    const contextValue = useMemo(
+      () => {
+        return {
+          urlParams,
+          refreshTimeRange,
+          uiFilters
+        };
+      },
+      [urlParams, refreshTimeRange, uiFilters]
+    );
+
+    return (
+      <UrlParamsContext.Provider children={children} value={contextValue} />
+    );
+  }
+);
 
 export { UrlParamsContext, UrlParamsProvider, useUiFilters };