Always bail out timed out children even if they receive an update

I moved resetChildExpirationTime to ReactFiberCompleteWork so we can insert an additional reset only for SuspenseComponent.
facebook · Oct 20, 2018 · bf584c8 · bf584c8
1 parent 0fc0446
commit bf584c8
Show file tree

Hide file tree

Showing 3 changed files with 150 additions and 87 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -77,6 +77,9 @@ import {
   prepareToHydrateHostTextInstance,
   popHydrationState,
 } from './ReactFiberHydrationContext';
+import {enableProfilerTimer} from 'shared/ReactFeatureFlags';
+import {ProfileMode} from 'react-reconciler/src/ReactTypeOfMode';
+import {NoWork, Never} from './ReactFiberExpirationTime';
 
 function markUpdate(workInProgress: Fiber) {
   // Tag the fiber with an update effect. This turns a Placement into
@@ -531,6 +534,88 @@ if (supportsMutation) {
   };
 }
 
+function resetChildExpirationTime(
+  workInProgress: Fiber,
+  renderTime: ExpirationTime,
+) {
+  if (renderTime !== Never && workInProgress.childExpirationTime === Never) {
+    // The children of this component are hidden. Don't bubble their
+    // expiration times.
+    return;
+  }
+
+  let newChildExpirationTime = NoWork;
+
+  // Bubble up the earliest expiration time.
+  if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
+    // We're in profiling mode.
+    // Let's use this same traversal to update the render durations.
+    let actualDuration = workInProgress.actualDuration;
+    let treeBaseDuration = workInProgress.selfBaseDuration;
+
+    // When a fiber is cloned, its actualDuration is reset to 0.
+    // This value will only be updated if work is done on the fiber (i.e. it doesn't bailout).
+    // When work is done, it should bubble to the parent's actualDuration.
+    // If the fiber has not been cloned though, (meaning no work was done),
+    // Then this value will reflect the amount of time spent working on a previous render.
+    // In that case it should not bubble.
+    // We determine whether it was cloned by comparing the child pointer.
+    const shouldBubbleActualDurations =
+      workInProgress.alternate === null ||
+      workInProgress.child !== workInProgress.alternate.child;
+
+    let child = workInProgress.child;
+    while (child !== null) {
+      const childUpdateExpirationTime = child.expirationTime;
+      const childChildExpirationTime = child.childExpirationTime;
+      if (
+        newChildExpirationTime === NoWork ||
+        (childUpdateExpirationTime !== NoWork &&
+          childUpdateExpirationTime < newChildExpirationTime)
+      ) {
+        newChildExpirationTime = childUpdateExpirationTime;
+      }
+      if (
+        newChildExpirationTime === NoWork ||
+        (childChildExpirationTime !== NoWork &&
+          childChildExpirationTime < newChildExpirationTime)
+      ) {
+        newChildExpirationTime = childChildExpirationTime;
+      }
+      if (shouldBubbleActualDurations) {
+        actualDuration += child.actualDuration;
+      }
+      treeBaseDuration += child.treeBaseDuration;
+      child = child.sibling;
+    }
+    workInProgress.actualDuration = actualDuration;
+    workInProgress.treeBaseDuration = treeBaseDuration;
+  } else {
+    let child = workInProgress.child;
+    while (child !== null) {
+      const childUpdateExpirationTime = child.expirationTime;
+      const childChildExpirationTime = child.childExpirationTime;
+      if (
+        newChildExpirationTime === NoWork ||
+        (childUpdateExpirationTime !== NoWork &&
+          childUpdateExpirationTime < newChildExpirationTime)
+      ) {
+        newChildExpirationTime = childUpdateExpirationTime;
+      }
+      if (
+        newChildExpirationTime === NoWork ||
+        (childChildExpirationTime !== NoWork &&
+          childChildExpirationTime < newChildExpirationTime)
+      ) {
+        newChildExpirationTime = childChildExpirationTime;
+      }
+      child = child.sibling;
+    }
+  }
+
+  workInProgress.childExpirationTime = newChildExpirationTime;
+}
+
 function completeWork(
   current: Fiber | null,
   workInProgress: Fiber,
@@ -540,22 +625,24 @@ function completeWork(
 
   switch (workInProgress.tag) {
     case IndeterminateComponent:
-      break;
     case FunctionComponent:
     case FunctionComponentLazy:
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     case ClassComponent: {
       const Component = workInProgress.type;
       if (isLegacyContextProvider(Component)) {
         popLegacyContext(workInProgress);
       }
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     }
     case ClassComponentLazy: {
       const Component = getResultFromResolvedLazyComponent(workInProgress.type);
       if (isLegacyContextProvider(Component)) {
         popLegacyContext(workInProgress);
       }
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     }
     case HostRoot: {
@@ -575,6 +662,7 @@ function completeWork(
         workInProgress.effectTag &= ~Placement;
       }
       updateHostContainer(workInProgress);
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     }
     case HostComponent: {
@@ -657,6 +745,7 @@ function completeWork(
           markRef(workInProgress);
         }
       }
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     }
     case HostText: {
@@ -691,10 +780,12 @@ function completeWork(
           );
         }
       }
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     }
     case ForwardRef:
     case ForwardRefLazy:
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     case SuspenseComponent: {
       const nextState = workInProgress.memoizedState;
@@ -706,26 +797,33 @@ function completeWork(
         // and the timed-out state, schedule an effect.
         workInProgress.effectTag |= Update;
       }
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
+      if (nextDidTimeout) {
+        const fallbackFragment: Fiber = (workInProgress.child: any).sibling;
+        workInProgress.childExpirationTime =
+          fallbackFragment.childExpirationTime;
+      }
       break;
     }
     case Fragment:
