Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 49 additions & 7 deletions lib/useOnyx.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import type {DependencyList} from 'react';

import {deepEqual, shallowEqual} from 'fast-equals';
import {useCallback, useEffect, useMemo, useRef, useSyncExternalStore} from 'react';
import type {DependencyList} from 'react';
import OnyxCache, {TASK} from './OnyxCache';

import type {Connection} from './OnyxConnectionManager';
import connectionManager from './OnyxConnectionManager';
import OnyxUtils from './OnyxUtils';
import type {CollectionKeyBase, OnyxKey, OnyxValue} from './types';

import OnyxCache, {TASK} from './OnyxCache';
import connectionManager from './OnyxConnectionManager';
import onyxSnapshotCache from './OnyxSnapshotCache';
import OnyxUtils from './OnyxUtils';
import memoizedShallowEqual from './memoizedShallowEqual';
import useLiveRef from './useLiveRef';

Expand All @@ -27,6 +30,14 @@ type UseOnyxOptions<TKey extends OnyxKey, TReturnValue> = {
* @see `useOnyx` cannot return `null` and so selector will replace `null` with `undefined` to maintain compatibility.
*/
selector?: UseOnyxSelector<TKey, TReturnValue>;

/**
* Defaults to `true`. When `false`, the connection stays open (the value stays cache-warm) but background
* writes no longer trigger a re-render, and the value is frozen: incidental re-renders keep returning the
* last delivered result instead of the latest one. The initial load is exempt, so a cold key still resolves.
* Flipping back to `true` re-renders and catches up to the latest value.
*/
subscribed?: boolean;
};

type FetchStatus = 'loading' | 'loaded';
Expand All @@ -46,6 +57,21 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
const currentDependenciesRef = useLiveRef(dependencies);
const selector = options?.selector;

// The Onyx callback reads `subscribed` via a ref so toggling it never re-subscribes. The ref is synced in an
// effect so the gate only ever reflects committed renders.
const subscribed = options?.subscribed !== false;
const subscribedRef = useRef(subscribed);
useEffect(() => {
subscribedRef.current = subscribed;
Comment on lines +64 to +65

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Sync subscribed ref before passive effects

When a consumer flips subscribed from true to false (for example during a screen-blur render), this passive effect leaves subscribedRef.current at the old true value until after commit. Any Onyx write from a sibling layout effect or immediate microtask in that commit→effect window will pass the guard in the connection callback and call onStoreChange(), causing the off-screen/background re-render this option is meant to suppress. Syncing the ref during render or in a layout effect avoids that stale-true window.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that there's a small gap. When subscribed flips from true to false, the ref only updates after React runs effects. If an Onyx write lands in that tiny window between the render committing and the effect running, the callback still sees true and triggers one re-render that ideally would have been skipped.

We're keeping it this way on purpose, for three reasons:

  1. First, the damage is tiny and harmless. Worst case is one extra render, showing correct data, on a screen that just went off-screen. Compare that to the opposite direction (false to true): if the ref were stale there, a visible screen would miss fresh data, which is a real correctness bug. That direction is already handled by the catch-up in this effect. So the rule of thumb here is: when the gate is briefly wrong, it should fail by rendering too much, never by showing stale data. This window fails in the safe direction.

  2. Second, the suggested fixes are worse than the problem. We can't "catch up" after the fact like we did for the focus direction, because you can't undo a render that already happened. The only real fix is updating the ref earlier. Updating it during render was already flagged as a P2 in an earlier round: with concurrent rendering, an aborted render could write false into the ref while the visible screen is still subscribed, and then a visible screen stops getting updates. useLayoutEffect would work but brings the SSR warning problem and would be the first use of that pattern in this library. Neither trade is worth it to skip one background render in case of that tiny gap.

  3. Third, TanStack Query has exactly the same window in its subscribed option and ships with it unmitigated. Their subscription is only torn down when React runs the passive effect, so a query update in that same commit-to-effect gap also re-renders the component that just unsubscribed. This isn't an implementation bug on our side, it's just how useSyncExternalStore works: subscription changes always take effect when effects flush, not when the render commits.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it's not worth investing on this, bringing more unecessary complexity is exactly what we want to avoid in the hook


// A write can land between the flip render (`false` to `true`) and this effect, while the stale ref still
// gates it. The gated callback leaves `shouldGetCachedValueRef` set, so deliver it now. No-op otherwise:
// `getSnapshot()` returns the same cached reference and `useSyncExternalStore` bails out.
if (subscribed && shouldGetCachedValueRef.current) {
onStoreChangeFnRef.current?.();
}
}, [subscribed]);
Comment thread
LukasMod marked this conversation as resolved.

