Fix React strict mode (facebookexperimental#1444)

Summary:
Pull Request resolved: facebookexperimental#1444

Fix Recoil for React strict mode which runs effects multiple times.

* Now atoms are re-initialized after the effect cleanup handler has cleaned them up.
* The Batcher will reenable the notification mechanism after it was disabled on unmounting to avoid late async updates from trying to re-render.
* Audit all other useEffect() calls except for `useRecoilInterface()` and dev tools `<Connector>`

Inspired from D24722151

Differential Revision: https://www.internalfb.com/diff/D32572832?entry_point=27

fbshipit-source-id: 930c0eb079d85bcd90863d0526445d1d1af0255f

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## UPCOMING
 ***Add new changes here as they land***
 
+- Fix Recoil with React strict mode that re-executes effects multiple times.
 - Add `refresh` to Recoil callback interface (#1413)
 - Fix transitive selector refresh for some cases (#1409)
 - `useRecoilStoreID()` hook to get an ID for the current <RecoilRoot> store. (#1417)

packages/recoil-sync/RecoilSync.js

@@ -174,12 +174,8 @@ function useRecoilSync({
   const snapshot = useRecoilSnapshot();
   const previousSnapshotRef = useRef(snapshot);
   useEffect(() => {
-    if (write != null) {
-      if (snapshot === previousSnapshotRef.current) {
-        return;
-      } else {
-        previousSnapshotRef.current = snapshot;
-      }
+    if (write != null && snapshot !== previousSnapshotRef.current) {
+      previousSnapshotRef.current = snapshot;
       const diff: ItemDiff = new Map();
       const atomRegistry = registries.getAtomRegistry(recoilStoreID, storeKey);
       const modifiedAtoms = snapshot.getNodes_UNSTABLE({isModified: true});

packages/recoil-sync/RecoilSync_URL.js

@@ -179,8 +179,9 @@ function useRecoilURLSync({
   const updateCachedState: () => void = useCallback(() => {
     cachedState.current = parseURL(getURL(), memoizedLoc, deserialize);
   }, [getURL, memoizedLoc, deserialize]);
-  const firstRender = useRef(true); // Avoid executing parseCurrentState() on each render
   const cachedState = useRef<?ItemSnapshot>(null);
+  // Avoid executing updateCachedState() on each render
+  const firstRender = useRef(true);
   firstRender.current && updateCachedState();
   firstRender.current = false;
   useEffect(updateCachedState, [updateCachedState]);

packages/recoil/core/Recoil_FunctionalCore.js

@@ -58,15 +58,13 @@ function initializeRetentionForNode(
     if (!gkx('recoil_memory_managament_2020')) {
       return;
     }
-    const nodesRetainedByZone = store.getState().retention.nodesRetainedByZone;
+    const {retention} = store.getState();
 
     function deleteFromZone(zone: RetentionZone) {
-      const set = nodesRetainedByZone.get(zone);
-      if (set) {
-        set.delete(nodeKey);
-      }
+      const set = retention.nodesRetainedByZone.get(zone);
+      set?.delete(nodeKey);
       if (set && set.size === 0) {
-        nodesRetainedByZone.delete(zone);
+        retention.nodesRetainedByZone.delete(zone);
       }
     }
 
@@ -103,6 +101,10 @@ function initializeNodeIfNewToStore(
   });
 }
 
+function initNode(store: Store, key: NodeKey): void {
+  initializeNodeIfNewToStore(store, store.getState().currentTree, key, 'get');
+}
+
 function cleanUpNode(store: Store, key: NodeKey) {
   const state = store.getState();
   state.nodeCleanupFunctions.get(key)?.();
@@ -249,9 +251,9 @@ module.exports = {
   getNodeLoadable,
   peekNodeLoadable,
   setNodeValue,
+  initNode,
   cleanUpNode,
   setUnvalidatedAtomValue_DEPRECATED,
   peekNodeInfo,
   getDownstreamNodes,
-  initializeNodeIfNewToStore,
 };

packages/recoil/core/Recoil_RecoilRoot.react.js

@@ -10,7 +10,7 @@
  */
 'use strict';
 
-import type {StoreID} from './Recoil_Keys';
+import type {NodeKey, StoreID} from './Recoil_Keys';
 import type {RecoilValue} from './Recoil_RecoilValue';
 import type {MutableSnapshot} from './Recoil_Snapshot';
 import type {Store, StoreRef, StoreState, TreeState} from './Recoil_State';
@@ -27,6 +27,7 @@ const {
 const {
   cleanUpNode,
   getDownstreamNodes,
+  initNode,
   setNodeValue,
   setUnvalidatedAtomValue_DEPRECATED,
 } = require('./Recoil_FunctionalCore');
@@ -251,8 +252,19 @@ function Batcher({
 }) {
   const storeRef = useStoreRef();
 
-  const [_, setState] = useState([]);
+  const [, setState] = useState([]);
   setNotifyBatcherOfChange(() => setState({}));
+  useEffect(() => {
+    setNotifyBatcherOfChange(() => setState({}));
+
+    // If an asynchronous selector resolves after the Batcher is unmounted,
+    // notifyBatcherOfChange will still be called. An error gets thrown whenever
+    // setState is called after a component is already unmounted, so this sets
+    // notifyBatcherOfChange to be a no-op.
+    return () => {
+      setNotifyBatcherOfChange(() => {});
+    };
+  }, [setNotifyBatcherOfChange]);
 
   useEffect(() => {
     // enqueueExecution runs this function immediately; it is only used to
@@ -263,16 +275,6 @@ function Batcher({
     });
   });
 
-  // If an asynchronous selector resolves after the Batcher is unmounted,
-  // notifyBatcherOfChange will still be called. An error gets thrown whenever
-  // setState is called after a component is already unmounted, so this sets
-  // notifyBatcherOfChange to be a no-op.
-  useEffect(() => {
-    return () => {
-      setNotifyBatcherOfChange(() => {});
-    };
-  }, [setNotifyBatcherOfChange]);
-
   return null;
 }
 
@@ -481,14 +483,29 @@ function RecoilRoot_INTERNAL({
   );
 
   // Cleanup when the <RecoilRoot> is unmounted
-  useEffect(
-    () => () => {
-      for (const atomKey of storeRef.current.getState().knownAtoms) {
+  const cleanedUpNodesRef = useRefInitOnce<Map<StoreID, Set<NodeKey>>>(
+    () => new Map(),
+  );
+  useEffect(() => {
+    // React is free to call effect cleanup handlers and effects at will, the
+    // deps array is only an optimization.  For example, React strict mode
+    // will execute each effect twice for testing.  Therefore, we need symmetry
+    // to re-initialize all known atoms after they were cleaned up.
+    const store = storeRef.current;
+    for (const atomKey of cleanedUpNodesRef.current.get(store.storeID) ?? []) {
+      initNode(store, atomKey);
+    }
+    cleanedUpNodesRef.current.delete(store.storeID);
+
+    return () => {
+      const {knownAtoms} = store.getState();
+      // eslint-disable-next-line fb-www/react-hooks-deps
+      cleanedUpNodesRef.current.set(store.storeID, new Set(knownAtoms));
+      for (const atomKey of knownAtoms) {
         cleanUpNode(storeRef.current, atomKey);
       }
-    },
-    [],
-  );
+    };
+  }, [storeRef, cleanedUpNodesRef]);
 
   return (
     <AppContext.Provider value={storeRef}>

packages/recoil/core/Recoil_Snapshot.js

@@ -22,10 +22,7 @@ import type {RecoilState, RecoilValue} from './Recoil_RecoilValue';
 import type {StateID, Store, StoreState, TreeState} from './Recoil_State';
 
 const {batchUpdates} = require('./Recoil_Batching');
-const {
-  initializeNodeIfNewToStore,
-  peekNodeInfo,
-} = require('./Recoil_FunctionalCore');
+const {initNode, peekNodeInfo} = require('./Recoil_FunctionalCore');
 const {graph} = require('./Recoil_Graph');
 const {getNextStoreID} = require('./Recoil_Keys');
 const {
@@ -98,13 +95,8 @@ class Snapshot {
     };
     // Initialize any nodes that are live in the parent store (primarily so that this
     // snapshot gets counted towards the node's live stores count).
-    for (const nodeKey of this._store.getState().nodeCleanupFunctions.keys()) {
-      initializeNodeIfNewToStore(
-        this._store,
-        storeState.currentTree,
-        nodeKey,
-        'get',
-      );
+    for (const nodeKey of this._store.getState().knownAtoms) {
+      initNode(this._store, nodeKey);
       updateRetainCount(this._store, nodeKey, 1);
     }
     this.retain();
@@ -142,7 +134,7 @@ class Snapshot {
     if (this._refCount === 0) {
       // Temporarily nerfing this to allow us to find broken call sites without
       // actually breaking anybody yet.
-      // for (const k of this._store.getState().nodeCleanupFunctions.keys()) {
+      // for (const k of this._store.getState().knownAtoms) {
       //   updateRetainCountToZero(this._store, k);
       // }
     }

packages/recoil/hooks/Recoil_Hooks.js

@@ -93,8 +93,8 @@ export type RecoilInterface = {
 };
 
 /**
- * Various things are broken with useRecoilInterface, particularly concurrent mode
- * and memory management. They will not be fixed.
+ * Various things are broken with useRecoilInterface, particularly concurrent
+ * mode, React strict mode, and memory management. They will not be fixed.
  * */
 function useRecoilInterface_DEPRECATED(): RecoilInterface {
   const storeRef = useStoreRef();

packages/recoil/recoil_values/Recoil_atom.js

@@ -435,7 +435,7 @@ function baseAtom<T>(options: BaseAtomOptions<T>): RecoilState<T> {
       liveStoresCount--;
       cleanupEffectsByStore.get(store)?.forEach(cleanup => cleanup());
       cleanupEffectsByStore.delete(store);
-      store.getState().knownAtoms.delete(key); // FIXME remove knownAtoms?
+      store.getState().knownAtoms.delete(key);
     };
   }