Skip to content

Commit 9e44db4

Browse files
committed
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 every render, so the effect re-ran every render; because the fetch hooks call `setState`, that render caused another. Under the hooks' own documented usage this was an unbounded request loop. Requests issued in 250ms by one mounted component, 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, 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 exactly what useQuery returned every render — resubscribed on both streams every render, losing any event delivered in the unsubscribe/resubscribe gap. Adds two internal primitives (not exported): `stableKey` derives a dependency from a structural VALUE (sorted keys, array order preserved, since `orderBy` is positional) so a rebuilt-but-equal object is a no-op; `useEventCallback` gives a handler a fixed identity while always invoking its latest version, synced in an effect rather than during render so a discarded concurrent render cannot publish a handler that never committed. Fixing this by asking callers to memoize was rejected: the TSDoc examples themselves pass object literals, and correctness must not rest on every call site remembering `useMemo`. 13 tests, each verified by reverting the fix it guards — restoring identity deps in useQuery (3 red), useObject's self-referential state (3), useMetadata's fetcher dep (2) and the subscription callback dep (2). Counts are asserted exactly rather than as an upper bound, which would pass on a loop that merely got slower. Coverage includes the inverse direction: a changed value still refetches, and every stabilized handler runs its newest version, so the ref indirection cannot silently trade a loop for a stale closure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LYnZrTwXbrctB8E8HpJAPT
1 parent 0076683 commit 9e44db4

6 files changed

