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.

Differential Revision: D27547654

fbshipit-source-id: 37e67cdb248d452b4714ae8748274b8cf2b4c61b

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 () => {