fix: useSelector was always re-rendering even if the returned value d…

…idn't change. This requires running the selector an extra time to check whether its value changed and requires a re-render, but when using useSelector to render a React element it needs to skip the check.

src/react/Computed.tsx

@@ -3,5 +3,5 @@ import type { ObservableReadable } from '../observableInterfaces';
 import { useSelector } from './useSelector';
 
 export function Computed({ children }: { children: ObservableReadable | (() => ReactNode) }): ReactElement {
-    return useSelector(children) as ReactElement;
+    return useSelector(children, { skipCheck: true }) as ReactElement;
 }

src/react/Show.tsx

@@ -22,17 +22,20 @@ type Props<T> = PropsBase<T> & (PropsIf<T> | PropsIfReady<T>);
 
 export function Show<T>(props: Props<T>): ReactElement;
 export function Show<T>({ if: if_, ifReady, else: else_, wrap, children }: Props<T>): ReactElement {
-    const child = useSelector(() => {
-        const value = computeSelector(if_ ?? ifReady);
-        return computeSelector(
-            (ifReady !== undefined ? isObservableValueReady(value) : value)
-                ? isFunction(children)
-                    ? () => children(value)
-                    : children
-                : else_
-                ? else_
-                : null,
-        );
-    });
+    const child = useSelector(
+        () => {
+            const value = computeSelector(if_ ?? ifReady);
+            return computeSelector(
+                (ifReady !== undefined ? isObservableValueReady(value) : value)
+                    ? isFunction(children)
+                        ? () => children(value)
+                        : children
+                    : else_
+                    ? else_
+                    : null,
+            );
+        },
+        { skipCheck: true },
+    );
     return wrap ? createElement(wrap, undefined, child) : child;
 }

src/react/Switch.tsx