-      break;
     case Mode:
-      break;
     case Profiler:
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     case HostPortal:
       popHostContainer(workInProgress);
       updateHostContainer(workInProgress);
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     case ContextProvider:
       // Pop provider fiber
       popProvider(workInProgress);
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     case ContextConsumer:
-      break;
     case PureComponent:
     case PureComponentLazy:
+      resetChildExpirationTime(workInProgress, renderExpirationTime);
       break;
     default:
       invariant(

diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -805,88 +805,6 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
   }
 }
 
-function resetChildExpirationTime(
-  workInProgress: Fiber,
-  renderTime: ExpirationTime,
-) {
-  if (renderTime !== Never && workInProgress.childExpirationTime === Never) {
-    // The children of this component are hidden. Don't bubble their
-    // expiration times.
-    return;
-  }
-
-  let newChildExpirationTime = NoWork;
-
-  // Bubble up the earliest expiration time.
-  if (enableProfilerTimer && workInProgress.mode & ProfileMode) {
-    // We're in profiling mode.
-    // Let's use this same traversal to update the render durations.
-    let actualDuration = workInProgress.actualDuration;
-    let treeBaseDuration = workInProgress.selfBaseDuration;
-
-    // When a fiber is cloned, its actualDuration is reset to 0.
-    // This value will only be updated if work is done on the fiber (i.e. it doesn't bailout).
-    // When work is done, it should bubble to the parent's actualDuration.
-    // If the fiber has not been cloned though, (meaning no work was done),
-    // Then this value will reflect the amount of time spent working on a previous render.
-    // In that case it should not bubble.
-    // We determine whether it was cloned by comparing the child pointer.
-    const shouldBubbleActualDurations =
-      workInProgress.alternate === null ||
-      workInProgress.child !== workInProgress.alternate.child;
-
-    let child = workInProgress.child;
-    while (child !== null) {
-      const childUpdateExpirationTime = child.expirationTime;
-      const childChildExpirationTime = child.childExpirationTime;
-      if (
-        newChildExpirationTime === NoWork ||
-        (childUpdateExpirationTime !== NoWork &&
-          childUpdateExpirationTime < newChildExpirationTime)
-      ) {
-        newChildExpirationTime = childUpdateExpirationTime;
-      }
-      if (
-        newChildExpirationTime === NoWork ||
-        (childChildExpirationTime !== NoWork &&
-          childChildExpirationTime < newChildExpirationTime)
-      ) {
-        newChildExpirationTime = childChildExpirationTime;
-      }
-      if (shouldBubbleActualDurations) {
-        actualDuration += child.actualDuration;
-      }
-      treeBaseDuration += child.treeBaseDuration;
-      child = child.sibling;
-    }
-    workInProgress.actualDuration = actualDuration;
-    workInProgress.treeBaseDuration = treeBaseDuration;
-  } else {
-    let child = workInProgress.child;
-    while (child !== null) {
-      const childUpdateExpirationTime = child.expirationTime;
-      const childChildExpirationTime = child.childExpirationTime;
-      if (
-        newChildExpirationTime === NoWork ||
-        (childUpdateExpirationTime !== NoWork &&
-          childUpdateExpirationTime < newChildExpirationTime)
-      ) {
-        newChildExpirationTime = childUpdateExpirationTime;
-      }
-      if (
-        newChildExpirationTime === NoWork ||
-        (childChildExpirationTime !== NoWork &&
-          childChildExpirationTime < newChildExpirationTime)
-      ) {
-        newChildExpirationTime = childChildExpirationTime;
-      }
-      child = child.sibling;
-    }
-  }
-
-  workInProgress.childExpirationTime = newChildExpirationTime;
-}
-
 function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
   // Attempt to complete the current unit of work, then move to the
   // next sibling. If there are no more siblings, return to the
@@ -929,7 +847,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {
         );
       }
       stopWorkTimer(workInProgress);
-      resetChildExpirationTime(workInProgress, nextRenderExpirationTime);
       if (__DEV__) {
         ReactCurrentFiber.resetCurrentFiber();
       }

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
@@ -435,5 +435,53 @@ describe('ReactSuspense', () => {
       ]);
       expect(root).toMatchRenderedOutput('AB:2C');
     });
+
+    it('bails out on timed-out primary children even if they receive an update', () => {
+      let instance;
+      class Stateful extends React.Component {
+        state = {step: 1};
+        render() {
+          instance = this;
+          return <Text text="Stateful" />;
+        }
+      }
+
+      function App(props) {
+        return (
+          <Suspense fallback={<Text text="Loading..." />}>
+            <Stateful />
+            <AsyncText ms={1000} text={props.text} />
+          </Suspense>
+        );
+      }
+
+      const root = ReactTestRenderer.create(<App text="A" />);
+
+      expect(ReactTestRenderer).toHaveYielded([
+        'Stateful',
+        'Suspend! [A]',
+        'Loading...',
+      ]);
+
+      jest.advanceTimersByTime(1000);
+      expect(ReactTestRenderer).toHaveYielded(['Promise resolved [A]', 'A']);
+      expect(root).toMatchRenderedOutput('StatefulA');
+
+      root.update(<App text="B" />);
+      expect(ReactTestRenderer).toHaveYielded([
+        'Stateful',
+        'Suspend! [B]',
+        'Loading...',
+      ]);
+
+      instance.setState({step: 2});
+
+      jest.advanceTimersByTime(1000);
+      expect(ReactTestRenderer).toHaveYielded([
+        'Promise resolved [B]',
+        'Stateful',
+        'B',
+      ]);
+    });
   });
 });