Release snapshots (#1006)

Summary: Let the Dev Tools retain snapshots that it's using. Pull Request resolved: facebookexperimental/Recoil#1006 Test Plan: Use React Playground and verify that: * The warning about unretained snapshots is eliminated. * Retained snapshots don't accumulate beyond the number it's supposed to retain. Reviewed By: maxijb Differential Revision: D28340893 Pulled By: davidmccabe fbshipit-source-id: e562b2917dde1e3f6ad9db49a67015a33e4eb0cf
AlexGuz23 · May 11, 2021 · 912d4d7 · 912d4d7
1 parent 49e5bc8
commit 912d4d7
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 48 deletions.
diff --git a/packages/recoil-devtools/src/pages/Page/PageScript.js b/packages/recoil-devtools/src/pages/Page/PageScript.js
@@ -35,49 +35,53 @@ async function normalizeSnapshot(
   if (snapshot == null) {
     return {modifiedValues: undefined};
   }
-
-  const dirtyNodes = snapshot.getNodes_UNSTABLE({isModified: onlyDirty});
-  const customSerialize = props.serializeFn ?? DefaultCustomSerialize;
   const modifiedValues = {};
-  const subscribers = new Set();
+  const release = snapshot.retain();
+  try {
+    const dirtyNodes = snapshot.getNodes_UNSTABLE({isModified: onlyDirty});
+    const customSerialize = props.serializeFn ?? DefaultCustomSerialize;
+    const subscribers = new Set();
 
-  // We wrap this loop into a promise to defer the execution
-  // of the second (subscribers) loop, so selector
-  // can settle before we check their values
-  await new Promise(resolve => {
-    for (const node of dirtyNodes) {
-      const info = snapshot.getInfo_UNSTABLE(node);
-      // We only accumulate subscribers if we are looking at only dirty nodes
-      if (onlyDirty) {
-        subscribers.add(...Array.from(info.subscribers.nodes));
+    // We wrap this loop into a promise to defer the execution
+    // of the second (subscribers) loop, so selector
+    // can settle before we check their values
+    await new Promise(resolve => {
+      for (const node of dirtyNodes) {
+        const info = snapshot.getInfo_UNSTABLE(node);
+        // We only accumulate subscribers if we are looking at only dirty nodes
+        if (onlyDirty) {
+          subscribers.add(...Array.from(info.subscribers.nodes));
+        }
+        modifiedValues[node.key] = {
+          content: serialize(
+            customSerialize(info.loadable?.contents, node.key),
+            props.maxDepth,
+            props.maxItems,
+          ),
+          nodeType: info.type,
+          deps: Array.from(info.deps).map(n => n.key),
+        };
       }
-      modifiedValues[node.key] = {
-        content: serialize(
-          customSerialize(info.loadable?.contents, node.key),
-          props.maxDepth,
-          props.maxItems,
-        ),
-        nodeType: info.type,
-        deps: Array.from(info.deps).map(n => n.key),
-      };
-    }
-    resolve();
-  });
+      resolve();
+    });
 
-  for (const node of subscribers) {
-    if (node != null) {
-      const info = snapshot.getInfo_UNSTABLE(node);
-      modifiedValues[node.key] = {
-        content: serialize(
-          customSerialize(info.loadable?.contents, node.key),
-          props.maxDepth,
-          props.maxItems,
-        ),
-        nodeType: info.type,
-        isSubscriber: true,
-        deps: Array.from(info.deps).map(n => n.key),
-      };
+    for (const node of subscribers) {
+      if (node != null) {
+        const info = snapshot.getInfo_UNSTABLE(node);
+        modifiedValues[node.key] = {
+          content: serialize(
+            customSerialize(info.loadable?.contents, node.key),
+            props.maxDepth,
+            props.maxItems,
+          ),
+          nodeType: info.type,
+          isSubscriber: true,
+          deps: Array.from(info.deps).map(n => n.key),
+        };
+      }
     }
+  } finally {
+    release();
   }
 
   return {
@@ -98,20 +102,29 @@ const __RECOIL_DEVTOOLS_EXTENSION__ = {
     debug('CONNECT_PAGE', props);
     initConnection(props);
     const {devMode, goToSnapshot, initialSnapshot} = props;
-    const previousSnapshots = new EvictableList<RecoilSnapshot>(
+    const previousSnapshots = new EvictableList<[RecoilSnapshot, () => void]>(
       props.persistenceLimit,
+      ([_, release]) => release(),
     );
     if (devMode && initialSnapshot != null) {
-      previousSnapshots.add(initialSnapshot);
+      previousSnapshots.add([initialSnapshot, initialSnapshot.retain()]);
+    }
+
+    let loadedSnapshot, releaseLoadedSnapshot;
+    function setLoadedSnapshot(s) {
+      releaseLoadedSnapshot?.();
+      [loadedSnapshot, releaseLoadedSnapshot] = [s, s?.retain()];
     }
+    setLoadedSnapshot(initialSnapshot);
 
-    let loadedSnapshot = initialSnapshot;
     const backgroundMessageListener = (message: MessageEventFromBackground) => {
       if (message.data?.source === ExtensionSourceContentScript) {
         if (message.data?.action === RecoilDevToolsActions.GO_TO_SNAPSHOT) {
-          loadedSnapshot = previousSnapshots.get(message.data?.snapshotId);
-          if (loadedSnapshot != null) {
-            goToSnapshot(loadedSnapshot);
+          const [snapshot] =
+            previousSnapshots.get(message.data?.snapshotId) ?? [];
+          setLoadedSnapshot(snapshot);
+          if (snapshot != null) {
+            goToSnapshot(snapshot);
           }
         }
       }
@@ -136,11 +149,11 @@ const __RECOIL_DEVTOOLS_EXTENSION__ = {
           return;
         }
         // reset the just loaded snapshot
-        loadedSnapshot = null;
+        setLoadedSnapshot(null);
         // On devMode we accumulate the list of rpevious snapshots
         // to be able to time travel
         if (devMode) {
-          previousSnapshots.add(snapshot);
+          previousSnapshots.add([snapshot, snapshot.retain()]);
         }
 
         window.postMessage(

diff --git a/packages/recoil-devtools/src/utils/EvictableList.js b/packages/recoil-devtools/src/utils/EvictableList.js
@@ -14,11 +14,13 @@ class EvictableList<TType> {
   list: Map<number, TType>;
   capacity: number;
   index: number;
+  onEvict: ?(TType) => void;
 
-  constructor(maxCapacity: number = 50) {
+  constructor(maxCapacity: number = 50, onEvict?: TType => void) {
     this.list = new Map();
-    this.capacity = maxCapacity;
     this.index = 0;
+    this.capacity = maxCapacity;
+    this.onEvict = onEvict;
   }
 
   add(elm: TType) {
@@ -32,6 +34,13 @@ class EvictableList<TType> {
   evict() {
     const keyToEvict = this.list.keys().next().value;
     if (keyToEvict != null) {
+      const onEvict = this.onEvict;
+      if (onEvict) {
+        const value = this.list.get(keyToEvict);
+        if (value !== undefined) {
+          onEvict(value);
+        }
+      }
       this.list.delete(keyToEvict);
     }
   }