Cancel pending commit before starting on root

acdlite · acdlite · commit 4221565e1552 · 2019-04-15T16:00:18.000-07:00
Moves the cancelTimeout call to right before creating a new work-in-
progress root. Fixes a class of bugs where a pending commit is not
cancelled, causing an incomplete tree to accidentally commit.

In the interest of fixing downstream bugs quickly, I'm landing this
without a test case; I'll add one in a follow up.
diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -479,15 +479,6 @@ function scheduleCallbackForRoot(
     }
   }
 
-  const timeoutHandle = root.timeoutHandle;
-  if (timeoutHandle !== noTimeout) {
-    // The root previous suspended and scheduled a timeout to commit a fallback
-    // state. Now that we have additional work, cancel the timeout.
-    root.timeoutHandle = noTimeout;
-    // $FlowFixMe Complains noTimeout is not a TimeoutID, despite the check above
-    cancelTimeout(timeoutHandle);
-  }
-
   // Add the current set of interactions to the pending set associated with
   // this root.
   schedulePendingInteraction(root, expirationTime);
@@ -673,6 +664,15 @@ export function flushControlled(fn: () => mixed): void {
 function prepareFreshStack(root, expirationTime) {
   root.pendingCommitExpirationTime = NoWork;
 
+  const timeoutHandle = root.timeoutHandle;
+  if (timeoutHandle !== noTimeout) {
+    // The root previous suspended and scheduled a timeout to commit a fallback
+    // state. Now that we have additional work, cancel the timeout.
+    root.timeoutHandle = noTimeout;
+    // $FlowFixMe Complains noTimeout is not a TimeoutID, despite the check above
+    cancelTimeout(timeoutHandle);
+  }
+
   if (workInProgress !== null) {
     let interruptedWork = workInProgress.return;
     while (interruptedWork !== null) {
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
@@ -1724,11 +1724,3 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
   });
 });
-
-// TODO:
-// An update suspends, timeout is scheduled. Update again with different timeout.
-// An update suspends, a higher priority update also suspends, each has different timeouts.
-// Can update siblings of a timed out placeholder without suspending
-// Pinging during the render phase
-// Synchronous thenable
-// Start time is computed using earliest suspended time
