Merge pull request #735 from callstack-internal/fix/selector-on-dependency-list

Fix/Ensure the new selector execution

Author: mountiny

lib/useOnyx.ts

@@ -73,7 +73,7 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
     const selector = options?.selector;
 
     // Create memoized version of selector for performance
-    const memoizedSelector = useMemo(() => {
+    const memoizedSelector = useMemo((): UseOnyxSelector<TKey, TReturnValue> | null => {
         if (!selector) {
             return null;
         }
@@ -220,6 +220,11 @@ function useOnyx<TKey extends OnyxKey, TReturnValue = OnyxValue<TKey>>(
         }
     }, [key, options?.canEvict]);
 
+    // Tracks the last memoizedSelector reference that getSnapshot() has computed with.
+    // When the selector changes, this mismatch forces getSnapshot() to re-evaluate
+    // even if all other conditions (isFirstConnection, shouldGetCachedValue, key) are false.
+    const lastComputedSelectorRef = useRef(memoizedSelector);
+
     const getSnapshot = useCallback(() => {
         // Check if we have any cache for this Onyx key
         // Don't use cache for first connection with initWithStoredValues: false
@@ -244,10 +249,12 @@ 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.
-        if (isFirstConnectionRef.current || shouldGetCachedValueRef.current || key !== previousKey) {
+        const hasSelectorChanged = lastComputedSelectorRef.current !== memoizedSelector;
+        if (isFirstConnectionRef.current || shouldGetCachedValueRef.current || key !== previousKey || 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>;
             const selectedValue = memoizedSelector ? memoizedSelector(value) : value;
+            lastComputedSelectorRef.current = memoizedSelector;
             newValueRef.current = (selectedValue ?? undefined) as TReturnValue | undefined;
 
             // We set this flag to `false` again since we don't want to get the newest cached value every time `getSnapshot()` is executed,

tests/unit/useOnyxTest.ts

@@ -429,7 +429,10 @@ describe('useOnyx', () => {
         it('should always use the current selector reference to return new data', async () => {
             Onyx.set(ONYXKEYS.TEST_KEY, {id: 'test_id', name: 'test_name'});
 
-            let selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name}`) as UseOnyxSelector<OnyxKey, string>;
+            let selector: UseOnyxSelector<OnyxKey, string> = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name}`) as UseOnyxSelector<
+                OnyxKey,
+                string
+            >;
 
             const {result, rerender} = renderHook(() =>
                 useOnyx(ONYXKEYS.TEST_KEY, {
@@ -440,10 +443,22 @@ describe('useOnyx', () => {
             expect(result.current[0]).toEqual('id - test_id, name - test_name');
             expect(result.current[1].status).toEqual('loaded');
 
-            selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed`) as UseOnyxSelector<OnyxKey, string>;
+            selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed synchronously`) as UseOnyxSelector<OnyxKey, string>;
+
             rerender(undefined);
 
-            expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed');
+            expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed synchronously');
+            expect(result.current[1].status).toEqual('loaded');
+
+            selector = ((entry: OnyxEntry<{id: string; name: string}>) => `id - ${entry?.id}, name - ${entry?.name} - selector changed after macrotask`) as UseOnyxSelector<OnyxKey, string>;
+
+            await act(async () => {
+                // This is necessary to avoid regressions of the selector interleaving bug, see https://github.com/Expensify/App/issues/79449
+                await waitForPromisesToResolve();
+                rerender(undefined);
+            });
+
+            expect(result.current[0]).toEqual('id - test_id, name - test_name - selector changed after macrotask');
             expect(result.current[1].status).toEqual('loaded');
         });
 
@@ -684,15 +699,16 @@ describe('useOnyx', () => {
 
             const dependencies = ['constant'];
             let selectorCallCount = 0;
+            const selector = ((data) => {
+                selectorCallCount++;
+                return `${dependencies.join(',')}:${(data as {value?: string})?.value}`;
+            }) as UseOnyxSelector<OnyxKey, string>;
 
             const {result, rerender} = renderHook(() =>
                 useOnyx(
                     ONYXKEYS.TEST_KEY,
                     {
-                        selector: (data) => {
-                            selectorCallCount++;
-                            return `${dependencies.join(',')}:${(data as {value?: string})?.value}`;
-                        },
+                        selector,
                     },
                     dependencies,
                 ),