Fix suppression of re-renders with Suspense (#952)

Summary:
Pull Request resolved: #952

Fix the ability to suppress re-rendering React Components that depends on Recoil Selectors which evaluate to equivalent values via reference equality.

The problem was that when components observed a selector in a pending state it would suspend using Suspense.  This would cancel the execution of `useEffect()`'s and skip recording the previous selector values.  Then, if the selector reverted to the same value it had before the suspense, it wouldn't appear the value changed and the component could get stuck in Suspense.

This fix also fixes the extra re-render observed in the `Recoil_PublicHooks-test.js` when the React Tracing API was removed.

Reviewed By: davidmccabe

Differential Revision: D27547654

fbshipit-source-id: 8e155154a7dd6160e9e7e493c7dfddd54f309ca9

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)

src/hooks/Recoil_Hooks.js

@@ -323,6 +323,12 @@ function useRecoilValueLoadable_MUTABLESOURCE<T>(
           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<T>(
         if (!prevLoadableRef.current?.is(newLoadable)) {
           forceUpdate(newLoadable);
         }
+        prevLoadableRef.current = newLoadable;
       },
       componentName,
     );
@@ -407,6 +414,7 @@ function useRecoilValueLoadable_LEGACY<T>(
       if (!prevLoadableRef.current?.is(newLoadable)) {
         forceUpdate(newLoadable);
       }
+      prevLoadableRef.current = newLoadable;
     }
 
     return subscription.release;

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(
+    <>
+      <Component />
+      <ReadComp />
+    </>,
+  );
+
+  // 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 () => {

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 <TestComponent side={side} />;
+  };
+  const c = renderElements(<SelectorComponent />);
+
+  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

src/testing/Recoil_TestingUtils.js

@@ -159,7 +159,7 @@ const resolvingAsyncSelector: <T>(T) => RecoilValue<T> = <T>(
     get: () => Promise.resolve(value),
   });
 
-const loadingAsyncSelector: () => RecoilValue<void> = () =>
+const loadingAsyncSelector: () => RecoilValueReadOnly<void> = () =>
   selector({
     key: `LoadingSelector${id++}`,
     get: () => new Promise(() => {}),