Toggle visibility of Suspense children in mutation phase, not layout

If parent reads visibility of children in a lifecycle, they should have already updated.
facebook · Nov 6, 2018 · d046c96 · d046c96
1 parent 8fab1de
commit d046c96
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 24 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -449,30 +449,8 @@ function commitLifeCycles(
       }
       return;
     }
-    case SuspenseComponent: {
-      let newState: SuspenseState | null = finishedWork.memoizedState;
-
-      let newDidTimeout;
-      let primaryChildParent = finishedWork;
-      if (newState === null) {
-        newDidTimeout = false;
-      } else {
-        newDidTimeout = true;
-        primaryChildParent = finishedWork.child;
-        if (newState.timedOutAt === NoWork) {
-          // If the children had not already timed out, record the time.
-          // This is used to compute the elapsed time during subsequent
-          // attempts to render the children.
-          newState.timedOutAt = requestCurrentTime();
-        }
-      }
-
-      if (primaryChildParent !== null) {
-        hideOrUnhideAllChildren(primaryChildParent, newDidTimeout);
-      }
-
-      return;
-    }
+    case SuspenseComponent:
+      break;
     case IncompleteClassComponent:
       break;
     default: {
@@ -1043,6 +1021,26 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       return;
     }
     case SuspenseComponent: {
+      let newState: SuspenseState | null = finishedWork.memoizedState;
+
+      let newDidTimeout;
+      let primaryChildParent = finishedWork;
+      if (newState === null) {
+        newDidTimeout = false;
+      } else {
+        newDidTimeout = true;
+        primaryChildParent = finishedWork.child;
+        if (newState.timedOutAt === NoWork) {
+          // If the children had not already timed out, record the time.
+          // This is used to compute the elapsed time during subsequent
+          // attempts to render the children.
+          newState.timedOutAt = requestCurrentTime();
+        }
+      }
+
+      if (primaryChildParent !== null) {
+        hideOrUnhideAllChildren(primaryChildParent, newDidTimeout);
+      }
       return;
     }
     case IncompleteClassComponent: {

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
@@ -17,6 +17,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
   beforeEach(() => {
     jest.resetModules();
     ReactFeatureFlags = require('shared/ReactFeatureFlags');
+    ReactFeatureFlags.enableHooks = true;
     ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
     ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
     React = require('react');
@@ -1378,6 +1379,45 @@ describe('ReactSuspenseWithNoopRenderer', () => {
       await advanceTimers(100);
       expect(ReactNoop.getChildren()).toEqual([span('Hi')]);
     });
+
+    it('toggles visibility during the mutation phase', async () => {
+      const {useRef, useLayoutEffect} = React;
+
+      function Parent() {
+        const child = useRef(null);
+
+        useLayoutEffect(() => {
+          ReactNoop.yield('Child is hidden: ' + child.current.hidden);
+        });
+
+        return (
+          <span ref={child} hidden={false}>
+            <AsyncText ms={1000} text="Hi" />
+          </span>
+        );
+      }
+
+      function App(props) {
+        return (
+          <Suspense fallback={<Text text="Loading..." />}>
+            <Parent />
+          </Suspense>
+        );
+      }
+
+      ReactNoop.renderLegacySyncRoot(<App middleText="B" />);
+
+      expect(ReactNoop.clearYields()).toEqual([
+        'Suspend! [Hi]',
+        'Loading...',
+        // The child should have already been hidden
+        'Child is hidden: true',
+      ]);
+
+      await advanceTimers(1000);
+
+      expect(ReactNoop.clearYields()).toEqual(['Promise resolved [Hi]', 'Hi']);
+    });
   });
 
   it('does not call lifecycles of a suspended component', async () => {