From 441b1dfbb4dcf1bd19d96bc37eb6ad35bd482e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 20:50:29 +0100 Subject: [PATCH 01/24] fix error with hot module replacement --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index cbe3c86f2..ee5460a5f 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have From 10ce0e2e687fce6b40723f137914c30e12ad3338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 21:18:15 +0100 Subject: [PATCH 02/24] update tests --- libs/isograph-react-disposable-state/package.json | 1 + .../src/useCachedResponsivePrecommitValue.test.tsx | 7 ++++--- pnpm-lock.yaml | 10 ++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/libs/isograph-react-disposable-state/package.json b/libs/isograph-react-disposable-state/package.json index 633ce0083..06f831afe 100644 --- a/libs/isograph-react-disposable-state/package.json +++ b/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" }, diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx index f743da216..d8415b931 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx @@ -2,7 +2,7 @@ import { describe, test, vi, expect, assert } from 'vitest'; import { ParentCache } from './ParentCache'; import { ItemCleanupPair } from '@isograph/disposable-types'; import { useCachedResponsivePrecommitValue } from './useCachedResponsivePrecommitValue'; -import React from 'react'; +import React, { StrictMode, type ReactElement } from 'react'; import { create } from 'react-test-renderer'; 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, + {Component}, + // @ts-expect-error isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 21a8545f2..0cb26c848 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -367,6 +367,9 @@ importers: '@types/react': specifier: 18.3.1 version: 18.3.1 + '@types/react-test-renderer': + specifier: ^18.3.0 + version: 18.3.0 react-test-renderer: specifier: ^18.2.0 version: 18.3.1(react@18.3.1) @@ -2270,6 +2273,9 @@ packages: '@types/react-router@5.1.20': resolution: {integrity: sha512-jGjmu/ZqS7FjSH6owMcD5qpq19+1RS9DeVRqfl1FeBMxTDQAGwlMWOcs52NDoXaNKyG3d1cYQFMs9rCrb88o9Q==} + '@types/react-test-renderer@18.3.0': + resolution: {integrity: sha512-HW4MuEYxfDbOHQsVlY/XtOvNHftCVEPhJF2pQXXwcUiUF+Oyb0usgp48HSgpK5rt8m9KZb22yqOeZm+rrVG8gw==} + '@types/react-transition-group@4.4.11': resolution: {integrity: sha512-RM05tAniPZ5DZPzzNFP+DmrcOdD0efDUxMy3145oljWSl3x9ZV5vhme98gTxFrj2lhXvmGNnUiuDyJgY9IKkNA==} @@ -10317,6 +10323,10 @@ snapshots: '@types/history': 4.7.11 '@types/react': 18.3.1 + '@types/react-test-renderer@18.3.0': + dependencies: + '@types/react': 18.3.1 + '@types/react-transition-group@4.4.11': dependencies: '@types/react': 18.3.1 From 61b16b97444a51b83039850afcdd66216a454d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:07:00 +0100 Subject: [PATCH 03/24] fix cleanup --- .../src/useDisposableState.ts | 24 ++++++++++++------- .../src/useLazyDisposableState.test.tsx | 9 +++++-- .../src/useLazyDisposableState.ts | 14 +++++++---- .../src/useUpdatableDisposableState.test.tsx | 5 ++-- 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 46a63fe81..81e18fffe 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -22,7 +22,6 @@ export function useDisposableState( const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); @@ -47,14 +46,23 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - useEffect(function cleanupItemCleanupPairRefIfSetStateNotCalled() { - return () => { - if (itemCleanupPairRef.current !== null) { - itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; + const lastCommittedParentCache = useRef | null>(null); + + useEffect( + function cleanupItemCleanupPairRefIfSetStateNotCalled() { + if (lastCommittedParentCache.current === parentCache) { + return; } - }; - }, []); + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; + return () => { + // current is a stale variable + 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 diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx index 9d206cd63..38f4c3679 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx +++ b/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'; @@ -55,7 +55,12 @@ describe('useLazyDisposableState', async () => { return null; } - const root = create(, { unstable_isConcurrent: true }); + const root = create( + + + , + { unstable_isConcurrent: true }, + ); await committed.promise; expect(cache1.disposeItem).toHaveBeenCalled(); expect(cache1.cache.factory).toHaveBeenCalledOnce(); diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 5c251fd32..38da37c4c 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -23,18 +23,24 @@ export function useLazyDisposableState( state: T; } { const itemCleanupPairRef = useRef | null>(null); - const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); + const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; return () => { - const cleanupFn = itemCleanupPairRef.current?.[1]; + // current is a stale variable + const cleanupFn = current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( @@ -43,7 +49,7 @@ export function useLazyDisposableState( } return cleanupFn(); }; - }, []); + }, [parentCache]); const returnedItem = preCommitItem?.state ?? itemCleanupPairRef.current?.[0]; diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx index e74c132c8..48459175e 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx @@ -1,5 +1,5 @@ import { describe, test, vi, expect } from 'vitest'; -import React from 'react'; +import React, { StrictMode } from 'react'; import { create } from 'react-test-renderer'; import { useUpdatableDisposableState, @@ -45,7 +45,8 @@ function promiseAndResolver() { // not is a bit worrisome. async function awaitableCreate(Component, isConcurrent) { const element = create( - Component, + {Component}, + isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); From 4fbfb36536fed98f6ee36174cb2e85c444ac1955 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:26:30 +0100 Subject: [PATCH 04/24] don't use stale itemCleanupPairRef --- .../src/useDisposableState.ts | 8 ++++---- .../src/useLazyDisposableState.ts | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 81e18fffe..d5e979459 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -54,11 +54,11 @@ export function useDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - current?.[1](); + if (itemCleanupPairRef.current !== null) { + itemCleanupPairRef.current[1](); + } }; }, [parentCache], diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 38da37c4c..42a857e65 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -36,11 +36,9 @@ export function useLazyDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - const cleanupFn = current?.[1]; + const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( From e9d1845cb86e31c64c5a3a943938d89003d4b3f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 3 Jan 2025 18:42:13 +0100 Subject: [PATCH 05/24] add comment for guards added in `useEffects` --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ .../src/useLazyDisposableState.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index ee5460a5f..21c2c9c9c 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 42a857e65..930752c36 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -32,6 +32,10 @@ export function useLazyDisposableState( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } From 52bef4623a255f16dc604fb738fac9fe3ef1e9ea Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 10 Jan 2025 22:31:15 +0100 Subject: [PATCH 06/24] fix throwing in `useDisposableState` --- .../src/useDisposableState.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index d5e979459..1fd13946c 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -35,11 +35,6 @@ export function useDisposableState( 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.', - ); } } }, From 820a8f7241b810a8fea5e9690509302d5de30800 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 10 Jan 2025 22:31:30 +0100 Subject: [PATCH 07/24] fix double cleanup in `useUpdatableDisposableState` --- .../src/useUpdatableDisposableState.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts index 19360fdf2..569db283f 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts +++ b/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(); } }; From 20c773ea970d4629dfa40d93907563b4bcdd7f5c Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:13:21 +0100 Subject: [PATCH 08/24] detect hmr --- .../src/useCachedResponsivePrecommitValue.ts | 17 +++++++++-------- .../src/useLazyDisposableState.ts | 10 ---------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 21c2c9c9c..6b97eedd9 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,15 +51,16 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { - // react reruns all `useEffect` in HMR since it doesn't know if the - // code inside of useEffect has changed. Since this is a library - // user can't change this code so we are safe to skip this rerun. - // This also prevents `useEffect` from running twice in Strict Mode. - if (lastCommittedParentCache.current === parentCache) { - return; - } + 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. + lastCommittedParentCache.current = null; + }; + }, []); - lastCommittedParentCache.current = parentCache; + useEffect(() => { // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have // expired by the time of the commit. diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 930752c36..433bf338e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -30,17 +30,7 @@ export function useLazyDisposableState( }, ); - const lastCommittedParentCache = useRef | null>(null); useEffect(() => { - // react reruns all `useEffect` in HMR since it doesn't know if the - // code inside of useEffect has changed. Since this is a library - // user can't change this code so we are safe to skip this rerun. - // This also prevents `useEffect` from running twice in Strict Mode. - if (lastCommittedParentCache.current === parentCache) { - return; - } - lastCommittedParentCache.current = parentCache; - return () => { const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. From 78c6037640052da733d24a3b1a4eed58cc582eec Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:57:37 +0100 Subject: [PATCH 09/24] fix errors --- .../src/useCachedResponsivePrecommitValue.ts | 1 + .../src/useDisposableState.ts | 8 +------- .../src/useLazyDisposableState.ts | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 6b97eedd9..7723cefb1 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -61,6 +61,7 @@ export function useCachedResponsivePrecommitValue( }, []); useEffect(() => { + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have // expired by the time of the commit. diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 1fd13946c..c6de3295e 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -41,18 +41,12 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - const lastCommittedParentCache = useRef | null>(null); - useEffect( function cleanupItemCleanupPairRefIfSetStateNotCalled() { - if (lastCommittedParentCache.current === parentCache) { - return; - } - lastCommittedParentCache.current = parentCache; - return () => { if (itemCleanupPairRef.current !== null) { itemCleanupPairRef.current[1](); + itemCleanupPairRef.current = null; } }; }, diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 433bf338e..0a0e55d9e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -39,7 +39,7 @@ export function useLazyDisposableState( 'cleanupFn unexpectedly null. This indicates a bug in react-disposable-state.', ); } - return cleanupFn(); + cleanupFn(); }; }, [parentCache]); From 2e24a7fcb409981e9ab2c2b716da11b1c7f4731e Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:01:52 +0100 Subject: [PATCH 10/24] set `itemCleanupPairRef` to null on cleanup --- .../src/useLazyDisposableState.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 0a0e55d9e..00d1a9aa1 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -40,6 +40,7 @@ export function useLazyDisposableState( ); } cleanupFn(); + itemCleanupPairRef.current = null; }; }, [parentCache]); From 3f441e05648963fd53f8686b5574a78159db1155 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:06:44 +0100 Subject: [PATCH 11/24] `revert last` --- libs/isograph-react-disposable-state/src/useDisposableState.ts | 1 - .../src/useLazyDisposableState.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index c6de3295e..02857aaac 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -46,7 +46,6 @@ export function useDisposableState( return () => { if (itemCleanupPairRef.current !== null) { itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; } }; }, diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 00d1a9aa1..0a0e55d9e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -40,7 +40,6 @@ export function useLazyDisposableState( ); } cleanupFn(); - itemCleanupPairRef.current = null; }; }, [parentCache]); From d3de5d4ee3b11808b395e83087a0d2157d95873b Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:20:18 +0100 Subject: [PATCH 12/24] use ref of type bool to detect hmr --- .../src/useCachedResponsivePrecommitValue.ts | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 7723cefb1..4fcc257d5 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -50,13 +50,17 @@ export function useCachedResponsivePrecommitValue( const [, rerender] = useState<{} | null>(null); const lastCommittedParentCache = useRef | 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. - lastCommittedParentCache.current = null; + // 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; }; }, []); @@ -78,28 +82,32 @@ export function useCachedResponsivePrecommitValue( // // 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) { From ef925c63df95b98b63236147b3eca34d90f81cbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 20:50:29 +0100 Subject: [PATCH 13/24] fix error with hot module replacement --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index cbe3c86f2..ee5460a5f 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have From a6af861b5079780b18dc2239450c9d86bff5821d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Tue, 12 Nov 2024 21:18:15 +0100 Subject: [PATCH 14/24] update tests --- libs/isograph-react-disposable-state/package.json | 1 + .../src/useCachedResponsivePrecommitValue.test.tsx | 7 ++++--- pnpm-lock.yaml | 10 ++++++++++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/libs/isograph-react-disposable-state/package.json b/libs/isograph-react-disposable-state/package.json index 633ce0083..06f831afe 100644 --- a/libs/isograph-react-disposable-state/package.json +++ b/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" }, diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx index f743da216..d8415b931 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.test.tsx @@ -2,7 +2,7 @@ import { describe, test, vi, expect, assert } from 'vitest'; import { ParentCache } from './ParentCache'; import { ItemCleanupPair } from '@isograph/disposable-types'; import { useCachedResponsivePrecommitValue } from './useCachedResponsivePrecommitValue'; -import React from 'react'; +import React, { StrictMode, type ReactElement } from 'react'; import { create } from 'react-test-renderer'; 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, + {Component}, + // @ts-expect-error isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 21a8545f2..0cb26c848 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -367,6 +367,9 @@ importers: '@types/react': specifier: 18.3.1 version: 18.3.1 + '@types/react-test-renderer': + specifier: ^18.3.0 + version: 18.3.0 react-test-renderer: specifier: ^18.2.0 version: 18.3.1(react@18.3.1) @@ -2270,6 +2273,9 @@ packages: '@types/react-router@5.1.20': resolution: {integrity: sha512-jGjmu/ZqS7FjSH6owMcD5qpq19+1RS9DeVRqfl1FeBMxTDQAGwlMWOcs52NDoXaNKyG3d1cYQFMs9rCrb88o9Q==} + '@types/react-test-renderer@18.3.0': + resolution: {integrity: sha512-HW4MuEYxfDbOHQsVlY/XtOvNHftCVEPhJF2pQXXwcUiUF+Oyb0usgp48HSgpK5rt8m9KZb22yqOeZm+rrVG8gw==} + '@types/react-transition-group@4.4.11': resolution: {integrity: sha512-RM05tAniPZ5DZPzzNFP+DmrcOdD0efDUxMy3145oljWSl3x9ZV5vhme98gTxFrj2lhXvmGNnUiuDyJgY9IKkNA==} @@ -10317,6 +10323,10 @@ snapshots: '@types/history': 4.7.11 '@types/react': 18.3.1 + '@types/react-test-renderer@18.3.0': + dependencies: + '@types/react': 18.3.1 + '@types/react-transition-group@4.4.11': dependencies: '@types/react': 18.3.1 From b377add478b0594a340437821e82938499da5c30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:07:00 +0100 Subject: [PATCH 15/24] fix cleanup --- .../src/useDisposableState.ts | 24 ++++++++++++------- .../src/useLazyDisposableState.test.tsx | 9 +++++-- .../src/useLazyDisposableState.ts | 14 +++++++---- .../src/useUpdatableDisposableState.test.tsx | 5 ++-- 4 files changed, 36 insertions(+), 16 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 46a63fe81..81e18fffe 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -22,7 +22,6 @@ export function useDisposableState( const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); @@ -47,14 +46,23 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - useEffect(function cleanupItemCleanupPairRefIfSetStateNotCalled() { - return () => { - if (itemCleanupPairRef.current !== null) { - itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; + const lastCommittedParentCache = useRef | null>(null); + + useEffect( + function cleanupItemCleanupPairRefIfSetStateNotCalled() { + if (lastCommittedParentCache.current === parentCache) { + return; } - }; - }, []); + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; + return () => { + // current is a stale variable + 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 diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx index 9d206cd63..38f4c3679 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.test.tsx +++ b/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'; @@ -55,7 +55,12 @@ describe('useLazyDisposableState', async () => { return null; } - const root = create(, { unstable_isConcurrent: true }); + const root = create( + + + , + { unstable_isConcurrent: true }, + ); await committed.promise; expect(cache1.disposeItem).toHaveBeenCalled(); expect(cache1.cache.factory).toHaveBeenCalledOnce(); diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 5c251fd32..38da37c4c 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -23,18 +23,24 @@ export function useLazyDisposableState( state: T; } { const itemCleanupPairRef = useRef | null>(null); - const preCommitItem = useCachedResponsivePrecommitValue( parentCache, (pair) => { - itemCleanupPairRef.current?.[1](); itemCleanupPairRef.current = pair; }, ); + const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + if (lastCommittedParentCache.current === parentCache) { + return; + } + lastCommittedParentCache.current = parentCache; + // capture last set pair in a variable + const current = itemCleanupPairRef.current; return () => { - const cleanupFn = itemCleanupPairRef.current?.[1]; + // current is a stale variable + const cleanupFn = current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( @@ -43,7 +49,7 @@ export function useLazyDisposableState( } return cleanupFn(); }; - }, []); + }, [parentCache]); const returnedItem = preCommitItem?.state ?? itemCleanupPairRef.current?.[0]; diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx index e74c132c8..48459175e 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx +++ b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.test.tsx @@ -1,5 +1,5 @@ import { describe, test, vi, expect } from 'vitest'; -import React from 'react'; +import React, { StrictMode } from 'react'; import { create } from 'react-test-renderer'; import { useUpdatableDisposableState, @@ -45,7 +45,8 @@ function promiseAndResolver() { // not is a bit worrisome. async function awaitableCreate(Component, isConcurrent) { const element = create( - Component, + {Component}, + isConcurrent ? { unstable_isConcurrent: true } : undefined, ); await shortPromise(); From faf696aceb6cfd6b7c83539b740585a48a3576f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:26:30 +0100 Subject: [PATCH 16/24] don't use stale itemCleanupPairRef --- .../src/useDisposableState.ts | 8 ++++---- .../src/useLazyDisposableState.ts | 6 ++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 81e18fffe..d5e979459 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -54,11 +54,11 @@ export function useDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - current?.[1](); + if (itemCleanupPairRef.current !== null) { + itemCleanupPairRef.current[1](); + } }; }, [parentCache], diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 38da37c4c..42a857e65 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -36,11 +36,9 @@ export function useLazyDisposableState( return; } lastCommittedParentCache.current = parentCache; - // capture last set pair in a variable - const current = itemCleanupPairRef.current; + return () => { - // current is a stale variable - const cleanupFn = current?.[1]; + const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. if (cleanupFn == null) { throw new Error( From 8352c7209a76a03bf9306db9bb4fb0e9351adf89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Patryk=20Wa=C5=82ach?= <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 3 Jan 2025 18:42:13 +0100 Subject: [PATCH 17/24] add comment for guards added in `useEffects` --- .../src/useCachedResponsivePrecommitValue.ts | 4 ++++ .../src/useLazyDisposableState.ts | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index ee5460a5f..21c2c9c9c 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,6 +51,10 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 42a857e65..930752c36 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -32,6 +32,10 @@ export function useLazyDisposableState( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { + // react reruns all `useEffect` in HMR since it doesn't know if the + // code inside of useEffect has changed. Since this is a library + // user can't change this code so we are safe to skip this rerun. + // This also prevents `useEffect` from running twice in Strict Mode. if (lastCommittedParentCache.current === parentCache) { return; } From c0083e3d8a75f9f3172908790568768a290e8428 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 10 Jan 2025 22:31:15 +0100 Subject: [PATCH 18/24] fix throwing in `useDisposableState` --- .../src/useDisposableState.ts | 5 ----- 1 file changed, 5 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index d5e979459..1fd13946c 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -35,11 +35,6 @@ export function useDisposableState( 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.', - ); } } }, From bef4639550fae8b58253b3792f4954576bf89210 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Fri, 10 Jan 2025 22:31:30 +0100 Subject: [PATCH 19/24] fix double cleanup in `useUpdatableDisposableState` --- .../src/useUpdatableDisposableState.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts b/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts index 19360fdf2..569db283f 100644 --- a/libs/isograph-react-disposable-state/src/useUpdatableDisposableState.ts +++ b/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(); } }; From 652be8457cdee567b60035d239db10b5ed2f7076 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:13:21 +0100 Subject: [PATCH 20/24] detect hmr --- .../src/useCachedResponsivePrecommitValue.ts | 17 +++++++++-------- .../src/useLazyDisposableState.ts | 10 ---------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 21c2c9c9c..6b97eedd9 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -51,15 +51,16 @@ export function useCachedResponsivePrecommitValue( const lastCommittedParentCache = useRef | null>(null); useEffect(() => { - // react reruns all `useEffect` in HMR since it doesn't know if the - // code inside of useEffect has changed. Since this is a library - // user can't change this code so we are safe to skip this rerun. - // This also prevents `useEffect` from running twice in Strict Mode. - if (lastCommittedParentCache.current === parentCache) { - return; - } + 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. + lastCommittedParentCache.current = null; + }; + }, []); - lastCommittedParentCache.current = parentCache; + useEffect(() => { // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have // expired by the time of the commit. diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 930752c36..433bf338e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -30,17 +30,7 @@ export function useLazyDisposableState( }, ); - const lastCommittedParentCache = useRef | null>(null); useEffect(() => { - // react reruns all `useEffect` in HMR since it doesn't know if the - // code inside of useEffect has changed. Since this is a library - // user can't change this code so we are safe to skip this rerun. - // This also prevents `useEffect` from running twice in Strict Mode. - if (lastCommittedParentCache.current === parentCache) { - return; - } - lastCommittedParentCache.current = parentCache; - return () => { const cleanupFn = itemCleanupPairRef.current?.[1]; // TODO confirm useEffect is called in order. From 8b0fdab4d15238bb5a37000725e3beb5e7fef3e0 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 20:57:37 +0100 Subject: [PATCH 21/24] fix errors --- .../src/useCachedResponsivePrecommitValue.ts | 1 + .../src/useDisposableState.ts | 8 +------- .../src/useLazyDisposableState.ts | 2 +- 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 6b97eedd9..7723cefb1 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -61,6 +61,7 @@ export function useCachedResponsivePrecommitValue( }, []); useEffect(() => { + lastCommittedParentCache.current = parentCache; // On commit, cacheItem may be disposed, because during the render phase, // we only temporarily retained the item, and the temporary retain could have // expired by the time of the commit. diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index 1fd13946c..c6de3295e 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -41,18 +41,12 @@ export function useDisposableState( [stateFromDisposableStateHook], ); - const lastCommittedParentCache = useRef | null>(null); - useEffect( function cleanupItemCleanupPairRefIfSetStateNotCalled() { - if (lastCommittedParentCache.current === parentCache) { - return; - } - lastCommittedParentCache.current = parentCache; - return () => { if (itemCleanupPairRef.current !== null) { itemCleanupPairRef.current[1](); + itemCleanupPairRef.current = null; } }; }, diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 433bf338e..0a0e55d9e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -39,7 +39,7 @@ export function useLazyDisposableState( 'cleanupFn unexpectedly null. This indicates a bug in react-disposable-state.', ); } - return cleanupFn(); + cleanupFn(); }; }, [parentCache]); From e32921992a067e36262ea4218bd460f67a1e295f Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:01:52 +0100 Subject: [PATCH 22/24] set `itemCleanupPairRef` to null on cleanup --- .../src/useLazyDisposableState.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 0a0e55d9e..00d1a9aa1 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -40,6 +40,7 @@ export function useLazyDisposableState( ); } cleanupFn(); + itemCleanupPairRef.current = null; }; }, [parentCache]); From ad22a27c93a01992aa1b526e959ed955f540c5e7 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:06:44 +0100 Subject: [PATCH 23/24] `revert last` --- libs/isograph-react-disposable-state/src/useDisposableState.ts | 1 - .../src/useLazyDisposableState.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useDisposableState.ts b/libs/isograph-react-disposable-state/src/useDisposableState.ts index c6de3295e..02857aaac 100644 --- a/libs/isograph-react-disposable-state/src/useDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useDisposableState.ts @@ -46,7 +46,6 @@ export function useDisposableState( return () => { if (itemCleanupPairRef.current !== null) { itemCleanupPairRef.current[1](); - itemCleanupPairRef.current = null; } }; }, diff --git a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts index 00d1a9aa1..0a0e55d9e 100644 --- a/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts +++ b/libs/isograph-react-disposable-state/src/useLazyDisposableState.ts @@ -40,7 +40,6 @@ export function useLazyDisposableState( ); } cleanupFn(); - itemCleanupPairRef.current = null; }; }, [parentCache]); From 50f95ea33c5a57a677eecfbaca18f4ffafe9ae40 Mon Sep 17 00:00:00 2001 From: PatrykWalach <35966385+PatrykWalach@users.noreply.github.com> Date: Mon, 13 Jan 2025 21:20:18 +0100 Subject: [PATCH 24/24] use ref of type bool to detect hmr --- .../src/useCachedResponsivePrecommitValue.ts | 52 +++++++++++-------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts index 7723cefb1..4fcc257d5 100644 --- a/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts +++ b/libs/isograph-react-disposable-state/src/useCachedResponsivePrecommitValue.ts @@ -50,13 +50,17 @@ export function useCachedResponsivePrecommitValue( const [, rerender] = useState<{} | null>(null); const lastCommittedParentCache = useRef | 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. - lastCommittedParentCache.current = null; + // 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; }; }, []); @@ -78,28 +82,32 @@ export function useCachedResponsivePrecommitValue( // // 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) {