[vtadmin-web] Add replaceQuery + pushQuery to useURLQuery hook

web/vtadmin/src/components/dataTable/DataTable.tsx

@@ -13,12 +13,12 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import qs from 'query-string';
 import * as React from 'react';
 import { useLocation } from 'react-router-dom';
 
 import { useURLPagination } from '../../hooks/useURLPagination';
 import { useURLQuery } from '../../hooks/useURLQuery';
+import { stringify } from '../../util/queryString';
 import { PaginationNav } from './PaginationNav';
 
 interface Props<T> {
@@ -48,7 +48,7 @@ export const DataTable = <T extends object>({ columns, data, pageSize = DEFAULT_
 
     const formatPageLink = (p: number) => ({
         pathname,
-        search: qs.stringify({ ...urlQuery, page: p === 1 ? undefined : p }),
+        search: stringify({ ...urlQuery.query, page: p === 1 ? undefined : p }),
     });
 
     return (

web/vtadmin/src/hooks/useURLPagination.test.tsx

@@ -50,21 +50,21 @@ describe('useURLPagination', () => {
             url: '/test?page=100&foo=bar',
             opts: { totalPages: 10 },
             expected: { page: 1 },
-            redirectParams: { pathname: '/test', search: '?foo=bar&page=1' },
+            redirectParams: { search: '?foo=bar&page=1' },
         },
         {
             name: 'redirects to the first page if current page is a negative number',
             url: '/test?page=-123&foo=bar',
             opts: { totalPages: 10 },
             expected: { page: 1 },
-            redirectParams: { pathname: '/test', search: '?foo=bar&page=1' },
+            redirectParams: { search: '?foo=bar&page=1' },
         },
         {
             name: 'redirects to the first page if current page is not a number',
             url: '/test?page=abc&foo=bar',
             opts: { totalPages: 10 },
             expected: { page: 1 },
-            redirectParams: { pathname: '/test', search: '?foo=bar&page=1' },
+            redirectParams: { search: '?foo=bar&page=1' },
         },
         {
             name: 'does not redirect if totalPages is 0',

web/vtadmin/src/hooks/useURLPagination.ts

@@ -15,7 +15,6 @@
  */
 import { useEffect } from 'react';
 
-import qs from 'query-string';
 import { useHistory, useLocation } from 'react-router-dom';
 import { useURLQuery } from './useURLQuery';
 
@@ -39,25 +38,24 @@ const FIRST_PAGE = 1;
 export const useURLPagination = ({ totalPages }: PaginationOpts): PaginationParams => {
     const history = useHistory();
     const location = useLocation();
-    const query = useURLQuery();
+    const { query, replaceQuery } = useURLQuery();
 
     // A slight nuance here -- if `page` is not in the URL at all, then we can assume
     // it's the first page. This makes for slightly nicer URLs for the first/default page:
     // "/foo" instead of "/foo?page=1". No redirect required.
-    //
-    // However, if the value in the URL *is* defined but is negative, non-numeric,
-    // too big, or otherwise Weird, then we *do* want to redirect to the first page.
     const page = !('page' in query) || query.page === null ? FIRST_PAGE : query.page;
 
     useEffect(() => {
-        const isPageTooBig = totalPages > 0 && page > totalPages;
-        const isPageTooSmall = page < FIRST_PAGE;
+        // If the value in the URL *is* defined but is negative, non-numeric,
+        // too big, or otherwise Weird, then we *do* want to redirect to the first page.
+        const isPageTooBig = typeof page === 'number' && totalPages > 0 && page > totalPages;
+        const isPageTooSmall = typeof page === 'number' && page < FIRST_PAGE;
 
         if (isPageTooBig || isPageTooSmall || typeof page !== 'number') {
-            const nextQuery = qs.stringify({ ...query, page: FIRST_PAGE });
-            history.replace({ pathname: location.pathname, search: `?${nextQuery}` });
+            // Replace history so the invalid value is not persisted in browser history
+            replaceQuery({ page: FIRST_PAGE });
         }
-    }, [page, totalPages, history, location.pathname, query]);
+    }, [page, totalPages, history, location.pathname, query, replaceQuery]);
 
     return {
         page,

web/vtadmin/src/hooks/useURLQuery.test.tsx

@@ -13,51 +13,226 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+import { act } from '@testing-library/react';
 import { renderHook } from '@testing-library/react-hooks';
 import { createMemoryHistory } from 'history';
 import { Router } from 'react-router-dom';
+import { QueryParams } from '../util/queryString';
 
 import { useURLQuery } from './useURLQuery';
 
 describe('useURLQuery', () => {
-    const tests: {
-        name: string;
-        url: string;
-        expected: ReturnType<typeof useURLQuery>;
-    }[] = [
-        {
-            name: 'parses numbers',
-            url: '/test?page=1',
-            expected: { page: 1 },
-        },
-        {
-            name: 'parses booleans',
-            url: '/test?foo=true&bar=false',
-            expected: { foo: true, bar: false },
-        },
-        {
-            name: 'parses arrays by duplicate keys',
-            url: '/test?list=1&list=2&list=3',
-            expected: { list: [1, 2, 3] },
-        },
-        {
-            name: 'parses complex URLs',
-            url: '/test?page=1&isTrue=true&isFalse=false&list=one&list=two&list=three&foo=bar',
-            expected: { page: 1, isTrue: true, isFalse: false, list: ['one', 'two', 'three'], foo: 'bar' },
-        },
-    ];
-
-    test.each(tests.map(Object.values))('%s', (name: string, url: string, expected: ReturnType<typeof useURLQuery>) => {
-        const history = createMemoryHistory({
-            initialEntries: [url],
+    describe('parsing', () => {
+        const tests: {
+            name: string;
+            url: string;
+            expected: ReturnType<typeof useURLQuery>['query'];
+        }[] = [
+            {
+                name: 'parses numbers',
+                url: '/test?page=1',
+                expected: { page: 1 },
+            },
+            {
+                name: 'parses booleans',
+                url: '/test?foo=true&bar=false',
+                expected: { foo: true, bar: false },
+            },
+            {
+                name: 'parses arrays by duplicate keys',
+                url: '/test?list=1&list=2&list=3',
+                expected: { list: [1, 2, 3] },
+            },
+            {
+                name: 'parses complex URLs',
+                url: '/test?page=1&isTrue=true&isFalse=false&list=one&list=two&list=three&foo=bar',
+                expected: { page: 1, isTrue: true, isFalse: false, list: ['one', 'two', 'three'], foo: 'bar' },
+            },
+        ];
+
+        test.each(tests.map(Object.values))(
+            '%s',
+            (name: string, url: string, expected: ReturnType<typeof useURLQuery>) => {
+                const history = createMemoryHistory({
+                    initialEntries: [url],
+                });
+
+                const { result } = renderHook(() => useURLQuery(), {
+                    wrapper: ({ children }) => {
+                        return <Router history={history}>{children}</Router>;
+                    },
+                });
+
+                expect(result.current.query).toEqual(expected);
+            }
+        );
+    });
+
+    describe('pushQuery', () => {
+        const tests: {
+            name: string;
+            initialEntries: string[];
+            nextQuery: QueryParams;
+            expected: QueryParams;
+        }[] = [
+            {
+                name: 'stringifies and pushes query parameters onto history',
+                initialEntries: ['/test'],
+                nextQuery: { goodnight: 'moon' },
+                expected: { goodnight: 'moon' },
+            },
+            {
+                name: 'merges the next query with the current query',
+                initialEntries: ['/test?hello=world'],
+                nextQuery: { goodnight: 'moon' },
+                expected: { hello: 'world', goodnight: 'moon' },
+            },
+            {
+                name: 'it does not merge array-like queries',
+                initialEntries: ['/test?arr=one&arr=two'],
+                nextQuery: { arr: [3, 4, 5] },
+                expected: { arr: [3, 4, 5] },
+            },
+        ];
+
+        test.concurrent.each(tests.map(Object.values))(
+            '%s',
+            (name: string, initialEntries: string[], nextQuery: QueryParams, expected: QueryParams) => {
+                const history = createMemoryHistory({ initialEntries });
+                const initialPathname = history.location.pathname;
+
+                jest.spyOn(history, 'push');
+
+                const { result } = renderHook(() => useURLQuery(), {
+                    wrapper: ({ children }) => {
+                        return <Router history={history}>{children}</Router>;
+                    },
+                });
+
+                act(() => {
+                    result.current.pushQuery(nextQuery);
+                });
+
+                expect(history.push).toHaveBeenCalledTimes(1);
+                expect(result.current.query).toEqual(expected);
+                expect(history.location.pathname).toEqual(initialPathname);
+            }
+        );
+    });
+
+    describe('replaceQuery', () => {
+        const tests: {
+            name: string;
+            initialEntries: string[];
+            nextQuery: QueryParams;
+            expected: QueryParams;
+        }[] = [
+            {
+                name: 'stringifies and replaces query parameters onto history',
+                initialEntries: ['/test'],
+                nextQuery: { goodnight: 'moon' },
+                expected: { goodnight: 'moon' },
+            },
+            {
+                name: 'merges the next query with the current query',
+                initialEntries: ['/test?hello=world'],
+                nextQuery: { goodnight: 'moon' },
+                expected: { hello: 'world', goodnight: 'moon' },
+            },
+            {
+                name: 'it does not merge array-like queries',
+                initialEntries: ['/test?arr=one&arr=two'],
+                nextQuery: { arr: [3, 4, 5] },
+                expected: { arr: [3, 4, 5] },
+            },
+        ];
+
+        test.concurrent.each(tests.map(Object.values))(
+            '%s',
+            (name: string, initialEntries: string[], nextQuery: QueryParams, expected: QueryParams) => {
+                const history = createMemoryHistory({ initialEntries });
+                const initialPathname = history.location.pathname;
+
+                jest.spyOn(history, 'replace');
+
+                const { result } = renderHook(() => useURLQuery(), {
+                    wrapper: ({ children }) => {
+                        return <Router history={history}>{children}</Router>;
+                    },
+                });
+
+                act(() => {
+                    result.current.replaceQuery(nextQuery);
+                });
+
+                expect(history.replace).toHaveBeenCalledTimes(1);
+                expect(result.current.query).toEqual(expected);
+                expect(history.location.pathname).toEqual(initialPathname);
+            }
+        );
+    });
+
+    it('uses parsing/formatting options when specified', () => {
+        const history = createMemoryHistory({ initialEntries: ['/test?foo=true&count=123'] });
+        const { result } = renderHook(
+            () =>
+                useURLQuery({
+                    parseBooleans: false,
+                    parseNumbers: false,
+                }),
+            {
+                wrapper: ({ children }) => {
+                    return <Router history={history}>{children}</Router>;
+                },
+            }
+        );
+
+        expect(result.current.query).toEqual({ foo: 'true', count: '123' });
+
+        act(() => {
+            result.current.pushQuery({ foo: false, count: 456 });
+        });
+
+        expect(result.current.query).toEqual({ foo: 'false', count: '456' });
+
+        act(() => {
+            result.current.pushQuery({ foo: true, count: 789 });
         });
 
+        expect(result.current.query).toEqual({ foo: 'true', count: '789' });
+    });
+
+    it('memoizes the query object by search string', () => {
+        const history = createMemoryHistory({ initialEntries: ['/test?hello=world'] });
+
+        jest.spyOn(history, 'push');
+
         const { result } = renderHook(() => useURLQuery(), {
             wrapper: ({ children }) => {
                 return <Router history={history}>{children}</Router>;
             },
         });
 
-        expect(result.current).toEqual(expected);
+        const firstResult = result.current.query;
+        expect(firstResult).toEqual({ hello: 'world' });
+
+        act(() => {
+            result.current.pushQuery({ hello: 'world' });
+        });
+
+        // Make sure the returned object is memoized when the search string
+        // is updated but the value doesn't change.
+        expect(history.push).toHaveBeenCalledTimes(1);
+        expect(result.current.query).toEqual({ hello: 'world' });
+        expect(result.current.query).toBe(firstResult);
+
+        // Make sure the returned query actually changes when the search string changes,
+        // and that we're not memoizing too aggressively.
+        act(() => {
+            result.current.pushQuery({ hello: 'moon' });
+        });
+
+        expect(history.push).toHaveBeenCalledTimes(2);
+        expect(result.current.query).toEqual({ hello: 'moon' });
     });
 });