Fix to always render w latest state

Summary:
Fixes the new Cache-friendly `useFragment()` implementation to handle the case of props changing to point to a different entity. The use-case is:

* Render user 1. This subscribes for updates on that user.
* In a batch, delete user 1 and enqueue an update to render user 2 instead.
* This should avoid incorrectly attempting to render the (deleted) user 1.

Currently we render once w the stale data and then render again with the correct data. This is now fixed: in addition to enqueuing a setState when the entity changes we also eagerly render w the new entity. Due to useFragmentInternal()'s use of state we can't avoid a double render in this case, but we can avoid rendering the stale data.

Reviewed By: davidmccabe

Differential Revision: D36956052

fbshipit-source-id: 9e54a8de7c2358cb35fa9f39ba2848dae1e9dfdf

packages/react-relay/relay-hooks/__tests__/useFragmentNode-test.js

@@ -154,6 +154,7 @@ describe.each([
     let renderPluralFragment;
     let forceSingularUpdate;
     let commitSpy;
+    let renderSpy;
     let SingularRenderer;
     let PluralRenderer;
 
@@ -180,6 +181,7 @@ describe.each([
       useEffect(() => {
         commitSpy(data);
       });
+      renderSpy(data);
 
       return [data];
     }
@@ -199,6 +201,32 @@ describe.each([
       commitSpy.mockClear();
     }
 
+    /// Asserts that a single rendering *batch* occurred, with possibly multiple render
+    /// calls and a single commit. `expectedCalls` describes the expected result as follows:
+    /// * items 0..length-1 (for length > 1) are calls expected to be rendered, but not committed
+    /// * item length-1 is expected to be rendered and committed
+    function assertRenderBatch(
+      expectedCalls: $ReadOnlyArray<{|data: $FlowFixMe|}>,
+    ) {
+      expect(expectedCalls.length >= 1).toBeTruthy(); // must expect at least one value
+
+      // the issue is that the initial miss-updates-on-subscribe thing is
+      // only on the second runAllImmediates here.
+      // This ensures that useEffect runs
+      internalAct(() => jest.runAllImmediates());
+      expect(renderSpy).toBeCalledTimes(expectedCalls.length);
+      expectedCalls.forEach((expected, idx) => {
+        const [actualData] = renderSpy.mock.calls[idx];
+        expect(actualData).toEqual(expected.data);
+      });
+      renderSpy.mockClear();
+
+      expect(commitSpy).toBeCalledTimes(1);
+      const [actualData] = commitSpy.mock.calls[0];
+      expect(actualData).toEqual(expectedCalls[expectedCalls.length - 1].data);
+      commitSpy.mockClear();
+    }
+
     function createFragmentRef(id: string, owner: OperationDescriptor) {
       return {
         [ID_KEY]: id,
@@ -215,6 +243,7 @@ describe.each([
         return jest.requireActual('scheduler/unstable_mock');
       });
       commitSpy = jest.fn();
+      renderSpy = jest.fn();
 
       // Set up environment and base data
       environment = createMockEnvironment();
@@ -434,6 +463,7 @@ describe.each([
       flushScheduler();
       environment.mockClear();
       commitSpy.mockClear();
+      renderSpy.mockClear();
     });
 
     it('should render singular fragment without error when data is available', () => {
@@ -651,7 +681,7 @@ describe.each([
 
     it('should re-read and resubscribe to fragment when fragment pointers change', () => {
       renderSingularFragment();
-      assertFragmentResults([
+      assertRenderBatch([
         {
           data: {
             id: '1',
@@ -677,7 +707,10 @@ describe.each([
         },
       });
 
-      internalAct(() => {
+      TestRenderer.act(() => {
+        environment.commitUpdate(store => {
+          store.delete('1');
+        });
         setSingularOwner(newQuery);
       });
 
@@ -689,9 +722,15 @@ describe.each([
         // Assert that ref now points to newQuery owner
         ...createFragmentRef('200', newQuery),
       };
-      assertFragmentResults([{data: expectedUser}]);
-
-      internalAct(() => {
+      if (isUsingReactCacheImplementation) {
+        // React Cache renders twice (because it has to update state for derived data),
+        // but avoids rendering with stale data on the initial update
+        assertRenderBatch([{data: expectedUser}, {data: expectedUser}]);
+      } else {
+        assertRenderBatch([{data: expectedUser}]);
+      }
+
+      TestRenderer.act(() => {
         environment.commitPayload(newQuery, {
           node: {
             __typename: 'User',
@@ -703,7 +742,7 @@ describe.each([
           },
         });
       });
-      assertFragmentResults([
+      assertRenderBatch([
         {
           data: {
             id: '200',

packages/react-relay/relay-hooks/react-cache/useFragmentInternal_REACT_CACHE.js

@@ -360,14 +360,14 @@ function useFragmentInternal_REACT_CACHE(
       return state;
     }
   };
-  const state = stateFromRawState(rawState);
+  let state = stateFromRawState(rawState);
 
   // This copy of the state we only update when something requires us to
   // unsubscribe and re-subscribe, namely a changed environment or
   // fragment selector.
   const [rawSubscribedState, setSubscribedState] = useState(state);
   // FIXME since this is used as an effect dependency, it needs to be memoized.
-  const subscribedState = stateFromRawState(rawSubscribedState);
+  let subscribedState = stateFromRawState(rawSubscribedState);
 
   const [previousFragmentSelector, setPreviousFragmentSelector] =
     useState(fragmentSelector);
@@ -376,11 +376,19 @@ function useFragmentInternal_REACT_CACHE(
     !areEqualSelectors(fragmentSelector, previousFragmentSelector) ||
     environment !== previousEnvironment
   ) {
+    // Enqueue setState to record the new selector and state
     setPreviousFragmentSelector(fragmentSelector);
     setPreviousEnvironment(environment);
-    const newState = getFragmentState(environment, fragmentSelector, isPlural);
+    const newState = stateFromRawState(
+      getFragmentState(environment, fragmentSelector, isPlural),
+    );
     setState(newState);
     setSubscribedState(newState); // This causes us to form a new subscription
+    // But render with the latest state w/o waiting for the setState. Otherwise
+    // the component would render the wrong information temporarily (including
+    // possibly incorrectly triggering some warnings below).
+    state = newState;
+    subscribedState = newState;
   }
 
   // Handle the queries for any missing client edges; this may suspend.