-
Notifications
You must be signed in to change notification settings - Fork 97
[POC] useOnyx subscribed #808
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
dd67718
f4bd80c
76830e4
9e6e74e
9b8bcea
52c7e8f
e12bd86
b51368c
13f825e
c034d60
578edad
a284267
db07712
a5f78e5
a513e2a
c1a8c77
e244efc
83b7e66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
LukasMod marked this conversation as resolved.
|
| 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 useLiveRef from './useLiveRef'; | ||
|
|
||
| type UseOnyxSelector<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>> = (data: OnyxValue<TKey> | undefined) => TReturnValue; | ||
|
|
@@ -26,6 +29,13 @@ 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`, keeps the connection open (value stays cache-warm) but stops | ||
| * re-rendering on background writes. It defers the render trigger, not the value: any other render still reads the latest value. | ||
| * Flipping back to `true` re-renders. | ||
| */ | ||
| subscribed?: boolean; | ||
| }; | ||
|
|
||
| type FetchStatus = 'loading' | 'loaded'; | ||
|
|
@@ -45,6 +55,13 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>( | |
| const currentDependenciesRef = useLiveRef(dependencies); | ||
| const selector = options?.selector; | ||
|
|
||
| // Read via a ref inside the Onyx callback so toggling `subscribed` never re-subscribes. | ||
| const subscribed = options?.subscribed !== false; | ||
| const subscribedRef = useRef(subscribed); | ||
| useEffect(() => { | ||
| subscribedRef.current = subscribed; | ||
|
Comment on lines
+64
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a consumer flips Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| }, [subscribed]); | ||
|
LukasMod marked this conversation as resolved.
LukasMod marked this conversation as resolved.
|
||
|
|
||
| // Create memoized version of selector for performance | ||
| const memoizedSelector = useMemo((): UseOnyxSelector<TKey, TReturnValue> | null => { | ||
| if (!selector) { | ||
|
|
@@ -265,8 +282,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, except for paused background writes. The initial load is never paused | ||
| // though, otherwise a cold `subscribed: false` key would stay stuck 'loading' until some render. | ||
| if (subscribedRef.current || resultRef.current?.[1]?.status === 'loading') { | ||
| onStoreChange(); | ||
| } | ||
| }, | ||
| reuseConnection: options?.reuseConnection, | ||
| }); | ||
|
|
||
|
LukasMod marked this conversation as resolved.
|
Uh oh!
There was an error while loading. Please reload this page.