diff --git a/.changeset/client-react-dependency-identity-loops.md b/.changeset/client-react-dependency-identity-loops.md new file mode 100644 index 0000000000..6589c5a853 --- /dev/null +++ b/.changeset/client-react-dependency-identity-loops.md @@ -0,0 +1,44 @@ +--- +"@objectstack/client-react": patch +--- + +fix(client-react): stop five hooks from looping on dependency identity (#4693, #4694) + +Five hooks keyed a `useCallback`/`useEffect` on values the caller supplies +inline — `where` / `fields` / `orderBy` objects, `onSuccess` / `onError` +handlers, and the `fetcher` `useMetadata` takes as a required positional +argument. Inline means a fresh identity on every render, so the effect re-ran on +every render; because the fetch hooks call `setState`, that render caused +another. The result was an unbounded request loop under the hooks' own +documented usage. + +Requests issued in 250ms by a single mounted component, measured before and +after: + +| hook | before | after | +|-----------------------------------|-------:|------:| +| `useQuery` (inline `where`) | 4691 | 1 | +| `useInfiniteQuery` (inline `where`) | 6611 | 1 | +| `useObject` (no options at all) | 4306 | 1 | +| `useView` (inline `onSuccess`) | 8197 | 1 | +| `useMetadata` (inline `fetcher`) | 7654 | 1 | + +`useObject` and `useMetadata` needed no particular usage to loop: the former +depended on its own `data` and `etag` state while writing both, and the latter +takes its fetcher positionally, so there is no non-inline way to call it. +`useMutation` was never affected — no effect drives it. + +The same root cause churned the realtime subscriptions (#4694): +`useAutoRefresh` with an unmemoized `refetch` — which is what `useQuery` +returned on every render — resubscribed on both streams every render, losing any +event delivered in the unsubscribe/resubscribe gap. + +Two internal primitives fix both halves: `stableKey` derives a dependency from a +structural value (sorted keys, array order preserved) so a rebuilt-but-equal +object is a no-op, and `useEventCallback` gives a handler a fixed identity while +always invoking its latest version. Neither is exported. + +A changed *value* still refetches, and every stabilized handler is asserted to +run its newest version rather than the one captured when the effect first ran — +the ref indirection would otherwise trade a loop for a stale closure. 13 tests +cover this, each verified by reverting the fix it guards. diff --git a/packages/client-react/src/data-hooks.tsx b/packages/client-react/src/data-hooks.tsx index e204fadb19..3a187a9795 100644 --- a/packages/client-react/src/data-hooks.tsx +++ b/packages/client-react/src/data-hooks.tsx @@ -10,6 +10,7 @@ import { useState, useEffect, useCallback, useRef } from 'react'; import { QueryAST, FilterCondition } from '@objectstack/spec/data'; import { PaginatedResult } from '@objectstack/client'; import { useClient } from './context'; +import { stableKey, useEventCallback } from './internal-deps'; /** * Query options for useQuery hook. @@ -113,6 +114,26 @@ export function useQuery( const resolvedLimit = limit; const resolvedOffset = offset; + // The query shape as a VALUE (#4693). `where` / `fields` / `orderBy` are + // objects and arrays, and the documented usage builds them inline, so keying + // the fetch on their identities re-ran it every render — and since it calls + // `setData`, every render caused another render. Measured before this fix: + // `useQuery('todo_task', { where: { status: 'open' } })` issued 4691 `find` + // calls in 250ms; the same call with a hoisted options object issued 1. + const queryKey = stableKey({ + query, + where: resolvedWhere, + fields: resolvedFields, + orderBy: resolvedSort, + limit: resolvedLimit, + offset: resolvedOffset, + }); + + // Handlers say what to do with a result; they are not part of what is being + // fetched, so they must not drive refetching. + const handleSuccess = useEventCallback(onSuccess); + const handleError = useEventCallback(onError); + const fetchData = useCallback(async (isRefetch = false) => { if (!enabled) return; @@ -141,16 +162,16 @@ export function useQuery( } setData(result); - onSuccess?.(result); + handleSuccess(result); } catch (err) { const error = err instanceof Error ? err : new Error('Query failed'); setError(error); - onError?.(error); + handleError(error); } finally { setIsLoading(false); setIsRefetching(false); } - }, [client, object, query, resolvedFields, resolvedWhere, resolvedSort, resolvedLimit, resolvedOffset, enabled, onSuccess, onError]); + }, [client, object, queryKey, enabled, handleSuccess, handleError]); // Initial fetch and dependency-based refetch useEffect(() => { @@ -520,6 +541,18 @@ export function useInfiniteQuery( const resolvedWhere = where; const resolvedSort = orderBy; + // Same value-keyed dependency as useQuery (#4693) — measured at 6611 `find` + // calls in 250ms before this fix, with inline options. + const queryKey = stableKey({ + query, + where: resolvedWhere, + fields: resolvedFields, + orderBy: resolvedSort, + pageSize, + }); + const handleSuccess = useEventCallback(onSuccess); + const handleError = useEventCallback(onError); + const [pages, setPages] = useState[]>([]); const [isLoading, setIsLoading] = useState(true); const [isFetchingNextPage, setIsFetchingNextPage] = useState(false); @@ -564,16 +597,16 @@ export function useInfiniteQuery( const hasMore = fetchedCount === pageSize; setHasNextPage(hasMore); - onSuccess?.(result); + handleSuccess(result); } catch (err) { const error = err instanceof Error ? err : new Error('Query failed'); setError(error); - onError?.(error); + handleError(error); } finally { setIsLoading(false); setIsFetchingNextPage(false); } - }, [client, object, query, resolvedFields, resolvedWhere, resolvedSort, pageSize, onSuccess, onError]); + }, [client, object, queryKey, handleSuccess, handleError]); // Initial fetch useEffect(() => { diff --git a/packages/client-react/src/hook-dependency-identity.test.tsx b/packages/client-react/src/hook-dependency-identity.test.tsx new file mode 100644 index 0000000000..0d53448486 --- /dev/null +++ b/packages/client-react/src/hook-dependency-identity.test.tsx @@ -0,0 +1,360 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Regression tests for the dependency-identity loops (#4693) and the + * subscription churn (#4694). + * + * Five hooks keyed a `useCallback`/`useEffect` on values the caller supplies + * inline — `where`/`fields`/`orderBy` objects, `onSuccess`/`onError` handlers, + * the `fetcher` `useMetadata` takes as a required argument. Inline means a new + * identity every render, so the effect re-ran every render, and because the + * fetch hooks call `setState` that render caused another. Measured on `main` + * before the fix, requests issued in 250ms from a single mounted component: + * + * | hook | before | after | + * |-----------------------------------|-------:|------:| + * | useQuery (inline `where`) | 4691 | 1 | + * | useInfiniteQuery (inline `where`) | 6611 | 1 | + * | useObject (NO options at all) | 4306 | 1 | + * | useView (inline `onSuccess`) | 8197 | 1 | + * | useMetadata (inline `fetcher`) | 7654 | 1 | + * + * `useObject` and `useMetadata` needed no particular usage to loop — the former + * depended on its own `data`/`etag` state while writing both, and the latter + * takes its fetcher positionally, so there is no non-inline way to call it. + * + * The counts are asserted as exact numbers rather than "small": a bound like + * `< 10` would pass on a loop that merely got slower, which is the failure this + * whole file exists to prevent. + * + * Each fix also has a matching staleness test. The mechanism — hold the handler + * in a ref, drop it from the deps — trades one bug for a worse one if the ref + * is read late or synced wrong, so every stabilized handler is asserted to run + * its LATEST version, not the one captured at subscribe time. + */ + +import * as React from 'react'; +import { describe, it, expect, vi } from 'vitest'; +import { render, renderHook, act } from '@testing-library/react'; +import type { ObjectStackClient } from '@objectstack/client'; +import type { DataEvent } from '@objectstack/spec/api'; +import { ObjectStackProvider } from './context'; +import { useQuery, useInfiniteQuery } from './data-hooks'; +import { useObject, useView, useMetadata } from './metadata-hooks'; +import { useAutoRefresh, useDataSubscriptionCallback } from './realtime-hooks'; +import { stableKey } from './internal-deps'; + +const RECORD_EVENT: DataEvent = { + id: 'f47ac10b-58cc-4372-a567-0e02b2c3d479', + type: 'data.record.updated', + object: 'project_task', + recordId: 'task_1', + timestamp: '2026-08-02T12:00:00.000Z', +} as DataEvent; + +/** Long enough for a runaway loop to make itself obvious (it managed ~5000). */ +const settle = () => new Promise((resolve) => setTimeout(resolve, 250)); + +function createFakeClient() { + const find = vi.fn(async (_object: string, _options?: Record) => ({ + records: [], + total: 0, + })); + const getCached = vi.fn(async (_objectName: string, _options?: Record) => ({ + data: { name: 'todo_task' }, + etag: { value: 'W/"1"' }, + notModified: false, + })); + const getItem = vi.fn(async (_kind: string, _objectName: string) => ({ name: 'todo_task' })); + const getView = vi.fn(async (_objectName: string, _viewType: string) => ({ + name: 'todo_task_list', + })); + const subscriptions: { unsubscribe: ReturnType; deliver: (e: DataEvent) => void }[] = []; + + const client = { + data: { find, query: find }, + meta: { getCached, getItem, getView }, + events: { + subscribeData: vi.fn((_object: string, callback: (e: DataEvent) => void) => { + const unsubscribe = vi.fn(); + subscriptions.push({ unsubscribe, deliver: callback }); + return unsubscribe; + }), + subscribeBulkData: vi.fn(() => vi.fn()), + subscribeMetadata: vi.fn(() => vi.fn()), + }, + setLocale: vi.fn(), + } as unknown as ObjectStackClient; + + return { client, find, getCached, getItem, getView, subscriptions }; +} + +function mount(client: ObjectStackClient, Component: React.ComponentType) { + return render( + + + + ); +} + +// --------------------------------------------------------------------------- +// #4693 — the fetch hooks issue exactly one request +// --------------------------------------------------------------------------- + +describe('#4693 fetch hooks key on values, not identities', () => { + it('useQuery fetches once with an inline options object', async () => { + const { client, find } = createFakeClient(); + mount(client, () => { + useQuery('todo_task', { where: { status: 'open' } as any, fields: ['id', 'subject'] }); + return null; + }); + + await settle(); + + expect(find).toHaveBeenCalledTimes(1); + }); + + it('useInfiniteQuery fetches once with an inline options object', async () => { + const { client, find } = createFakeClient(); + mount(client, () => { + useInfiniteQuery('todo_task', { where: { status: 'open' } as any }); + return null; + }); + + await settle(); + + expect(find).toHaveBeenCalledTimes(1); + }); + + it('useObject fetches once — it used to loop on its own state', async () => { + const { client, getCached } = createFakeClient(); + mount(client, () => { + useObject('todo_task'); + return null; + }); + + await settle(); + + // The pre-fix loop needed no options at all: `data` and `etag` were in the + // fetch's dependency array and the fetch wrote both. + expect(getCached).toHaveBeenCalledTimes(1); + }); + + it('useView fetches once with an inline onSuccess', async () => { + const { client, getView } = createFakeClient(); + mount(client, () => { + useView('todo_task', 'list', { onSuccess: () => {} }); + return null; + }); + + await settle(); + + expect(getView).toHaveBeenCalledTimes(1); + }); + + it('useMetadata fetches once with an inline fetcher', async () => { + const { client } = createFakeClient(); + const fetcher = vi.fn(async () => ({ ok: true })); + mount(client, () => { + useMetadata(() => fetcher()); + return null; + }); + + await settle(); + + // `fetcher` is positional and required, so inline is the only natural call. + expect(fetcher).toHaveBeenCalledTimes(1); + }); +}); + +// --------------------------------------------------------------------------- +// #4693 — a changed VALUE still refetches +// --------------------------------------------------------------------------- + +describe('#4693 value changes still drive a refetch', () => { + it('useQuery refetches when the where VALUE changes, not when it is rebuilt', async () => { + const { client, find } = createFakeClient(); + + function Harness({ status }: { status: string }) { + useQuery('todo_task', { where: { status } as any }); + return null; + } + + const { rerender } = render( + + + + ); + await settle(); + expect(find).toHaveBeenCalledTimes(1); + + // Same value, freshly built object — must not refetch. Without the + // value-keyed dependency this is where the loop restarted. + rerender( + + + + ); + await settle(); + expect(find).toHaveBeenCalledTimes(1); + + // Genuinely different filter — must refetch, or the fix would have traded a + // loop for a stale list. + rerender( + + + + ); + await settle(); + expect(find).toHaveBeenCalledTimes(2); + expect((find.mock.calls[1] as any)[1].where).toEqual({ status: 'done' }); + }); + + it('useObject refetches when the object name changes', async () => { + const { client, getCached } = createFakeClient(); + + function Harness({ name }: { name: string }) { + useObject(name); + return null; + } + + const { rerender } = render( + + + + ); + await settle(); + expect(getCached).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + await settle(); + expect(getCached).toHaveBeenCalledTimes(2); + expect(getCached.mock.calls[1][0]).toBe('account'); + }); +}); + +// --------------------------------------------------------------------------- +// #4693 / #4694 — stabilized handlers must not go stale +// --------------------------------------------------------------------------- + +describe('#4693 stabilized handlers still run their latest version', () => { + it('useQuery calls the onSuccess from the current render', async () => { + const { client } = createFakeClient(); + const first = vi.fn(); + const second = vi.fn(); + + function Harness({ onSuccess }: { onSuccess: () => void }) { + const { refetch } = useQuery('todo_task', { where: { status: 'open' } as any, onSuccess }); + // Expose refetch so the test can trigger a second call after the handler + // has been swapped. + (globalThis as any).__refetch = refetch; + return null; + } + + const { rerender } = render( + + + + ); + await settle(); + expect(first).toHaveBeenCalledTimes(1); + + rerender( + + + + ); + await settle(); + + // Swapping the handler must not refetch (that was the loop)... + expect(second).not.toHaveBeenCalled(); + + // ...but the next fetch must reach the NEW handler, not the one captured + // when the effect first ran. + await act(async () => { + await (globalThis as any).__refetch(); + }); + expect(second).toHaveBeenCalledTimes(1); + expect(first).toHaveBeenCalledTimes(1); + delete (globalThis as any).__refetch; + }); +}); + +// --------------------------------------------------------------------------- +// #4694 — subscriptions stop churning +// --------------------------------------------------------------------------- + +describe('#4694 subscription callbacks do not churn the subscription', () => { + it('useAutoRefresh subscribes once across renders with an unmemoized refetch', () => { + const { client } = createFakeClient(); + const events = (client as any).events; + + const { rerender } = renderHook( + () => useAutoRefresh('project_task', () => {}), + { + wrapper: ({ children }: { children: React.ReactNode }) => ( + {children} + ), + } + ); + + rerender(); + rerender(); + + // Before the fix: 3 renders → 3 subscriptions on each stream, each render + // unsubscribing the previous one and losing anything delivered in the gap. + expect(events.subscribeData).toHaveBeenCalledTimes(1); + expect(events.subscribeBulkData).toHaveBeenCalledTimes(1); + }); + + it('useDataSubscriptionCallback delivers to the latest callback after a swap', () => { + const { client, subscriptions } = createFakeClient(); + const first = vi.fn(); + const second = vi.fn(); + + const { rerender } = renderHook( + ({ callback }) => useDataSubscriptionCallback('project_task', callback), + { + wrapper: ({ children }: { children: React.ReactNode }) => ( + {children} + ), + initialProps: { callback: first as (e: DataEvent) => void }, + } + ); + + rerender({ callback: second as (e: DataEvent) => void }); + + // Still one subscription — and it must invoke the CURRENT callback. A ref + // synced in the wrong phase would deliver to `first` forever, which is a + // worse bug than the churn this replaced. + expect(subscriptions).toHaveLength(1); + act(() => subscriptions[0].deliver(RECORD_EVENT)); + + expect(second).toHaveBeenCalledTimes(1); + expect(first).not.toHaveBeenCalled(); + }); +}); + +// --------------------------------------------------------------------------- +// stableKey +// --------------------------------------------------------------------------- + +describe('stableKey', () => { + it('is insensitive to key order but sensitive to values', () => { + expect(stableKey({ a: 1, b: 2 })).toBe(stableKey({ b: 2, a: 1 })); + expect(stableKey({ a: 1 })).not.toBe(stableKey({ a: 2 })); + }); + + it('preserves array order, which is semantic for orderBy', () => { + expect(stableKey(['-created_at', 'name'])).not.toBe(stableKey(['name', '-created_at'])); + }); + + it('distinguishes nested differences and undefined from absent', () => { + expect(stableKey({ where: { a: { b: 1 } } })).not.toBe(stableKey({ where: { a: { b: 2 } } })); + expect(stableKey({ a: undefined })).not.toBe(stableKey({})); + }); +}); diff --git a/packages/client-react/src/internal-deps.ts b/packages/client-react/src/internal-deps.ts new file mode 100644 index 0000000000..58706035aa --- /dev/null +++ b/packages/client-react/src/internal-deps.ts @@ -0,0 +1,76 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Dependency-array primitives (#4693, #4694) — internal, not exported from the + * package entry point. + * + * Every fetch and subscription hook here keys a `useCallback`/`useEffect` on + * values the caller supplies inline: `where` / `fields` / `orderBy` objects, + * `onSuccess` / `onError` handlers, the `fetcher` `useMetadata` takes as a + * required argument. Inline means a fresh identity on every render, so the + * effect re-ran on every render — and for the fetch hooks that effect calls + * `setState`, which renders again. Five hooks were in an unbounded request + * loop under their own documented usage. + * + * The two helpers below fix the two halves of that: + * + * - {@link stableKey} turns a structural value into a dependency that changes + * when the *value* changes rather than when the object is rebuilt; + * - {@link useEventCallback} gives a caller's handler a fixed identity, so + * passing it inline no longer re-runs anything. + * + * Neither is a substitute for the caller memoizing — they remove the need to. + * Correctness must not rest on every call site remembering `useMemo`, least of + * all when the TSDoc examples themselves pass object literals. + */ + +import { useCallback, useEffect, useRef } from 'react'; + +/** + * Order-independent stringify, used to derive a dependency from a structural + * value. Object keys are sorted so `{ a, b }` and `{ b, a }` agree; array order + * is preserved because it is semantic (`orderBy: ['-created_at', 'name']`). + * + * Mirrors the same helper in `service-settings` and `service-automation` — kept + * local for the same reason they are: it is three lines and a shared util + * package would be a heavier dependency than the code it carries. + * + * Values JSON cannot represent (functions, symbols) collapse to `undefined` + * here. That is correct for this use: a handler's identity must not drive a + * refetch, which is exactly what {@link useEventCallback} is for. + */ +export function stableKey(input: unknown): string { + if (input === null || typeof input !== 'object') return JSON.stringify(input) ?? 'undefined'; + if (Array.isArray(input)) return '[' + input.map(stableKey).join(',') + ']'; + const obj = input as Record; + const keys = Object.keys(obj).sort(); + return '{' + keys.map((k) => JSON.stringify(k) + ':' + stableKey(obj[k])).join(',') + '}'; +} + +/** + * Wraps a caller-supplied handler in a function whose identity never changes, + * while always invoking the most recent version. + * + * This is what lets the fetch and subscription effects drop `onSuccess` / + * `onError` / `callback` from their dependency arrays. Those handlers say what + * to do when something happens; they are not part of *what is subscribed to* or + * *what is fetched*, so they have no business re-running an effect. + * + * The ref is updated in an effect rather than during render: a render may be + * thrown away under concurrent rendering, and writing through a ref then would + * publish a handler from a render that never committed. + * + * Returns `undefined` from the call when no handler was supplied, so optional + * handlers need no call-site guard. + */ +export function useEventCallback( + fn: ((...args: A) => R) | undefined +): (...args: A) => R | undefined { + const ref = useRef(fn); + + useEffect(() => { + ref.current = fn; + }, [fn]); + + return useCallback((...args: A) => ref.current?.(...args), []); +} diff --git a/packages/client-react/src/metadata-hooks.tsx b/packages/client-react/src/metadata-hooks.tsx index b31f4cc1b7..bf5ab00100 100644 --- a/packages/client-react/src/metadata-hooks.tsx +++ b/packages/client-react/src/metadata-hooks.tsx @@ -6,8 +6,9 @@ * React hooks for accessing ObjectStack metadata (schemas, views, fields) */ -import { useState, useEffect, useCallback } from 'react'; +import { useState, useEffect, useCallback, useRef } from 'react'; import { useClient, useObjectStackLocale } from './context'; +import { useEventCallback } from './internal-deps'; /** * Metadata query options @@ -89,6 +90,22 @@ export function useObject( onError } = options; + // `data` and `etag` are this hook's OWN state, and the fetch writes both. + // Depending on them made the fetch its own trigger: `setData` → new `data` + // identity → new `fetchMetadata` → the effect below re-ran → fetch again. + // Unlike the data hooks this needed no particular usage to fire — measured at + // 4306 metadata requests in 250ms for a bare `useObject('todo_task')` + // (#4693). They are read, never depended on, so they belong in refs. + const dataRef = useRef(data); + const etagRef = useRef(etag); + useEffect(() => { + dataRef.current = data; + etagRef.current = etag; + }, [data, etag]); + + const handleSuccess = useEventCallback(onSuccess); + const handleError = useEventCallback(onError); + const fetchMetadata = useCallback(async () => { if (!enabled) return; @@ -100,7 +117,7 @@ export function useObject( if (useCache) { // Use cached metadata endpoint const result = await client.meta.getCached(objectName, { - ifNoneMatch: ifNoneMatch || etag, + ifNoneMatch: ifNoneMatch || etagRef.current, ifModifiedSince }); @@ -113,21 +130,21 @@ export function useObject( } } - onSuccess?.(result.data || data); + handleSuccess(result.data || dataRef.current); } else { // Direct fetch without cache const result = await client.meta.getItem('object', objectName); setData(result); - onSuccess?.(result); + handleSuccess(result); } } catch (err) { const error = err instanceof Error ? err : new Error('Failed to fetch object metadata'); setError(error); - onError?.(error); + handleError(error); } finally { setIsLoading(false); } - }, [client, objectName, locale, enabled, useCache, ifNoneMatch, ifModifiedSince, etag, data, onSuccess, onError]); + }, [client, objectName, locale, enabled, useCache, ifNoneMatch, ifModifiedSince, handleSuccess, handleError]); useEffect(() => { fetchMetadata(); @@ -180,6 +197,9 @@ export function useView( const { enabled = true, onSuccess, onError } = options; + const handleSuccess = useEventCallback(onSuccess); + const handleError = useEventCallback(onError); + const fetchView = useCallback(async () => { if (!enabled) return; @@ -189,15 +209,15 @@ export function useView( const result = await client.meta.getView(objectName, viewType); setData(result); - onSuccess?.(result); + handleSuccess(result); } catch (err) { const error = err instanceof Error ? err : new Error('Failed to fetch view configuration'); setError(error); - onError?.(error); + handleError(error); } finally { setIsLoading(false); } - }, [client, objectName, viewType, locale, enabled, onSuccess, onError]); + }, [client, objectName, viewType, locale, enabled, handleSuccess, handleError]); useEffect(() => { fetchView(); @@ -286,6 +306,15 @@ export function useMetadata( const { enabled = true, onSuccess, onError } = options; + // `fetcher` is a REQUIRED positional argument, so an inline arrow is the only + // natural way to call this hook — which made the loop unconditional in + // practice (7654 fetcher invocations in 250ms before this fix, #4693). + // Stabilizing it here means the fetch re-runs on `client` / `locale` / + // `enabled` changes, as intended, and not on the caller's render cadence. + const runFetcher = useEventCallback(fetcher); + const handleSuccess = useEventCallback(onSuccess); + const handleError = useEventCallback(onError); + const fetchMetadata = useCallback(async () => { if (!enabled) return; @@ -293,17 +322,17 @@ export function useMetadata( setIsLoading(true); setError(null); - const result = await fetcher(client); - setData(result); - onSuccess?.(result); + const result = await runFetcher(client); + setData(result as T); + handleSuccess(result as T); } catch (err) { const error = err instanceof Error ? err : new Error('Failed to fetch metadata'); setError(error); - onError?.(error); + handleError(error); } finally { setIsLoading(false); } - }, [client, fetcher, locale, enabled, onSuccess, onError]); + }, [client, runFetcher, locale, enabled, handleSuccess, handleError]); useEffect(() => { fetchMetadata(); diff --git a/packages/client-react/src/realtime-hooks.tsx b/packages/client-react/src/realtime-hooks.tsx index e57ec36251..9135e6c827 100644 --- a/packages/client-react/src/realtime-hooks.tsx +++ b/packages/client-react/src/realtime-hooks.tsx @@ -7,9 +7,10 @@ * Events are automatically cleaned up when components unmount. */ -import { useEffect, useState, useCallback } from 'react'; +import { useEffect, useState } from 'react'; import type { MetadataEvent, DataEvent, BulkDataEvent } from '@objectstack/spec/api'; import { useClient } from './context'; +import { useEventCallback } from './internal-deps'; /** * Hook to subscribe to metadata events @@ -134,20 +135,25 @@ export function useMetadataSubscriptionCallback( options?: { packageId?: string } ): void { const client = useClient(); + // The callback is what RUNS on an event, not part of what is subscribed to + // (#4694). Depending on its identity tore down and reopened the subscription + // on every render whenever the caller passed an inline function — which the + // examples above do — losing any event that arrived in the gap. + const handleEvent = useEventCallback(callback); useEffect(() => { if (!client) return; const unsubscribe = client.events.subscribeMetadata( type, - callback, + handleEvent, options ); return () => { unsubscribe(); }; - }, [client, type, callback, options?.packageId]); + }, [client, type, handleEvent, options?.packageId]); } /** @@ -176,20 +182,23 @@ export function useDataSubscriptionCallback( options?: { recordId?: string } ): void { const client = useClient(); + // Stable identity so an inline callback does not churn the subscription on + // every render (#4694) — see useMetadataSubscriptionCallback. + const handleEvent = useEventCallback(callback); useEffect(() => { if (!client) return; const unsubscribe = client.events.subscribeData( object, - callback, + handleEvent, options ); return () => { unsubscribe(); }; - }, [client, object, callback, options?.recordId]); + }, [client, object, handleEvent, options?.recordId]); } /** @@ -270,16 +279,19 @@ export function useBulkDataSubscriptionCallback( callback: (event: BulkDataEvent) => void ): void { const client = useClient(); + // Stable identity so an inline callback does not churn the subscription on + // every render (#4694) — see useMetadataSubscriptionCallback. + const handleEvent = useEventCallback(callback); useEffect(() => { if (!client) return; - const unsubscribe = client.events.subscribeBulkData(object, callback); + const unsubscribe = client.events.subscribeBulkData(object, handleEvent); return () => { unsubscribe(); }; - }, [client, object, callback]); + }, [client, object, handleEvent]); } /** @@ -355,19 +367,14 @@ export function useAutoRefresh( refetch: () => void, options?: { recordId?: string } ): void { - const handleEvent = useCallback((_event: DataEvent) => { - // Refetch on any data change - refetch(); - }, [refetch]); + // No `useCallback` needed: both subscription hooks stabilize the handler + // themselves (#4694), so a caller passing an unmemoized `refetch` — which + // `useQuery` returned on every render before #4693 — no longer resubscribes. + useDataSubscriptionCallback(object, (_event: DataEvent) => refetch(), options); // A bulk event carries only a count, so when `options.recordId` narrows this // hook to one record there is no way to tell whether that record was in the // match set. Refetch anyway: a redundant query is cheap, and the alternative // is showing a record that a predicate write already changed. - const handleBulkEvent = useCallback((_event: BulkDataEvent) => { - refetch(); - }, [refetch]); - - useDataSubscriptionCallback(object, handleEvent, options); - useBulkDataSubscriptionCallback(object, handleBulkEvent); + useBulkDataSubscriptionCallback(object, (_event: BulkDataEvent) => refetch()); }