[ML] Improvements for urlState hook.

x-pack/plugins/ml/public/application/util/url_state.test.ts

@@ -5,7 +5,7 @@
  */
 
 import { renderHook, act } from '@testing-library/react-hooks';
-import { getUrlState, useUrlState } from './url_state';
+import { parseUrlState, useUrlState } from './url_state';
 
 const mockHistoryPush = jest.fn();
 
@@ -22,7 +22,7 @@ jest.mock('react-router-dom', () => ({
 describe('getUrlState', () => {
   test('properly decode url with _g and _a', () => {
     expect(
-      getUrlState(
+      parseUrlState(
         "?_a=(mlExplorerFilter:(),mlExplorerSwimlane:(viewByFieldName:action),query:(query_string:(analyze_wildcard:!t,query:'*')))&_g=(ml:(jobIds:!(dec-2)),refreshInterval:(display:Off,pause:!f,value:0),time:(from:'2019-01-01T00:03:40.000Z',mode:absolute,to:'2019-08-30T11:55:07.000Z'))&savedSearchId=571aaf70-4c88-11e8-b3d7-01146121b73d"
       )
     ).toEqual({

x-pack/plugins/ml/public/application/util/url_state.ts

@@ -5,7 +5,7 @@
  */
 
 import { parse, stringify } from 'query-string';
-import { useCallback } from 'react';
+import { useCallback, useEffect, useMemo, useState } from 'react';
 import { isEqual } from 'lodash';
 import { decode, encode } from 'rison-node';
 import { useHistory, useLocation } from 'react-router-dom';
@@ -30,7 +30,7 @@ function isRisonSerializationRequired(queryParam: string): boolean {
   return risonSerializedParams.has(queryParam);
 }
 
-export function getUrlState(search: string): Dictionary<any> {
+export function parseUrlState(search: string): Dictionary<any> {
   const urlState: Dictionary<any> = {};
   const parsedQueryString = parse(search, { sort: false });
 
@@ -58,54 +58,87 @@ export function getUrlState(search: string): Dictionary<any> {
 //   different urlStates(e.g. `_a` / `_g`) don't overwrite each other.
 export const useUrlState = (accessor: string): UrlState => {
   const history = useHistory();
-  const { search } = useLocation();
+  const { search: locationSearchString } = useLocation();
+
+  // We maintain a local state of useLocation's search.
+  // This allows us to use the callback variant of setSearch()
+  // later on so we can make sure we always act on the
+  // latest url state.
+  const [searchString, setSearchString] = useState(locationSearchString);
+
+  useEffect(() => {
+    setSearchString(locationSearchString);
+  }, [locationSearchString]);
+
+  useEffect(() => {
+    // Only push to history if something related to the accessor of this
+    // url state instance is affected (e.g. a change in '_g' should not trigger
+    // a push in the '_a' instance).
+    if (
+      !isEqual(parseUrlState(locationSearchString)[accessor], parseUrlState(searchString)[accessor])
+    ) {
+      history.push({ search: searchString });
+    }
+  }, [searchString]);
 
   const setUrlState = useCallback(
     (attribute: string | Dictionary<any>, value?: any) => {
-      const urlState = getUrlState(search);
-      const parsedQueryString = parse(search, { sort: false });
+      setSearchString((prevSearchString) => {
+        const urlState = parseUrlState(prevSearchString);
+        const parsedQueryString = parse(prevSearchString, { sort: false });
 
-      if (!Object.prototype.hasOwnProperty.call(urlState, accessor)) {
-        urlState[accessor] = {};
-      }
-
-      if (typeof attribute === 'string') {
-        if (isEqual(getNestedProperty(urlState, `${accessor}.${attribute}`), value)) {
-          return;
+        if (!Object.prototype.hasOwnProperty.call(urlState, accessor)) {
+          urlState[accessor] = {};
         }
 
-        urlState[accessor][attribute] = value;
-      } else {
-        const attributes = attribute;
-        Object.keys(attributes).forEach((a) => {
-          urlState[accessor][a] = attributes[a];
-        });
-      }
+        if (typeof attribute === 'string') {
+          if (isEqual(getNestedProperty(urlState, `${accessor}.${attribute}`), value)) {
+            return prevSearchString;
+          }
 
-      try {
-        const oldLocationSearch = stringify(parsedQueryString, { sort: false, encode: false });
+          urlState[accessor][attribute] = value;
+        } else {
+          const attributes = attribute;
+          Object.keys(attributes).forEach((a) => {
+            urlState[accessor][a] = attributes[a];
+          });
+        }
 
-        Object.keys(urlState).forEach((a) => {
-          if (isRisonSerializationRequired(a)) {
-            parsedQueryString[a] = encode(urlState[a]);
-          } else {
-            parsedQueryString[a] = urlState[a];
-          }
-        });
-        const newLocationSearch = stringify(parsedQueryString, { sort: false, encode: false });
+        try {
+          const oldLocationSearchString = stringify(parsedQueryString, {
+            sort: false,
+            encode: false,
+          });
 
-        if (oldLocationSearch !== newLocationSearch) {
-          history.push({
-            search: stringify(parsedQueryString, { sort: false }),
+          Object.keys(urlState).forEach((a) => {
+            if (isRisonSerializationRequired(a)) {
+              parsedQueryString[a] = encode(urlState[a]);
+            } else {
+              parsedQueryString[a] = urlState[a];
+            }
+          });
+          const newLocationSearchString = stringify(parsedQueryString, {
+            sort: false,
+            encode: false,
           });
+
+          if (oldLocationSearchString !== newLocationSearchString) {
+            return stringify(parsedQueryString, { sort: false });
+          }
+        } catch (error) {
+          // eslint-disable-next-line no-console
+          console.error('Could not save url state', error);
         }
-      } catch (error) {
-        // eslint-disable-next-line no-console
-        console.error('Could not save url state', error);
-      }
+
+        // as a fallback and to satisfy the hooks callback requirements
+        // return the previous state if we didn't need or were not able to update.
+        return prevSearchString;
+      });
     },
-    [search]
+    [searchString]
   );
 
-  return [getUrlState(search)[accessor], setUrlState];
+  const urlState = useMemo(() => parseUrlState(searchString)[accessor], [searchString]);
+
+  return [urlState, setUrlState];
 };