// Create memoized version of selector for performance
const memoizedSelector = useMemo((): UseOnyxSelector<TKey, TReturnValue> | null => {
if (!selector) {
Expand Down Expand Up @@ -161,6 +187,20 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
const lastComputedSelectorRef = useRef(memoizedSelector);

const getSnapshot = useCallback(() => {
// While unsubscribed, keep returning the last delivered result so incidental re-renders don't leak fresh data past the gate. Pending writes leave `shouldGetCachedValueRef`
// set, and the `subscribed` flip effect delivers the fresh value once the consumer resubscribes.
// First connection and `loading` are exempt so a cold `subscribed: false` key still resolves.
const hasSelectorChanged = lastComputedSelectorRef.current !== memoizedSelector;

if (!subscribedRef.current && connectedKeyRef.current === key && resultRef.current[1].status !== 'loading') {
// A selector-identity change is a consumer-driven dirtying signal with no Onyx write behind it,
// so mark the snapshot dirty; otherwise the catch-up effect can't detect it on resubscribe.
if (hasSelectorChanged) {
shouldGetCachedValueRef.current = true;
}
return resultRef.current;
Comment on lines +195 to +201

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Recompute selector changes when resubscribing

When a consumer is paused and its selector prop changes while subscribed is false, this early return runs before hasSelectorChanged can be evaluated. If no Onyx write or dependency invalidation happened, shouldGetCachedValueRef remains false, so flipping subscribed back to true does not call onStoreChange() and the hook can keep returning the old selector output until some later store update or parent render. Let selector changes bypass the paused fast path, or mark the snapshot dirty, so resubscribe actually catches up.

Useful? React with 👍 / 👎.

}

// Check if we have any cache for this Onyx key
// Don't use cache during active data updates (when shouldGetCachedValueRef is true)
const isFirstConnection = connectedKeyRef.current !== key;
Expand All @@ -175,7 +215,6 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
// We get the value from cache while the first connection to Onyx is being made or if the key has changed,
// so we can return any cached value right away. For the case where the key has changed, If we don't return the cached value right away, then the UI will show the incorrect (previous) value for a brief period which looks like a UI glitch to the user. After the connection is made, we only
// update `newValueRef` when `Onyx.connect()` callback is fired.
const hasSelectorChanged = lastComputedSelectorRef.current !== memoizedSelector;
if (isFirstConnection || shouldGetCachedValueRef.current || hasSelectorChanged) {
// Gets the value from cache and maps it with selector. It changes `null` to `undefined` for `useOnyx` compatibility.
const value = OnyxUtils.tryGetCachedValue(key) as OnyxValue<TKey>;
Expand Down Expand Up @@ -268,8 +307,11 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
// Invalidate snapshot cache for this key when data changes
onyxSnapshotCache.invalidateForKey(key);

// Finally, we signal that the store changed, making `getSnapshot()` be called again.
onStoreChange();
// Trigger a re-render unless paused. The initial load is never paused — gating it would leave
// a cold `subscribed: false` key stuck at 'loading' until some unrelated render.
if (subscribedRef.current || resultRef.current?.[1]?.status === 'loading') {
onStoreChange();
}
},
reuseConnection: options?.reuseConnection,
});
Expand Down
Loading
Loading