diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b28f21109..7203314512 100644 --- a/CHANGELOG.md +++ b/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 store. (#1417) diff --git a/packages/recoil-sync/RecoilSync.js b/packages/recoil-sync/RecoilSync.js index 364ddf76d2..6df8be5a36 100644 --- a/packages/recoil-sync/RecoilSync.js +++ b/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}); diff --git a/packages/recoil-sync/RecoilSync_URL.js b/packages/recoil-sync/RecoilSync_URL.js index 479207bc2a..fc00219f05 100644 --- a/packages/recoil-sync/RecoilSync_URL.js +++ b/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(null); + // Avoid executing updateCachedState() on each render + const firstRender = useRef(true); firstRender.current && updateCachedState(); firstRender.current = false; useEffect(updateCachedState, [updateCachedState]); diff --git a/packages/recoil/core/Recoil_FunctionalCore.js b/packages/recoil/core/Recoil_FunctionalCore.js index 4929cf9d35..b47f51164d 100644 --- a/packages/recoil/core/Recoil_FunctionalCore.js +++ b/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, }; diff --git a/packages/recoil/core/Recoil_RecoilRoot.react.js b/packages/recoil/core/Recoil_RecoilRoot.react.js index 50b58dd1b1..8e904da60b 100644 --- a/packages/recoil/core/Recoil_RecoilRoot.react.js +++ b/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 is unmounted - useEffect( - () => () => { - for (const atomKey of storeRef.current.getState().knownAtoms) { + const cleanedUpNodesRef = useRefInitOnce>>( + () => 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 ( diff --git a/packages/recoil/core/Recoil_Snapshot.js b/packages/recoil/core/Recoil_Snapshot.js index c68aefc029..13e6188306 100644 --- a/packages/recoil/core/Recoil_Snapshot.js +++ b/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); // } } diff --git a/packages/recoil/hooks/Recoil_Hooks.js b/packages/recoil/hooks/Recoil_Hooks.js index 530fa3b27b..bc7b832e87 100644 --- a/packages/recoil/hooks/Recoil_Hooks.js +++ b/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(); diff --git a/packages/recoil/recoil_values/Recoil_atom.js b/packages/recoil/recoil_values/Recoil_atom.js index fd164fc84f..062cd783f3 100644 --- a/packages/recoil/recoil_values/Recoil_atom.js +++ b/packages/recoil/recoil_values/Recoil_atom.js @@ -435,7 +435,7 @@ function baseAtom(options: BaseAtomOptions): RecoilState { liveStoresCount--; cleanupEffectsByStore.get(store)?.forEach(cleanup => cleanup()); cleanupEffectsByStore.delete(store); - store.getState().knownAtoms.delete(key); // FIXME remove knownAtoms? + store.getState().knownAtoms.delete(key); }; }