Lines changed: 586 additions & 37 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@objectstack/client-react": patch
3+
---
4+
5+
fix(client-react): stop five hooks from looping on dependency identity (#4693, #4694)
6+
7+
Five hooks keyed a `useCallback`/`useEffect` on values the caller supplies
8+
inline — `where` / `fields` / `orderBy` objects, `onSuccess` / `onError`
9+
handlers, and the `fetcher` `useMetadata` takes as a required positional
10+
argument. Inline means a fresh identity on every render, so the effect re-ran on
11+
every render; because the fetch hooks call `setState`, that render caused
12+
another. The result was an unbounded request loop under the hooks' own
13+
documented usage.
14+
15+
Requests issued in 250ms by a single mounted component, measured before and
16+
after:
17+
18+
| hook | before | after |
19+
|-----------------------------------|-------:|------:|
20+
| `useQuery` (inline `where`) | 4691 | 1 |
21+
| `useInfiniteQuery` (inline `where`) | 6611 | 1 |
22+
| `useObject` (no options at all) | 4306 | 1 |
23+
| `useView` (inline `onSuccess`) | 8197 | 1 |
24+
| `useMetadata` (inline `fetcher`) | 7654 | 1 |
25+
26+
`useObject` and `useMetadata` needed no particular usage to loop: the former
27+
depended on its own `data` and `etag` state while writing both, and the latter
28+
takes its fetcher positionally, so there is no non-inline way to call it.
29+
`useMutation` was never affected — no effect drives it.
30+
31+
The same root cause churned the realtime subscriptions (#4694):
32+
`useAutoRefresh` with an unmemoized `refetch` — which is what `useQuery`
33+
returned on every render — resubscribed on both streams every render, losing any
34+
event delivered in the unsubscribe/resubscribe gap.
35+
36+
Two internal primitives fix both halves: `stableKey` derives a dependency from a
37+
structural value (sorted keys, array order preserved) so a rebuilt-but-equal
38+
object is a no-op, and `useEventCallback` gives a handler a fixed identity while
39+
always invoking its latest version. Neither is exported.
40+
41+
A changed *value* still refetches, and every stabilized handler is asserted to
42+
run its newest version rather than the one captured when the effect first ran —
43+
the ref indirection would otherwise trade a loop for a stale closure. 13 tests
44+
cover this, each verified by reverting the fix it guards.

packages/client-react/src/data-hooks.tsx

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { useState, useEffect, useCallback, useRef } from 'react';
1010
import { QueryAST, FilterCondition } from '@objectstack/spec/data';
1111
import { PaginatedResult } from '@objectstack/client';
1212
import { useClient } from './context';
13+
import { stableKey, useEventCallback } from './internal-deps';
1314

1415
/**
1516
* Query options for useQuery hook.
@@ -113,6 +114,26 @@ export function useQuery<T = any>(
113114
const resolvedLimit = limit;
114115
const resolvedOffset = offset;
115116

117+
// The query shape as a VALUE (#4693). `where` / `fields` / `orderBy` are
118+
// objects and arrays, and the documented usage builds them inline, so keying
119+
// the fetch on their identities re-ran it every render — and since it calls
120+
// `setData`, every render caused another render. Measured before this fix:
121+
// `useQuery('todo_task', { where: { status: 'open' } })` issued 4691 `find`
122+
// calls in 250ms; the same call with a hoisted options object issued 1.
123+
const queryKey = stableKey({
124+
query,
125+
where: resolvedWhere,
126+
fields: resolvedFields,
127+
orderBy: resolvedSort,
128+
limit: resolvedLimit,
129+
offset: resolvedOffset,
130+
});
131+
132+
// Handlers say what to do with a result; they are not part of what is being
133+
// fetched, so they must not drive refetching.
134+
const handleSuccess = useEventCallback(onSuccess);
135+
const handleError = useEventCallback(onError);
136+
116137
const fetchData = useCallback(async (isRefetch = false) => {
117138
if (!enabled) return;
118139

@@ -141,16 +162,16 @@ export function useQuery<T = any>(
141162
}
142163

143164
setData(result);
144-
onSuccess?.(result);
165+
handleSuccess(result);
145166
} catch (err) {
146167
const error = err instanceof Error ? err : new Error('Query failed');
147168
setError(error);
148-
onError?.(error);
169+
handleError(error);
149170
} finally {
150171
setIsLoading(false);
151172
setIsRefetching(false);
152173
}
153-
}, [client, object, query, resolvedFields, resolvedWhere, resolvedSort, resolvedLimit, resolvedOffset, enabled, onSuccess, onError]);
174+
}, [client, object, queryKey, enabled, handleSuccess, handleError]);
154175

155176
// Initial fetch and dependency-based refetch
156177
useEffect(() => {
@@ -520,6 +541,18 @@ export function useInfiniteQuery<T = any>(
520541
const resolvedWhere = where;
521542
const resolvedSort = orderBy;
522543

544+
// Same value-keyed dependency as useQuery (#4693) — measured at 6611 `find`
545+
// calls in 250ms before this fix, with inline options.
546+
const queryKey = stableKey({
547+
query,
548+
where: resolvedWhere,
549+
fields: resolvedFields,
550+
orderBy: resolvedSort,
551+
pageSize,
552+
});
553+
const handleSuccess = useEventCallback(onSuccess);
554+
const handleError = useEventCallback(onError);
555+
523556
const [pages, setPages] = useState<PaginatedResult<T>[]>([]);
524557
const [isLoading, setIsLoading] = useState(true);
525558
const [isFetchingNextPage, setIsFetchingNextPage] = useState(false);
@@ -564,16 +597,16 @@ export function useInfiniteQuery<T = any>(
564597
const hasMore = fetchedCount === pageSize;
565598
setHasNextPage(hasMore);
566599

567-
onSuccess?.(result);
600+
handleSuccess(result);
568601
} catch (err) {
569602
const error = err instanceof Error ? err : new Error('Query failed');
570603
setError(error);
571-
onError?.(error);
604+
handleError(error);
572605
} finally {
573606
setIsLoading(false);
574607
setIsFetchingNextPage(false);
575608
}
576-
}, [client, object, query, resolvedFields, resolvedWhere, resolvedSort, pageSize, onSuccess, onError]);
609+
}, [client, object, queryKey, handleSuccess, handleError]);
577610

578611
// Initial fetch
579612
useEffect(() => {

0 commit comments

Comments
 (0)