diff --git a/web/vtadmin/src/components/dataTable/DataTable.tsx b/web/vtadmin/src/components/dataTable/DataTable.tsx index 3cca7131052..51e19f1b917 100644 --- a/web/vtadmin/src/components/dataTable/DataTable.tsx +++ b/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 { @@ -48,7 +48,7 @@ export const DataTable = ({ 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 ( diff --git a/web/vtadmin/src/hooks/useURLPagination.test.tsx b/web/vtadmin/src/hooks/useURLPagination.test.tsx index a6dd87e85a7..f50ac4ba4cb 100644 --- a/web/vtadmin/src/hooks/useURLPagination.test.tsx +++ b/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', diff --git a/web/vtadmin/src/hooks/useURLPagination.ts b/web/vtadmin/src/hooks/useURLPagination.ts index bfa5f63668e..dc20c55d87b 100644 --- a/web/vtadmin/src/hooks/useURLPagination.ts +++ b/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, diff --git a/web/vtadmin/src/hooks/useURLQuery.test.tsx b/web/vtadmin/src/hooks/useURLQuery.test.tsx index e1420fe8b48..d0b47a5c3ef 100644 --- a/web/vtadmin/src/hooks/useURLQuery.test.tsx +++ b/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; - }[] = [ - { - 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) => { - const history = createMemoryHistory({ - initialEntries: [url], + describe('parsing', () => { + const tests: { + name: string; + url: string; + expected: ReturnType['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) => { + const history = createMemoryHistory({ + initialEntries: [url], + }); + + const { result } = renderHook(() => useURLQuery(), { + wrapper: ({ children }) => { + return {children}; + }, + }); + + 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 {children}; + }, + }); + + 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 {children}; + }, + }); + + 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 {children}; + }, + } + ); + + 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 {children}; }, }); - 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' }); }); }); diff --git a/web/vtadmin/src/hooks/useURLQuery.ts b/web/vtadmin/src/hooks/useURLQuery.ts index 2baa8ec9dc1..e87a7f9df86 100644 --- a/web/vtadmin/src/hooks/useURLQuery.ts +++ b/web/vtadmin/src/hooks/useURLQuery.ts @@ -13,26 +13,106 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import qs from 'query-string'; -import { useLocation } from 'react-router-dom'; +import { useCallback, useMemo } from 'react'; +import { useHistory, useLocation } from 'react-router-dom'; +import { ArrayFormatType, parse, QueryParams, stringify } from '../util/queryString'; + +export interface URLQueryOptions { + arrayFormat?: ArrayFormatType; + parseBooleans?: boolean; + parseNumbers?: boolean; +} /** - * useURLQuery is a hook for parsing query parameters from the current URL - * into a map, where "query parameters" are those appearing after the "?". + * useURLQuery is a hook for getting and setting query parameters from the current URL, + * where "query parameters" are those appearing after the "?": + * + * https://test.com/some/route?foo=bar&count=123&list=one&list=two&list=3 + * ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + * + * The query parameters from the above URL would be parsed as: * - * So, given a URL like: https://test.com/some/route?foo=bar&count=123 - * ^^^^^^^^^^^^^^^^^ - * ... useURLQuery() would return `{ foo: "bar", count: 123 }` + * { foo: "bar", count: 123, list: ["one", "two", "three"] } + * + * For lots more usage examples, see the useURLQuery unit tests. */ -export const useURLQuery = () => { - const { search } = useLocation(); - - // For full options, see: https://github.com/sindresorhus/query-string - return qs.parse(search, { - // Parse arrays with elements using duplicate keys - // 'foo=1&foo=2&foo=3' => { foo: [1, 2, 3] } - arrayFormat: 'none', - parseBooleans: true, - parseNumbers: true, - }); +export const useURLQuery = ( + opts: URLQueryOptions = {} +): { + /** + * The current URL query parameters, parsed into an object. + */ + query: QueryParams; + + /** + * `pushQuery` merges `nextQuery` with the current query parameters + * and pushes the resulting search string onto the history stack. + * + * This does not affect location.pathname: if your current path + * is "/test?greeting=hello", then calling `pushQuery({ greeting: "hi" })` + * will push "/test?greeting=hi". If you *do* want to update the pathname, + * then use useHistory()'s history.push directly. + */ + pushQuery: (nextQuery: QueryParams) => void; + + /** + * `replaceQuery` merges `nextQuery` with the current query parameters + * and replaces the resulting search string onto the history stack. + * + * This does not affect location.pathname: if your current path + * is "/test?greeting=hello", then calling `replaceQuery({ greeting: "hi" })` + * will replace "/test?greeting=hi". If you *do* want to update the pathname, + * then use useHistory()'s history.replace directly. + */ + replaceQuery: (nextQuery: QueryParams) => void; +} => { + const history = useHistory(); + const location = useLocation(); + + // A spicy note: typically, we always want to use the `location` from useLocation() instead of useHistory(). + // From the React documentation: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/api/history.md#history-is-mutable + // + // The history object is mutable. Therefore it is recommended to access the location from the render props of , + // not from history.location. This ensures your assumptions about React are correct in lifecycle hooks. + // + // However, in a *test* environment, the "?...string" one usually finds at `location.search` + // is (confusingly) nested at `location.location.search`. This seems like a discrepancy between how + // `history.push` + `history.replace` calls are handled by `Router` + memory history (used for tests) + // vs. `BrowserRouter` (used "for real", in the browser). + // + // So, in practice, this `search` variable is set to `location.search` "for real" (in the browser) + // and only falls back to `location.location.search` for tests. It's... not ideal. :/ But it seems to work. + const search = location.search || history.location.search; + + // Destructure `opts` for more granular useMemo and useCallback dependencies. + const { arrayFormat, parseBooleans, parseNumbers } = opts; + + // Parse the URL search string into a mapping from URL parameter key to value. + const query = useMemo( + () => + parse(search, { + arrayFormat, + parseBooleans, + parseNumbers, + }), + [search, arrayFormat, parseBooleans, parseNumbers] + ); + + const pushQuery = useCallback( + (nextQuery: QueryParams) => { + const nextSearch = stringify({ ...query, ...nextQuery }, { arrayFormat }); + return history.push({ search: `?${nextSearch}` }); + }, + [arrayFormat, history, query] + ); + + const replaceQuery = useCallback( + (nextQuery: QueryParams) => { + const nextSearch = stringify({ ...query, ...nextQuery }, { arrayFormat }); + return history.replace({ search: `?${nextSearch}` }); + }, + [arrayFormat, history, query] + ); + + return { query, pushQuery, replaceQuery }; }; diff --git a/web/vtadmin/src/util/queryString.ts b/web/vtadmin/src/util/queryString.ts new file mode 100644 index 00000000000..2aad2945ec1 --- /dev/null +++ b/web/vtadmin/src/util/queryString.ts @@ -0,0 +1,52 @@ +/** + * Copyright 2021 The Vitess Authors. + * + * Licensed 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 { merge } from 'lodash-es'; +import qs from 'query-string'; + +export type QueryParams = ParsedQuery; + +export interface ParsedQuery { + [key: string]: T | T[] | null | undefined; +} + +// See https://github.com/sindresorhus/query-string#arrayformat +export type ArrayFormatType = 'bracket' | 'index' | 'comma' | 'separator' | 'none'; + +const DEFAULT_ARRAY_FORMAT = 'none'; + +const DEFAULT_PARSE_OPTIONS: qs.ParseOptions = { + arrayFormat: DEFAULT_ARRAY_FORMAT, + parseBooleans: true, + parseNumbers: true, +}; + +/** + * `parse` is a simple wrapper around query-string's `parse` function, specifying + * VTAdmin's query parsing defaults. See https://github.com/sindresorhus/query-string. + */ +export const parse = (search: string, opts: qs.ParseOptions = {}) => + qs.parse(search, merge({}, DEFAULT_PARSE_OPTIONS, opts)); + +const DEFAULT_STRINGIFY_OPTIONS: qs.StringifyOptions = { + arrayFormat: DEFAULT_ARRAY_FORMAT, +}; + +/** + * `stringify` is a simple wrapper around query-string's `stringify` function, specifying + * VTAdmin's query stringification defaults. See https://github.com/sindresorhus/query-string. + */ +export const stringify = (query: ParsedQuery, opts: qs.StringifyOptions = {}) => + qs.stringify(query, merge({}, DEFAULT_STRINGIFY_OPTIONS, opts));