diff --git a/.changeset/eleven-gifts-shine.md b/.changeset/eleven-gifts-shine.md new file mode 100644 index 000000000..c87972366 --- /dev/null +++ b/.changeset/eleven-gifts-shine.md @@ -0,0 +1,12 @@ +--- +'@tanstack/react-db': patch +--- + +fix(react-db): Avoid using refs to track changes in React hooks. + +Using refs to track previous versions of variables, and reading those refs +during render, is an anti-pattern that breaks the Rules of Hooks and may lead to +subtle bugs, especially in concurrent mode. + +Using state instead is more idiomatic and ensures there will be no state +tearing, even during concurrent mode updates. diff --git a/packages/react-db/src/useLiveQuery.ts b/packages/react-db/src/useLiveQuery.ts index 2ada39077..02427d1d3 100644 --- a/packages/react-db/src/useLiveQuery.ts +++ b/packages/react-db/src/useLiveQuery.ts @@ -1,4 +1,4 @@ -import { useRef, useSyncExternalStore } from 'react' +import { useCallback, useState, useSyncExternalStore } from 'react' import { BaseQueryBuilder, createLiveQueryCollection, @@ -13,7 +13,6 @@ import type { InferResultType, InitialQueryBuilder, LiveQueryCollectionConfig, - LiveQueryObserver, NonSingleResult, QueryBuilder, SingleResult, @@ -321,28 +320,11 @@ export function useLiveQuery( // Check if it's already a collection const inputIsCollection = isCollection(configOrQueryOrCollection) - // Use refs to cache collection and track dependencies - const collectionRef = useRef | null>( - null, - ) - const depsRef = useRef | null>(null) - const configRef = useRef(null) - - // The shared observer owns subscription, the ready-race, and the snapshot. - const observerRef = useRef | null>( - null, - ) - - // Check if we need to create/recreate the collection - const needsNewCollection = - !collectionRef.current || - (inputIsCollection && configRef.current !== configOrQueryOrCollection) || - (!inputIsCollection && - (depsRef.current === null || - depsRef.current.length !== deps.length || - depsRef.current.some((dep, i) => dep !== deps[i]))) - - if (needsNewCollection) { + const createCollection = (): Collection< + object, + string | number, + {} + > | null => { if (inputIsCollection) { // Warn when passing a collection directly with on-demand sync mode // In on-demand mode, data is only loaded when queries with predicates request it @@ -361,8 +343,7 @@ export function useLiveQuery( } // It's already a collection, ensure sync is started for React hooks configOrQueryOrCollection.startSyncImmediate() - collectionRef.current = configOrQueryOrCollection - configRef.current = configOrQueryOrCollection + return configOrQueryOrCollection } else { // Handle different callback return types if (typeof configOrQueryOrCollection === `function`) { @@ -372,22 +353,22 @@ export function useLiveQuery( if (result === undefined || result === null) { // Callback returned undefined/null - disabled query - collectionRef.current = null + return null } else if (isCollection(result)) { // Callback returned a Collection instance - use it directly result.startSyncImmediate() - collectionRef.current = result + return result } else if (result instanceof BaseQueryBuilder) { // Callback returned QueryBuilder - create live query collection using the original callback // (not the result, since the result might be from a different query builder instance) - collectionRef.current = createLiveQueryCollection({ + return createLiveQueryCollection({ query: configOrQueryOrCollection, startSync: true, gcTime: DEFAULT_GC_TIME_MS, }) } else if (result && typeof result === `object`) { // Assume it's a LiveQueryCollectionConfig - collectionRef.current = createLiveQueryCollection({ + return createLiveQueryCollection({ startSync: true, gcTime: DEFAULT_GC_TIME_MS, ...result, @@ -398,51 +379,76 @@ export function useLiveQuery( `useLiveQuery callback must return a QueryBuilder, LiveQueryCollectionConfig, Collection, undefined, or null. Got: ${typeof result}`, ) } - depsRef.current = [...deps] } else { // Original logic for config objects - collectionRef.current = createLiveQueryCollection({ + return createLiveQueryCollection({ startSync: true, gcTime: DEFAULT_GC_TIME_MS, ...configOrQueryOrCollection, }) - depsRef.current = [...deps] } } } - // Recreate the observer when the underlying collection changes. The observer - // is not disposed explicitly here or on unmount: `useSyncExternalStore` - // unsubscribes it when the subscribe changes or the component unmounts, which - // detaches the collection subscription; the observer is then GC'd. (An unmount - // effect that disposed it would misfire under StrictMode/offscreen effect - // replay, leaving a disposed observer in the ref.) + // Use state to cache collection and track dependencies + const [collection, setCollection] = useState(createCollection) + const [prevCollection, setPrevCollection] = useState(collection) + const [prevDeps, setPrevDeps] = useState( + !inputIsCollection ? [...deps] : null, + ) + const [prevConfig, setPrevConfig] = useState( + inputIsCollection ? configOrQueryOrCollection : null, + ) + + // Check if we need to create/recreate the collection + const needsNewCollection = + (inputIsCollection && prevConfig !== configOrQueryOrCollection) || + (!inputIsCollection && + (prevDeps === null || + prevDeps.length !== deps.length || + prevDeps.some((dep, i) => dep !== deps[i]))) + if (needsNewCollection) { + setCollection(createCollection) + if (isCollection(configOrQueryOrCollection)) { + setPrevConfig(configOrQueryOrCollection) + } else { + setPrevDeps([...deps]) + } + } + + // The shared observer owns subscription, the ready-race, and the snapshot. + const [observer, setObserver] = useState(() => // Defer the initial notify: useSyncExternalStore must not be notified // synchronously during subscribe. // Wholesale mode: React re-reads getSnapshot() on notify, keeps the // hook's pre-observer loading policy, and — because wholesale delivers // nothing synchronously during subscribe — never notifies // useSyncExternalStore inside its own subscribe call. - observerRef.current = createLiveQueryObserver(collectionRef.current, { - mode: `wholesale`, - }) + createLiveQueryObserver(collection, { mode: `wholesale` }), + ) + + // Recreate the observer when the underlying collection changes. The observer + // is not disposed explicitly here or on unmount: `useSyncExternalStore` + // unsubscribes it when the subscribe changes or the component unmounts, which + // detaches the collection subscription; the observer is then GC'd. (An unmount + // effect that disposed it would misfire under StrictMode/offscreen effect + // replay, leaving a disposed observer in the ref.) + if (prevCollection !== collection) { + setPrevCollection(collection) + setObserver(createLiveQueryObserver(collection, { mode: `wholesale` })) } - const observer = observerRef.current! // Stable subscribe bound to the current observer; the observer owns the // subscription, ready-race, and disposal. - const subscribeRef = useRef< - ((onStoreChange: () => void) => () => void) | null - >(null) - if (!subscribeRef.current || needsNewCollection) { - subscribeRef.current = (onStoreChange) => - observer.subscribe(() => onStoreChange()) - } + const subscribe = useCallback( + (onStoreChange: () => void) => observer.subscribe(() => onStoreChange()), + [observer], + ) + + const getSnapshot = useCallback(() => observer.getSnapshot(), [observer]) // The observer returns a stable snapshot per revision, which is the return // shape this hook exposes. Keep the return loose to satisfy the overloads. - return useSyncExternalStore(subscribeRef.current, () => - observer.getSnapshot(), - ) as any + return useSyncExternalStore(subscribe, getSnapshot) as any } diff --git a/packages/react-db/src/useLiveSuspenseQuery.ts b/packages/react-db/src/useLiveSuspenseQuery.ts index 162bf1f3f..73892064e 100644 --- a/packages/react-db/src/useLiveSuspenseQuery.ts +++ b/packages/react-db/src/useLiveSuspenseQuery.ts @@ -1,4 +1,4 @@ -import { useRef } from 'react' +import { useMemo, useState } from 'react' import { useLiveQuery } from './useLiveQuery' import type { Collection, @@ -12,6 +12,8 @@ import type { SingleResult, } from '@tanstack/db' +const safePromiseCache = new WeakMap, Promise>() + /** * Create a live query with React Suspense support * @param queryFn - Query function that defines what data to fetch @@ -156,18 +158,16 @@ export function useLiveSuspenseQuery( configOrQueryOrCollection: any, deps: Array = [], ) { - const promiseRef = useRef | null>(null) - const collectionRef = useRef | null>(null) - const hasBeenReadyRef = useRef(false) - // Use useLiveQuery to handle collection management and reactivity const result = useLiveQuery(configOrQueryOrCollection, deps) + const [prevCollection, setPrevCollection] = useState(result.collection) + const [hasBeenReady, setHasBeenReady] = useState(false) + // Reset promise and ready state when collection changes (deps changed) - if (collectionRef.current !== result.collection) { - promiseRef.current = null - collectionRef.current = result.collection - hasBeenReadyRef.current = false + if (prevCollection !== result.collection) { + setPrevCollection(result.collection) + setHasBeenReady(false) } // SUSPENSE LOGIC: Throw promise or error based on collection status @@ -188,15 +188,13 @@ export function useLiveSuspenseQuery( const collectionStatus = result.collection.status // Track when we reach ready state - if (collectionStatus === `ready`) { - hasBeenReadyRef.current = true - promiseRef.current = null + if (collectionStatus === `ready` && !hasBeenReady) { + setHasBeenReady(true) } // Only throw errors during initial load (before first ready) // After success, errors surface as stale data (matches TanStack Query behavior) - if (collectionStatus === `error` && !hasBeenReadyRef.current) { - promiseRef.current = null + if (collectionStatus === `error` && !hasBeenReady) { // TODO: Once collections hold a reference to their last error object (#671), // we should rethrow that actual error instead of creating a generic message throw new Error(`Collection "${result.collection.id}" failed to load`) @@ -204,20 +202,26 @@ export function useLiveSuspenseQuery( if (collectionStatus === `loading` || collectionStatus === `idle`) { // Create or reuse promise for current collection - if (!promiseRef.current) { - promiseRef.current = result.collection.preload() + const promise = result.collection.preload() + let safePromise = safePromiseCache.get(promise) + if (!safePromise) { + safePromise = promise.catch(() => {}) + safePromiseCache.set(promise, safePromise) } // THROW PROMISE - React Suspense catches this (React 18+ required) // Note: We don't check React version here. In React <18, this will be caught // by an Error Boundary, which provides a reasonable failure mode. - throw promiseRef.current + throw safePromise } // Return data without status/loading flags (handled by Suspense/ErrorBoundary) // If error after success, return last known good state (stale data) - return { - state: result.state, - data: result.data, - collection: result.collection, - } + return useMemo( + () => ({ + state: result.state, + data: result.data, + collection: result.collection, + }), + [result.collection, result.data, result.state], + ) }