diff --git a/CHANGELOG.md b/CHANGELOG.md index 865bc44..11f6dad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,15 @@ ## LATER -- Performance optimization to suppress re-rendering components: - - When subscribed selectors evaluate to the same value. (#749) - - On initial render when not using React Concurrent Mode (#820) - Memory management - Selector cache configuration ## NEXT - (Add new changes here as they land) +- Performance optimization to suppress re-rendering components: + - When subscribed selectors evaluate to the same value. (#749, #952) + - On initial render when not using React Concurrent Mode (#820) - Improved TypeScript and Flow typing for `Loadable`s (#966) - Added override prop to RecoilRoot - Fix not calling onSet() handler triggered from a setSelf() in onSet() for Atom Effects (#974, #979) diff --git a/src/hooks/Recoil_Hooks.js b/src/hooks/Recoil_Hooks.js index f87bed9..0fe2d50 100644 --- a/src/hooks/Recoil_Hooks.js +++ b/src/hooks/Recoil_Hooks.js @@ -323,6 +323,12 @@ function useRecoilValueLoadable_MUTABLESOURCE( if (!prevLoadableRef.current.is(newLoadable)) { callback(); } + // If the component is suspended then the effect setting prevLoadableRef + // will not run. So, set the previous value here when its subscription + // is fired to wake it up. We can't just rely on this, though, because + // this only executes when an atom/selector is dirty and the atom/selector + // passed to the hook can dynamically change. + prevLoadableRef.current = newLoadable; }, componentName, ); @@ -370,6 +376,7 @@ function useRecoilValueLoadable_LEGACY( if (!prevLoadableRef.current?.is(newLoadable)) { forceUpdate(newLoadable); } + prevLoadableRef.current = newLoadable; }, componentName, ); @@ -407,6 +414,7 @@ function useRecoilValueLoadable_LEGACY( if (!prevLoadableRef.current?.is(newLoadable)) { forceUpdate(newLoadable); } + prevLoadableRef.current = newLoadable; } return subscription.release; diff --git a/src/hooks/__tests__/Recoil_PublicHooks-test.js b/src/hooks/__tests__/Recoil_PublicHooks-test.js index cf3b7c3..58f589c 100644 --- a/src/hooks/__tests__/Recoil_PublicHooks-test.js +++ b/src/hooks/__tests__/Recoil_PublicHooks-test.js @@ -1578,6 +1578,77 @@ testRecoil('Resolution of suspense causes render just once', async gks => { expect(commit).toHaveBeenCalledTimes(BASE_CALLS + 3); }); +testRecoil('Wakeup from Suspense to previous value', async gks => { + const BASE_CALLS = + mutableSourceExists() || + gks.includes('recoil_suppress_rerender_in_callback') + ? 0 + : 1; + + const myAtom = atom({ + key: `atom${nextID++}`, + default: {value: 0}, + }); + const mySelector = selector({ + key: `selector${nextID++}`, + get: ({get}) => get(myAtom).value, + }); + + jest.useFakeTimers(); + const [Component, updateValue] = componentThatWritesAtom(myAtom); + const [ReadComp, commit] = componentThatReadsAtomWithCommitCount(mySelector); + const [container, suspense] = renderElementsWithSuspenseCount( + <> + + + , + ); + + // Render initial state "0" + act(() => jest.runAllTimers()); + await flushPromisesAndTimers(); + expect(container.textContent).toEqual('0'); + expect(suspense).toHaveBeenCalledTimes(0); + expect(commit).toHaveBeenCalledTimes(BASE_CALLS + 1); + + // Set selector to a pending state should cause component to suspend + act(() => updateValue({value: new Promise(() => {})})); + act(() => jest.runAllTimers()); + await flushPromisesAndTimers(); + expect(container.textContent).toEqual('loading'); + expect(suspense).toHaveBeenCalledTimes(1); + expect(commit).toHaveBeenCalledTimes(BASE_CALLS + 1); + + // Setting selector back to the previous state before it was pending should + // wake it up and render in previous state + act(() => updateValue({value: 0})); + act(() => jest.runAllTimers()); + await flushPromisesAndTimers(); + expect(container.textContent).toEqual('0'); + expect(suspense).toHaveBeenCalledTimes(1); + expect(commit).toHaveBeenCalledTimes(BASE_CALLS + 2); + + // Setting selector to a new state "1" should update and re-render + act(() => updateValue({value: 1})); + act(() => jest.runAllTimers()); + await flushPromisesAndTimers(); + expect(container.textContent).toEqual('1'); + expect(suspense).toHaveBeenCalledTimes(1); + expect(commit).toHaveBeenCalledTimes(BASE_CALLS + 3); + + // Setting selector to the same value "1" should avoid a re-render + act(() => updateValue({value: 1})); + act(() => jest.runAllTimers()); + await flushPromisesAndTimers(); + expect(container.textContent).toEqual('1'); + expect(suspense).toHaveBeenCalledTimes(1); + expect(commit).toHaveBeenCalledTimes( + BASE_CALLS + + 3 + + (gks.includes('recoil_suppress_rerender_in_callback') ? 0 : 1), + ); +}); + testRecoil( 'useTransactionObservation_DEPRECATED: Transaction dirty atoms are set', async () => { diff --git a/src/recoil_values/__tests__/Recoil_selector-test.js b/src/recoil_values/__tests__/Recoil_selector-test.js index 75ece52..bb50132 100644 --- a/src/recoil_values/__tests__/Recoil_selector-test.js +++ b/src/recoil_values/__tests__/Recoil_selector-test.js @@ -36,6 +36,7 @@ let React, ReadsAtom, renderElements, resolvingAsyncSelector, + loadingAsyncSelector, flushPromisesAndTimers, DefaultValue, mutableSourceExists, @@ -70,6 +71,7 @@ const testRecoil = getRecoilTestFn(() => { ReadsAtom, renderElements, resolvingAsyncSelector, + loadingAsyncSelector, flushPromisesAndTimers, } = require('../../testing/Recoil_TestingUtils')); ({noWait} = require('../Recoil_WaitFor')); @@ -774,6 +776,34 @@ testRecoil('Updating with changed selector', gks => { expect(c.textContent).toEqual('FOO'); }); +testRecoil('Change component prop to suspend and wake', () => { + const awakeSelector = constSelector('WAKE'); + const suspendedSelector = loadingAsyncSelector(); + + function TestComponent({side}) { + return ( + useRecoilValue(side === 'AWAKE' ? awakeSelector : suspendedSelector) ?? + 'LOADING' + ); + } + + let setSide; + const SelectorComponent = function () { + const [side, setSideState] = useState('AWAKE'); + setSide = setSideState; + return ; + }; + const c = renderElements(); + + expect(c.textContent).toEqual('WAKE'); + + act(() => setSide('SLEEP')); + expect(c.textContent).toEqual('loading'); + + act(() => setSide('AWAKE')); + expect(c.textContent).toEqual('WAKE'); +}); + /** * This test ensures that we are not running the selector's get() an unnecessary * number of times in response to async selectors resolving (i.e. by retrying diff --git a/src/testing/Recoil_TestingUtils.js b/src/testing/Recoil_TestingUtils.js index 5c72a54..6c5bf5f 100644 --- a/src/testing/Recoil_TestingUtils.js +++ b/src/testing/Recoil_TestingUtils.js @@ -159,7 +159,7 @@ const resolvingAsyncSelector: (T) => RecoilValue = ( get: () => Promise.resolve(value), }); -const loadingAsyncSelector: () => RecoilValue = () => +const loadingAsyncSelector: () => RecoilValueReadOnly = () => selector({ key: `LoadingSelector${id++}`, get: () => new Promise(() => {}),