Fix error occurring in useEffect on hmr

libs/isograph-react-disposable-state/package.json

@@ -25,6 +25,7 @@
   },
   "devDependencies": {
     "@types/react": "18.3.1",
+    "@types/react-test-renderer": "^18.3.0",
     "react-test-renderer": "^18.2.0",
     "typescript": "5.6.3"
   },

libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx

@@ -1,5 +1,5 @@
 import { ItemCleanupPair } from '@isograph/disposable-types';
-import React from 'react';
+import React, { StrictMode, type ReactElement } from 'react';
 import { create } from 'react-test-renderer';
 import { assert, describe, expect, test, vi } from 'vitest';
 import { CacheItem, CacheItemState } from './CacheItem';
@@ -51,9 +51,10 @@ function promiseAndResolver() {
 
 // The fact that sometimes we need to render in concurrent mode and sometimes
 // not is a bit worrisome.
-async function awaitableCreate(Component, isConcurrent) {
+async function awaitableCreate(Component: ReactElement, isConcurrent) {
   const element = create(
-    Component,
+    <StrictMode>{Component}</StrictMode>,
+    // @ts-expect-error
     isConcurrent ? { unstable_isConcurrent: true } : undefined,
   );
   await shortPromise();

libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts

@@ -50,6 +50,20 @@ export function useCachedResponsivePrecommitValue<T>(
   const [, rerender] = useState<{} | null>(null);
   const lastCommittedParentCache = useRef<ParentCache<T> | null>(null);
 
+  const maybeHiddenOrFastRefresh = useRef(false);
+  useEffect(() => {
+    return () => {
+      // Attempt to detect if the component was
+      // hidden (by Offscreen API), or fast refresh occured;
+      // Only in these situations would the effect cleanup
+      // for "unmounting" run multiple times, so if
+      // we are ever able to read this ref with a value
+      // of true, it means that one of these cases
+      // has happened.
+      maybeHiddenOrFastRefresh.current = true;
+    };
+  }, []);
+
   useEffect(() => {
     lastCommittedParentCache.current = parentCache;
     // On commit, cacheItem may be disposed, because during the render phase,
@@ -68,28 +82,32 @@ export function useCachedResponsivePrecommitValue<T>(
     //
     // After the above, we have a non-disposed item and a cleanup function, which we
     // can pass to onCommit.
-    const undisposedPair = cacheItem.permanentRetainIfNotDisposed(
-      disposeOfTemporaryRetain,
-    );
-    if (undisposedPair !== null) {
-      onCommit(undisposedPair);
-    } else {
-      // The cache item we created during render has been disposed. Check if the parent
-      // cache is populated.
-      const existingCacheItemCleanupPair =
-        parentCache.getAndPermanentRetainIfPresent();
-      if (existingCacheItemCleanupPair !== null) {
-        onCommit(existingCacheItemCleanupPair);
-      } else {
-        // We did not find an item in the parent cache, create a new one.
-        onCommit(parentCache.factory());
+    if (maybeHiddenOrFastRefresh.current === false) {
+      const undisposedPair = cacheItem.permanentRetainIfNotDisposed(
+        disposeOfTemporaryRetain,
+      );
+      if (undisposedPair !== null) {
+        onCommit(undisposedPair);
+        return;
       }
-      // TODO: Consider whether we always want to rerender if the committed item
-      // was not returned during the last render, or whether some callers will
-      // prefer opting out of this behavior (e.g. if every disposable item behaves
-      // identically, but must be loaded.)
-      rerender({});
     }
+    maybeHiddenOrFastRefresh.current = false;
+
+    // The cache item we created during render has been disposed. Check if the parent
+    // cache is populated.
+    const existingCacheItemCleanupPair =
+      parentCache.getAndPermanentRetainIfPresent();
+    if (existingCacheItemCleanupPair !== null) {
+      onCommit(existingCacheItemCleanupPair);
+    } else {
+      // We did not find an item in the parent cache, create a new one.
+      onCommit(parentCache.factory());
+    }
+    // TODO: Consider whether we always want to rerender if the committed item
+    // was not returned during the last render, or whether some callers will
+    // prefer opting out of this behavior (e.g. if every disposable item behaves
+    // identically, but must be loaded.)
+    rerender({});
   }, [parentCache]);
 
   if (lastCommittedParentCache.current === parentCache) {

libs/isograph-react-disposable-state/src/useDisposableState.ts

@@ -21,7 +21,6 @@ export function useDisposableState<T = never>(
   const preCommitItem = useCachedResponsivePrecommitValue(
     parentCache,
     (pair) => {
-      itemCleanupPairRef.current?.[1]();
       itemCleanupPairRef.current = pair;
     },
   );
@@ -35,25 +34,22 @@ export function useDisposableState<T = never>(
         if (itemCleanupPairRef.current !== null) {
           itemCleanupPairRef.current[1]();
           itemCleanupPairRef.current = null;
-        } else {
-          throw new Error(
-            'itemCleanupPairRef.current is unexpectedly null. ' +
-              'This indicates a bug in react-disposable-state.',
-          );
         }
       }
     },
     [stateFromDisposableStateHook],
   );
 
-  useEffect(function cleanupItemCleanupPairRefIfSetStateNotCalled() {
-    return () => {
-      if (itemCleanupPairRef.current !== null) {
-        itemCleanupPairRef.current[1]();
-        itemCleanupPairRef.current = null;
-      }
-    };
-  }, []);
+  useEffect(
+    function cleanupItemCleanupPairRefIfSetStateNotCalled() {
+      return () => {
+        if (itemCleanupPairRef.current !== null) {
+          itemCleanupPairRef.current[1]();
+        }
+      };
+    },
+    [parentCache],
+  );
 
   // Safety: we can be in one of three states. Pre-commit, in which case
   // preCommitItem is assigned, post-commit but before setState has been

libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx

@@ -1,5 +1,5 @@
 import { ItemCleanupPair } from '@isograph/disposable-types';
-import React, { useEffect, useState } from 'react';
+import React, { StrictMode, useEffect, useState } from 'react';
 import { create } from 'react-test-renderer';
 import { describe, expect, test, vi } from 'vitest';
 import { ParentCache } from './ParentCache';
@@ -56,7 +56,12 @@ describe('useLazyDisposableState', async () => {
       return null;
     }
 
-    const root = create(<TestComponent />, { unstable_isConcurrent: true });
+    const root = create(
+      <StrictMode>
+        <TestComponent />
+      </StrictMode>,
+      { unstable_isConcurrent: true },
+    );
     await committed.promise;
     expect(cache1.disposeItem).toHaveBeenCalled();
     expect(cache1.cache.factory).toHaveBeenCalledOnce();

libs/isograph-react-disposable-state/src/useLazyDisposableState.ts

@@ -21,11 +21,9 @@ export function useLazyDisposableState<T>(
   state: T;
 } {
   const itemCleanupPairRef = useRef<ItemCleanupPair<T> | null>(null);
-
   const preCommitItem = useCachedResponsivePrecommitValue(
     parentCache,
     (pair) => {
-      itemCleanupPairRef.current?.[1]();
       itemCleanupPairRef.current = pair;
     },
   );
@@ -39,9 +37,9 @@ export function useLazyDisposableState<T>(
           'cleanupFn unexpectedly null. This indicates a bug in react-disposable-state.',
         );
       }
-      return cleanupFn();
+      cleanupFn();
     };
-  }, []);
+  }, [parentCache]);
 
   const returnedItem = preCommitItem?.state ?? itemCleanupPairRef.current?.[0];
 

libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx

@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { StrictMode } from 'react';
 import { create } from 'react-test-renderer';
 import { describe, expect, test, vi } from 'vitest';
 import {
@@ -45,7 +45,8 @@ function promiseAndResolver() {
 // not is a bit worrisome.
 async function awaitableCreate(Component, isConcurrent) {
   const element = create(
-    Component,
+    <StrictMode>{Component}</StrictMode>,
+
     isConcurrent ? { unstable_isConcurrent: true } : undefined,
   );
   await shortPromise();

libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts

@@ -109,6 +109,7 @@ export function useUpdatableDisposableState<
   useEffect(() => {
     return function disposeAllRemainingItems() {
       for (const undisposedICI of undisposedICIs.current) {
+        undisposedICIs.current.delete(undisposedICI);
         undisposedICI.cleanup();
       }
     };