diff --git a/.changeset/fix-immediate-request-state.md b/.changeset/fix-immediate-request-state.md new file mode 100644 index 0000000..a77d609 --- /dev/null +++ b/.changeset/fix-immediate-request-state.md @@ -0,0 +1,10 @@ +--- +'@fingerprint/react': minor +--- + +Behavior bug fixes: + +- Stale automatic `useVisitorData` requests (when `immediate` changes from `true` to `false` during an automatic request) no longer overwrite state (the result is ignored). +- Loading state is now correctly synchronized when `immediate` changes. + +We recommend double-checking that your implementation isn't relying on the previous incorrect behavior when upgrading. diff --git a/README.md b/README.md index 16174c2..f5267eb 100644 --- a/README.md +++ b/README.md @@ -129,7 +129,6 @@ export default App The `useVisitorData` hook also returns a `getData` method you can use to make an API call on command. -- You can pass `{ immediate: false }` to `useVisitorData` to disable automatic visitor identification on render. Both `useVisitorData` and `getData` accept all the [get options](https://docs.fingerprint.com/reference/js-agent-get-function#get-options) available in the JavaScript agent `get` function. @@ -184,6 +183,8 @@ function App() { export default App ``` +- You can pass `{ immediate: false }` to disable automatic identification and call `getData` manually instead. +- By default (`{ immediate: true }`), the hook automatically identifies the visitor after mounting and whenever its request options change. Changing `immediate` from `false` to `true` also starts an identification request with the latest options. - See the full code example in the [examples folder](./examples/). - See our [Use cases](https://demo.fingerprint.com) page for [open-source](https://github.com/fingerprintjs/fingerprintjs-pro-use-cases) real-world examples of using Fingerprint to detect fraud and streamline user experiences. diff --git a/__tests__/helpers.tsx b/__tests__/helpers.tsx index 94faa0f..437ed5b 100644 --- a/__tests__/helpers.tsx +++ b/__tests__/helpers.tsx @@ -1,6 +1,5 @@ import { PropsWithChildren } from 'react' import { FingerprintProvider, FingerprintProviderOptions } from '../src' -import { act } from '@testing-library/react' export const getDefaultLoadOptions = () => ({ apiKey: 'test_api_key', @@ -18,9 +17,3 @@ export const wait = (ms: number) => new Promise((resolve) => { setTimeout(resolve, ms) }) - -export const actWait = async (ms: number) => { - await act(async () => { - await wait(ms) - }) -} diff --git a/__tests__/use-visitor-data.test.tsx b/__tests__/use-visitor-data.test.tsx index cff147c..b5afa45 100644 --- a/__tests__/use-visitor-data.test.tsx +++ b/__tests__/use-visitor-data.test.tsx @@ -1,6 +1,6 @@ import { useVisitorData, UseVisitorDataReturn } from '../src' -import { act, render, renderHook, screen } from '@testing-library/react' -import { actWait, createWrapper, wait } from './helpers' +import { act, render, renderHook, screen, waitFor } from '@testing-library/react' +import { createWrapper, wait } from './helpers' import { useEffect, useState } from 'react' import userEvent from '@testing-library/user-event' import * as agent from '@fingerprint/agent' @@ -56,16 +56,16 @@ describe('useVisitorData', () => { }) ) - await actWait(500) - - expect(mockStart).toHaveBeenCalled() - expect(mockGet).toHaveBeenCalled() - expect(result.current).toMatchObject( - expect.objectContaining({ - isLoading: false, - data: mockGetResult, - }) - ) + await waitFor(() => { + expect(mockStart).toHaveBeenCalled() + expect(mockGet).toHaveBeenCalled() + expect(result.current).toMatchObject( + expect.objectContaining({ + isLoading: false, + data: mockGetResult, + }) + ) + }) }) it('should avoid duplicate requests if one is already pending', async () => { @@ -83,9 +83,9 @@ describe('useVisitorData', () => { }) ) - await Promise.all([result.current.getData(), result.current.getData()]) - - await actWait(500) + await act(async () => { + await Promise.all([result.current.getData(), result.current.getData()]) + }) expect(mockStart).toHaveBeenCalled() expect(mockGet).toHaveBeenCalledTimes(1) @@ -128,13 +128,92 @@ describe('useVisitorData', () => { expect(mockGet).not.toHaveBeenCalled() }) + it('should fetch and enter loading when immediate changes from false to true', async () => { + let resolveRequest!: (value: GetResult) => void + mockGet.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRequest = resolve + }) + ) + + const wrapper = createWrapper() + const { result, rerender } = renderHook( + ({ immediate }: { immediate: boolean }) => useVisitorData({ immediate, tag: 1 }), + { + wrapper, + initialProps: { immediate: false }, + } + ) + + // Automatic fetching stays idle while immediate is disabled. + expect(result.current.isLoading).toBe(false) + expect(mockGet).not.toHaveBeenCalled() + + // Enabling immediate after mount starts a request and exposes loading right away. + rerender({ immediate: true }) + + expect(result.current.isLoading).toBe(true) + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(1) + expect(mockGet).toHaveBeenCalledWith({ tag: 1 }) + }) + + // The automatically started request updates the hook like an initial immediate fetch. + act(() => { + resolveRequest(mockGetResult) + }) + + await waitFor(() => { + expect(result.current).toMatchObject({ + isLoading: false, + isFetched: true, + data: mockGetResult, + }) + }) + }) + + it('should leave loading when immediate is disabled during an automatic request', async () => { + let resolveRequest!: (value: GetResult) => void + const request = new Promise((resolve) => { + resolveRequest = resolve + }) + mockGet.mockReturnValueOnce(request) + + const wrapper = createWrapper() + const { result, rerender } = renderHook(({ immediate }: { immediate: boolean }) => useVisitorData({ immediate }), { + wrapper, + initialProps: { immediate: true }, + }) + + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(1) + }) + expect(result.current.isLoading).toBe(true) + + // Disabling automatic fetching makes the active response irrelevant. + rerender({ immediate: false }) + expect(result.current.isLoading).toBe(false) + + await act(async () => { + resolveRequest(mockGetResult) + await request + }) + + expect(result.current).toMatchObject({ + isLoading: false, + isFetched: false, + data: undefined, + }) + }) + it('should support immediate fetch with cache disabled', async () => { const wrapper = createWrapper() renderHook(() => useVisitorData({ immediate: true }), { wrapper }) - await actWait(500) - - expect(mockGet).toHaveBeenCalledTimes(1) + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(1) + }) expect(mockGet).toHaveBeenCalledWith({}) }) @@ -180,17 +259,100 @@ describe('useVisitorData', () => { ) - await actWait(1000) + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(1) + }) await user.click(screen.getByRole('button', { name: 'Change options' })) - await actWait(1000) - - expect(mockGet).toHaveBeenCalledTimes(2) + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(2) + }) expect(mockGet).toHaveBeenNthCalledWith(1, { tag: 1 }) expect(mockGet).toHaveBeenNthCalledWith(2, { tag: 2 }) }) + it('should set error state when immediate mount fetch fails', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + mockGet.mockRejectedValue('mount failed') + + const wrapper = createWrapper() + const { result } = renderHook(() => useVisitorData({ immediate: true }), { wrapper }) + + await waitFor(() => { + expect(result.current).toMatchObject({ + isLoading: false, + isFetched: false, + data: undefined, + error: expect.objectContaining({ message: 'mount failed' }), + }) + }) + expect(consoleError).toHaveBeenCalledWith('Failed to fetch visitor data automatically: Error: mount failed') + + consoleError.mockRestore() + }) + + it('should set error state when an immediate fetch throws synchronously', async () => { + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + + const { result } = renderHook(() => useVisitorData({ immediate: true })) + + await waitFor(() => { + expect(result.current).toMatchObject({ + isLoading: false, + isFetched: false, + data: undefined, + error: expect.objectContaining({ message: 'You forgot to wrap your component in .' }), + }) + }) + expect(consoleError).toHaveBeenCalledWith( + 'Failed to fetch visitor data automatically: Error: You forgot to wrap your component in .' + ) + + consoleError.mockRestore() + }) + + it('should not apply an outdated immediate response after getOptions change mid-flight', async () => { + // First agent call stays pending until we resolve it later. + let resolveFirst!: (value: GetResult) => void + const firstRequest = new Promise((resolve) => { + resolveFirst = resolve + }) + const secondResult = { ...mockGetResult, visitor_id: 'second-visitor' } + + mockGet.mockImplementationOnce(() => firstRequest).mockResolvedValueOnce(secondResult) + + const wrapper = createWrapper() + const { result, rerender } = renderHook(({ tag }: { tag: number }) => useVisitorData({ immediate: true, tag }), { + wrapper, + initialProps: { tag: 1 }, + }) + + // Mount started one in-flight request for tag: 1. + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(1) + }) + expect(result.current.isLoading).toBe(true) + + // Changing options cancels the previous effect and starts a new immediate fetch. + rerender({ tag: 2 }) + expect(result.current.isLoading).toBe(true) + + await waitFor(() => { + expect(mockGet).toHaveBeenCalledTimes(2) + expect(result.current.data).toEqual(secondResult) + }) + + // The slow first response arrives after the second request has already settled. + // Without ignore/cleanup, this would overwrite data with the stale tag: 1 result. + await act(async () => { + resolveFirst(mockGetResult) + await firstRequest + }) + + expect(result.current.data).toEqual(secondResult) + }) + it('should correctly pass errors from agent', async () => { const ERROR_CLIENT_TIMEOUT = 'timeout' mockGet.mockRejectedValue(new Error(ERROR_CLIENT_TIMEOUT)) diff --git a/src/use-visitor-data.ts b/src/use-visitor-data.ts index 67a1b62..77f67aa 100644 --- a/src/use-visitor-data.ts +++ b/src/use-visitor-data.ts @@ -7,7 +7,8 @@ import { GetOptions, GetResult } from '@fingerprint/agent' export interface UseVisitorDataConfig { /** - * Determines whether the `getData()` method will be called immediately after the hook mounts or not + * Controls automatic visitor data fetching. When `true`, the hook fetches after mounting, and whenever the + * request options change. When changed from `false` to `true`, visitor data is initiated after the current render. */ immediate: boolean } @@ -45,6 +46,7 @@ export function useVisitorData( const { getVisitorData } = useContext(FingerprintContext) const [currentGetOptions, setCurrentGetOptions] = useState(getOptions) + const [currentImmediate, setCurrentImmediate] = useState(immediate) const [queryState, setQueryState] = useState({ isLoading: immediate, data: undefined, @@ -52,17 +54,41 @@ export function useVisitorData( error: undefined, }) + const setLoading = () => { + setQueryState({ + isLoading: true, + isFetched: false, + data: undefined, + error: undefined, + }) + } + + const setSuccess = (data: GetResult) => { + setQueryState({ + isLoading: false, + isFetched: true, + data, + error: undefined, + }) + } + + const setFailure = (unknownError: unknown) => { + const error = toError(unknownError) + setQueryState({ + isLoading: false, + isFetched: false, + data: undefined, + error, + }) + return error + } + const getData = useCallback( async (params = {}) => { assertIsDefined(params, 'getDataParams') try { - setQueryState({ - isLoading: true, - isFetched: false, - data: undefined, - error: undefined, - }) + setLoading() const getDataOptions: GetOptions = { ...currentGetOptions, @@ -70,44 +96,74 @@ export function useVisitorData( } const result = await getVisitorData(getDataOptions) - setQueryState({ - isLoading: false, - isFetched: true, - data: result, - error: undefined, - }) + setSuccess(result) return result } catch (unknownError) { - const error = toError(unknownError) - - setQueryState({ - isLoading: false, - isFetched: false, - data: undefined, - error, - }) - - throw error + throw setFailure(unknownError) } }, [currentGetOptions, getVisitorData] ) + /** + * When `immediate` is enabled, fetches visitor data on mount and whenever `getOptions` change. + * We don't reuse `getData` here because it sets loading state synchronously, which is not allowed + * inside an effect — `isLoading: true` is covered by the initial state and the render-phase reset below. + * The `ignore` flag prevents an outdated in-flight response from overwriting a newer request's state: + * https://react.dev/reference/react/useEffect#fetching-data-with-effects + */ useEffect(() => { - if (immediate) { - // TODO: refactor mount fetch to avoid synchronous setState inside the effect - // eslint-disable-next-line react-hooks/set-state-in-effect -- preserve existing getData() mount behavior - getData().catch((unknownError: unknown) => { - console.error(`Failed to fetch visitor data on mount: ${String(unknownError)}`) - }) + if (!currentImmediate) { + return + } + + let ignore = false + + const fetchVisitorData = async () => { + try { + const result = await getVisitorData(currentGetOptions) + if (!ignore) { + setSuccess(result) + } + } catch (unknownError: unknown) { + if (ignore) { + return + } + + const error = setFailure(unknownError) + console.error(`Failed to fetch visitor data automatically: ${error}`) + } } - }, [immediate, getData]) - if (!Object.is(currentGetOptions, getOptions) && !areGetOptionsEqual(currentGetOptions, getOptions)) { + void fetchVisitorData() + + return () => { + ignore = true + } + }, [currentImmediate, getVisitorData, currentGetOptions]) + + // When automatic-fetch inputs change, store them and reset to loading during render so the effect can start + // the request without synchronously setting state: https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes + const didImmediateChange = currentImmediate !== immediate + const didGetOptionsChange = + !Object.is(currentGetOptions, getOptions) && !areGetOptionsEqual(currentGetOptions, getOptions) + + if (didImmediateChange) { + setCurrentImmediate(immediate) + } + + if (didGetOptionsChange) { setCurrentGetOptions(getOptions) } + if (immediate && (didImmediateChange || didGetOptionsChange)) { + setLoading() + } else if (didImmediateChange) { + // Disabling automatic fetching clears loading while the pending response is ignored. + setQueryState((state) => ({ ...state, isLoading: false })) + } + return useMemo( () => ({ ...queryState,