Fix race condition in useReactiveVar (#10072)

Co-authored-by: Ben Newman <[email protected]>
Co-authored-by: Jerel Miller <[email protected]>

.changeset/angry-llamas-walk.md

@@ -0,0 +1,5 @@
+---
+"@apollo/client": patch
+---
+
+Fixes race conditions in useReactiveVar that may prevent updates to the reactive variable from propagating through the hook.

src/react/hooks/__tests__/useReactiveVar.test.tsx

@@ -323,5 +323,59 @@ describe("useReactiveVar Hook", () => {
         resolve();
       }
     );
+
+    itAsync(
+      "should survive many rerenderings despite racing asynchronous updates",
+      (resolve, reject) => {
+        const rv = makeVar(0);
+
+        function App() {
+          const value = useReactiveVar(rv);
+          return (
+            <div className="App">
+              <h1>{value}</h1>
+            </div>
+          );
+        }
+
+        const goalCount = 1000;
+        let updateCount = 0;
+        let stopped = false;
+
+        function spam() {
+          if (stopped) return;
+          try {
+            if (++updateCount <= goalCount) {
+              act(() => {
+                rv(updateCount);
+                setTimeout(spam, Math.random() * 10);
+              });
+            } else {
+              stopped = true;
+              expect(rv()).toBe(goalCount);
+              screen
+                .findByText(String(goalCount))
+                .then((element) => {
+                  expect(element.nodeName.toLowerCase()).toBe("h1");
+                })
+                .then(resolve, reject);
+            }
+          } catch (e) {
+            stopped = true;
+            reject(e);
+          }
+        }
+        spam();
+        spam();
+        spam();
+        spam();
+
+        render(
+          <StrictMode>
+            <App />
+          </StrictMode>
+        );
+      }
+    );
   });
 });

src/react/hooks/useReactiveVar.ts

@@ -1,26 +1,24 @@
 import * as React from "react";
 import type { ReactiveVar } from "../../core/index.js";
+import { useSyncExternalStore } from "./useSyncExternalStore.js";
 
 export function useReactiveVar<T>(rv: ReactiveVar<T>): T {
-  const value = rv();
-
-  // We don't actually care what useState thinks the value of the variable
-  // is, so we take only the update function from the returned array.
-  const setValue = React.useState(value)[1];
-
-  // We subscribe to variable updates on initial mount and when the value has
-  // changed. This avoids a subtle bug in React.StrictMode where multiple
-  // listeners are added, leading to inconsistent updates.
-  React.useEffect(() => {
-    const probablySameValue = rv();
-    if (value !== probablySameValue) {
-      // If the value of rv has already changed, we don't need to listen for the
-      // next change, because we can report this change immediately.
-      setValue(probablySameValue);
-    } else {
-      return rv.onNextChange(setValue);
-    }
-  }, [value]);
-
-  return value;
+  return useSyncExternalStore(
+    React.useCallback(
+      (update) => {
+        // By reusing the same onNext function in the nested call to
+        // rv.onNextChange(onNext), we can keep using the initial clean-up function
+        // returned by rv.onNextChange(function onNext(v){...}), without having to
+        // register the new clean-up function (returned by the nested
+        // rv.onNextChange(onNext)) with yet another callback.
+        return rv.onNextChange(function onNext() {
+          update();
+          rv.onNextChange(onNext);
+        });
+      },
+      [rv]
+    ),
+    rv,
+    rv
+  );
 }