@@ -31,7 +31,7 @@ export function Switch<T>({
     children: Partial<Record<any, () => ReactNode>>;
 }): ReactElement {
     // Select from an object of cases
-    return ((children as Record<any, () => ReactNode>)[useSelector(value)!]?.() ??
+    return ((children as Record<any, () => ReactNode>)[useSelector(value, { skipCheck: true })!]?.() ??
         (children as Record<any, () => ReactNode>)['default']?.() ??
         null) as ReactElement;
 }

src/react/reactInterfaces.ts

@@ -27,4 +27,5 @@ export type FCReactive<P, P2> = P &
 
 export interface UseSelectorOptions {
     suspense?: boolean;
+    skipCheck?: boolean;
 }

src/react/reactive-observer.tsx

@@ -68,7 +68,7 @@ function createReactiveComponent<P = object>(
 
                     // Convert children if it's a function
                     if (key === 'children' && (isFunction(p) || isObservable(p))) {
-                        props[key] = useSelector(p);
+                        props[key] = useSelector(p, { skipCheck: true });
                     }
                     // Convert reactive props
                     else if (key.startsWith('$') || key.endsWith('$')) {
@@ -126,14 +126,17 @@ function createReactiveComponent<P = object>(
 
             // If observing wrap the whole render in a useSelector to listen to it
             if (observe) {
-                return useSelector(() => {
-                    reactGlobals.inObserver = true;
-                    try {
-                        return Reflect.apply(target, thisArg, argArray);
-                    } finally {
-                        reactGlobals.inObserver = false;
-                    }
-                });
+                return useSelector(
+                    () => {
+                        reactGlobals.inObserver = true;
+                        try {
+                            return Reflect.apply(target, thisArg, argArray);
+                        } finally {
+                            reactGlobals.inObserver = false;
+                        }
+                    },
+                    { skipCheck: true },
+                );
             } else {
                 return Reflect.apply(target, thisArg, argArray);
             }

src/react/useSelector.ts

@@ -1,24 +1,40 @@
-import { computeSelector, isPromise, Selector, trackSelector } from '@legendapp/state';
+import { computeSelector, isPrimitive, isPromise, ListenerParams, Selector, trackSelector } from '@legendapp/state';
 import React, { useRef } from 'react';
+import type { UseSelectorOptions } from './reactInterfaces';
 import { useSyncExternalStore } from 'use-sync-external-store/shim';
 import { reactGlobals } from './react-globals';
-import type { UseSelectorOptions } from './reactInterfaces';
 
 interface SelectorFunctions<T> {
     subscribe: (onStoreChange: () => void) => () => void;
     getVersion: () => number;
     run: (selector: Selector<T>) => T;
 }
 
-function createSelectorFunctions<T>(): SelectorFunctions<T> {
+function createSelectorFunctions<T>(options: UseSelectorOptions | undefined): SelectorFunctions<T> {
     let version = 0;
     let notify: () => void;
     let dispose: (() => void) | undefined;
     let resubscribe: (() => void) | undefined;
+    let _selector: Selector<T>;
+    let prev: T;
 
-    const _update = () => {
-        version++;
-        notify?.();
+    const _update = ({ value }: ListenerParams) => {
+        // If skipCheck then don't need to re-run selector
+        let changed = options?.skipCheck;
+        if (!changed) {
+            // Re-run the selector to get the new value
+            const newValue = computeSelector(_selector);
+            // If newValue is different than previous value then it's changed.
+            // Also if the selector returns an observable directly then its value will be the same as
+            // the value from the listener, and that should always re-render.
+            if (newValue !== prev || (!isPrimitive(newValue) && newValue === value)) {
+                changed = true;
+            }
+        }
+        if (changed) {
+            version++;
+            notify?.();
+        }
     };
 
     return {
@@ -41,6 +57,8 @@ function createSelectorFunctions<T>(): SelectorFunctions<T> {
         },
         getVersion: () => version,
         run: (selector: Selector<T>) => {
+            // Update the cached selector
+            _selector = selector;
             // Dispose if already listening
             dispose?.();
 
@@ -53,6 +71,8 @@ function createSelectorFunctions<T>(): SelectorFunctions<T> {
             dispose = _dispose;
             resubscribe = _resubscribe;
 
+            prev = value;
+
             return value;
         },
     };
@@ -66,10 +86,13 @@ export function useSelector<T>(selector: Selector<T>, options?: UseSelectorOptio
 
     const ref = useRef<SelectorFunctions<T>>();
     if (!ref.current) {
-        ref.current = createSelectorFunctions<T>();
+        ref.current = createSelectorFunctions<T>(options);
     }
     const { subscribe, getVersion, run } = ref.current;
 
+    // Run the selector
+    // Note: The selector needs to run on every render because it may have different results
+    // than the previous run if it uses local state
     const value = run(selector) as any;
 
     useSyncExternalStore(subscribe, getVersion, getVersion);

src/trackSelector.ts

@@ -1,5 +1,6 @@
 import { computeSelector, isObservable } from './helpers';
 import type {
+    ListenerParams,
     NodeValue,
     ObservableListenerDispose,
     ObserveEvent,
@@ -12,7 +13,7 @@ import { beginTracking, endTracking, tracking } from './tracking';
 
 export function trackSelector<T>(
     selector: Selector<T>,
-    update: () => void,
+    update: (params: ListenerParams) => void,
     observeEvent?: ObserveEvent<T>,
     observeOptions?: ObserveOptions,
     createResubscribe?: boolean,
@@ -23,12 +24,11 @@ export function trackSelector<T>(
     let tracker;
     let resubscribe: ObservableListenerDispose | undefined;
     let updateFn = update;
-    let noArgs = true;
 
     if (isObservable(selector)) {
         value = selector.peek();
-        dispose = selector.onChange(update, { noArgs: true });
-        resubscribe = createResubscribe ? selector.onChange(update, { noArgs: true }) : undefined;
+        dispose = selector.onChange(update);
+        resubscribe = createResubscribe ? selector.onChange(update) : undefined;
     } else {
         // Compute the selector inside a tracking context
         beginTracking();
@@ -40,7 +40,6 @@ export function trackSelector<T>(
         if ((process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test') && tracker && nodes) {
             tracker.traceListeners?.(nodes);
             if (tracker.traceUpdates) {
-                noArgs = false;
                 updateFn = tracker.traceUpdates(update) as () => void;
             }
             // Clear tracing so it doesn't leak to other components
@@ -54,8 +53,8 @@ export function trackSelector<T>(
 
         // useSyncExternalStore doesn't subscribe until after the component mount.
         // We want to subscribe immediately so we don't miss any updates
-        dispose = setupTracking(nodes, updateFn, noArgs, observeOptions?.immediate);
-        resubscribe = createResubscribe ? () => setupTracking(nodes, updateFn, noArgs) : undefined;
+        dispose = setupTracking(nodes, updateFn, false, observeOptions?.immediate);
+        resubscribe = createResubscribe ? () => setupTracking(nodes, updateFn) : undefined;
     }
 
     return { value, dispose, resubscribe };

tests/react.test.ts

@@ -33,12 +33,12 @@ describe('useSelector', () => {
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(3);
         expect(result.current).toEqual('hello there');
         act(() => {
             obs.set('z');
         });
-        expect(num).toEqual(3);
+        expect(num).toEqual(5);
         expect(result.current).toEqual('z there');
     });
     test('useSelector with observable', () => {
@@ -79,12 +79,12 @@ describe('useSelector', () => {
             obs.set('hello');
             obs.set('hello2');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(4); // Once for each set plus the render
         expect(result.current).toEqual('hello2 there');
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(3);
+        expect(num).toEqual(6); // Once for set plus render
         expect(result.current).toEqual('hello there');
     });
     test('useSelector two observables', () => {
@@ -106,17 +106,17 @@ describe('useSelector', () => {
             obs2.set('bb');
             obs2.set('b');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(6);
         expect(result.current).toEqual('a b there');
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(3);
+        expect(num).toEqual(8);
         expect(result.current).toEqual('hello b there');
         act(() => {
             obs2.set('z');
         });
-        expect(num).toEqual(4);
+        expect(num).toEqual(10);
         expect(result.current).toEqual('hello z there');
     });
     test('useSelector cleaned up', () => {
@@ -176,7 +176,7 @@ describe('useSelector', () => {
         });
 
         expect(num).toEqual(3);
-        expect(numSelects).toEqual(3);
+        expect(numSelects).toEqual(6);
 
         act(() => {
             fr();
@@ -189,7 +189,7 @@ describe('useSelector', () => {
         });
 
         expect(num).toEqual(5);
-        expect(numSelects).toEqual(5);
+        expect(numSelects).toEqual(11);
     });
     test('useSelector runs twice in strict mode', () => {
         const obs = observable('hi');
@@ -211,7 +211,7 @@ describe('useSelector', () => {
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(4);
+        expect(num).toEqual(6);
     });
     test('Renders once with one selector listening to multiple', () => {
         const obs = observable('hi');
@@ -232,7 +232,7 @@ describe('useSelector', () => {
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(3);
     });
     test('Renders once for each selector', () => {
         const obs = observable('hi');
@@ -262,7 +262,7 @@ describe('useSelector', () => {
             obs.set('hello');
         });
         // Goes up by two because it runs, decides to re-render, and runs again
-        expect(num).toEqual(6);
+        expect(num).toEqual(7);
     });
     test('useSelector renders once when set to the same thing', () => {
         const obs = observable('hi');
@@ -278,11 +278,45 @@ describe('useSelector', () => {
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(3);
         act(() => {
             obs.set('hello');
         });
-        expect(num).toEqual(2);
+        expect(num).toEqual(3); // Doesn't re-run the selector so it's not different
+    });
+    test('useSelector renders once when it returns the same thing', () => {
+        const obs = observable('hi');
+        let num = 0;
+        let num2 = 0;
+        let lastValue = false;
+
+        const Test = function Test() {
+            num2++;
+            lastValue = useSelector(() => {
+                num++;
+                return obs.get() === 'hi';
+            });
+
+            return createElement('div', undefined);
+        };
+        function App() {
+            return createElement(Test);
+        }
+        render(createElement(App));
+
+        expect(lastValue).toEqual(true);
+        expect(num).toEqual(1);
+        expect(num2).toEqual(1);
+        act(() => {
+            obs.set('hello');
+        });
+        expect(num).toEqual(3);
+        expect(num2).toEqual(2);
+        act(() => {
+            obs.set('hello2');
+        });
+        expect(num).toEqual(4);
+        expect(num2).toEqual(2);
     });
 });