fix: race condition with pagination token (#105)

* fix: race condition with pagination token

* test: adding tests for usePagination hook

src/components/hooks/test/usePagination.test.tsx

@@ -0,0 +1,150 @@
+import {
+    fireEvent,
+    getByLabelText,
+    getByText,
+    render,
+    waitFor
+} from '@testing-library/react';
+import {
+    PaginatedEntityResponse,
+    RequestConfig
+} from 'models/AdminEntity/types';
+import * as React from 'react';
+import { PaginationConfig, usePagination } from '../usePagination';
+
+const valueLabel = 'pagination-value';
+const fetchLabel = 'pagination-doFetch';
+const moreItemsAvailableLabel = 'pagination-moreItemsAvailable';
+
+interface PaginationItem {
+    id: string;
+}
+
+type FetchResponse = PaginatedEntityResponse<PaginationItem>;
+interface PaginationTesterProps {
+    config: PaginationConfig<{}>;
+    doFetch: jest.Mock<Promise<FetchResponse>>;
+}
+
+const PaginationTester = ({ config, doFetch }: PaginationTesterProps) => {
+    const fetchable = usePagination(config, doFetch);
+    const onClickFetch = () => fetchable.fetch();
+
+    return (
+        <div>
+            <div aria-label={valueLabel}>
+                <ul>
+                    {fetchable.value.map(({ id }) => (
+                        <li key={`item-${id}`}>{`item-${id}`}</li>
+                    ))}
+                </ul>
+            </div>
+            <div aria-label={moreItemsAvailableLabel}>
+                {fetchable.moreItemsAvailable ? 'true' : 'false'}
+            </div>
+            <button aria-label={fetchLabel} onClick={onClickFetch}>
+                Fetch Data
+            </button>
+        </div>
+    );
+};
+
+describe('usePagination', () => {
+    let entityCounter: number;
+    let config: PaginationConfig<{}>;
+    let doFetch: jest.Mock<Promise<FetchResponse>>;
+
+    beforeEach(() => {
+        entityCounter = 0;
+        doFetch = jest
+            .fn()
+            .mockImplementation(
+                (fetchArg: any, { limit = 25 }: RequestConfig) =>
+                    Promise.resolve({
+                        entities: Array.from({ length: limit }, () => {
+                            const id = `${entityCounter}`;
+                            entityCounter += 1;
+                            return { id };
+                        }),
+                        token: `${entityCounter}`
+                    })
+            );
+        config = {
+            cacheItems: false,
+            fetchArg: {},
+            limit: 25
+        };
+    });
+
+    const renderTester = () =>
+        render(<PaginationTester config={config} doFetch={doFetch} />);
+    const getElements = async (container: HTMLElement) => {
+        return waitFor(() => {
+            return {
+                fetchButton: getByLabelText(container, fetchLabel),
+                moreItemsAvailable: getByLabelText(
+                    container,
+                    moreItemsAvailableLabel
+                )
+            };
+        });
+    };
+
+    const waitForLastItemRendered = async (container: HTMLElement) => {
+        return waitFor(() => getByText(container, `item-${entityCounter - 1}`));
+    };
+
+    it('should pass returned token in subsequent calls', async () => {
+        const { container } = renderTester();
+        const { fetchButton } = await getElements(container);
+
+        expect(doFetch).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ token: '' })
+        );
+
+        fireEvent.click(fetchButton);
+        await waitForLastItemRendered(container);
+        expect(doFetch).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ token: `${config.limit}` })
+        );
+    });
+
+    it('should reset token when config changes', async () => {
+        const { container } = renderTester();
+        await getElements(container);
+
+        expect(doFetch).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ token: '' })
+        );
+
+        doFetch.mockClear();
+        entityCounter = 0;
+
+        // Change the config to trigger a rest of the pagination hook
+        config.limit = 10;
+        await getElements(renderTester().container);
+
+        expect(doFetch).toHaveBeenCalledWith(
+            expect.anything(),
+            expect.objectContaining({ token: '' })
+        );
+    });
+
+    it('should set moreItemsAvailable if token is returned', async () => {
+        const { moreItemsAvailable } = await getElements(
+            renderTester().container
+        );
+        expect(moreItemsAvailable.textContent).toBe('true');
+    });
+
+    it('should not set moreItemsAvailable if no token is returned', async () => {
+        doFetch.mockResolvedValue({ entities: [{ id: '0' }] });
+        const { moreItemsAvailable } = await getElements(
+            renderTester().container
+        );
+        expect(moreItemsAvailable.textContent).toBe('false');
+    });
+});

src/components/hooks/types.ts

@@ -56,7 +56,7 @@ export interface FetchableData<T> {
     debugName: string;
     fetch(): void;
     lastError: Error | null;
-    state: FetchableState<T>;
+    state: FetchableState<any>;
     value: T;
 }
 
@@ -65,6 +65,10 @@ export interface FetchableExecution {
     terminateExecution(cause: string): Promise<void>;
 }
 
+export interface PaginationValue<T> {
+    token?: string;
+    items: T[];
+}
 export interface PaginatedFetchableData<T> extends FetchableData<T[]> {
     /** Whether or not a fetch would yield more items. Useful for determining if
      * a "load more" button should be shown

src/components/hooks/usePagination.ts

@@ -1,9 +1,12 @@
-import { useContext, useEffect, useState } from 'react';
-
-import { CacheContext, getCacheKey } from 'components/Cache';
+import { CacheContext } from 'components/Cache';
 import { RequestConfig } from 'models';
-
-import { FetchFn, PaginatedFetchableData, PaginatedFetchFn } from './types';
+import { useContext, useMemo } from 'react';
+import {
+    FetchFn,
+    PaginatedFetchableData,
+    PaginatedFetchFn,
+    PaginationValue
+} from './types';
 import { useFetchableData } from './useFetchableData';
 
 export interface PaginationConfig<FetchArgType> extends RequestConfig {
@@ -33,52 +36,46 @@ export function usePagination<T, FetchArgType>(
     doFetch: PaginatedFetchFn<T, FetchArgType>
 ): PaginatedFetchableData<T> {
     const { cacheItems = false, debugName } = config;
-    const cacheKey = getCacheKey(config);
-    const [token, setToken] = useState('');
-    const [moreItemsAvailable, setMoreItemsAvailable] = useState(false);
     const cache = useContext(CacheContext);
 
-    // Reset our state if the pagination config changes
-    useEffect(() => {
-        setToken('');
-        setMoreItemsAvailable(false);
-    }, [cacheKey]);
+    const fetch: FetchFn<
+        PaginationValue<T>,
+        PaginationConfig<FetchArgType>
+    > = useMemo(
+        () => async (params, currentValue) => {
+            const { token: previousToken = '', items: previousItems = [] } =
+                currentValue || {};
+            const { fetchArg, ...requestConfig } = params;
 
-    const fetch: FetchFn<T[], PaginationConfig<FetchArgType>> = async (
-        params,
-        currentValue = []
-    ) => {
-        const { fetchArg, ...requestConfig } = params;
+            // If our last fetch call returned a token,
+            // we have to pass that along in order to retrieve the next page
+            const finalConfig = { ...requestConfig, token: previousToken };
 
-        // If our last fetch call returned a token,
-        // we have to pass that along in order to retrieve the next page
-        if (token) {
-            requestConfig.token = token;
-        }
+            const { entities, token } = await doFetch(fetchArg, finalConfig);
+            const result = cacheItems ? cache.mergeArray(entities) : entities;
 
-        const { entities, token: newToken } = await doFetch(
-            fetchArg,
-            requestConfig
-        );
-        const values = cacheItems ? cache.mergeArray(entities) : entities;
+            const items = previousItems.concat(result);
+            return {
+                items,
+                token
+            };
+        },
+        [cache, cacheItems]
+    );
 
-        if (newToken) {
-            setToken(newToken);
-        }
-        const newValue = currentValue.concat(values);
-        setMoreItemsAvailable(!!newToken);
-        return newValue;
-    };
+    const fetchable = useFetchableData(
+        {
+            debugName,
+            defaultValue: { token: '', items: [] },
+            doFetch: fetch
+        },
+        config
+    );
 
+    const { items: value, token } = fetchable.value;
     return {
-        ...useFetchableData(
-            {
-                debugName,
-                defaultValue: [],
-                doFetch: fetch
-            },
-            config
-        ),
-        moreItemsAvailable
+        ...fetchable,
+        value,
+        moreItemsAvailable: !!token
     };
 }