Revert "Remove JND delay for non-transition updates (facebook#26597)"

This reverts commit 0b931f9.

packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js

@@ -236,7 +236,7 @@ describe('ReactCache', () => {
 
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [4]']);
-    await waitForAll([1, 4, 'Suspend! [5]']);
+    await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']);
 
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [5]']);
@@ -264,7 +264,7 @@ describe('ReactCache', () => {
     ]);
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [2]']);
-    await waitForAll([1, 2, 'Suspend! [3]']);
+    await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']);
 
     jest.advanceTimersByTime(100);
     assertLog(['Promise resolved [3]']);

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -145,6 +145,7 @@ import {
   includesExpiredLane,
   getNextLanes,
   getLanesToRetrySynchronouslyOnError,
+  getMostRecentEventTime,
   markRootUpdated,
   markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
   markRootPinged,
@@ -283,6 +284,8 @@ import {
 } from './ReactFiberRootScheduler';
 import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';
 
+const ceil = Math.ceil;
+
 const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map;
 
 const {
@@ -1190,6 +1193,38 @@ function finishConcurrentRender(
         break;
       }
 
+      if (!shouldForceFlushFallbacksInDEV()) {
+        // This is not a transition, but we did trigger an avoided state.
+        // Schedule a placeholder to display after a short delay, using the Just
+        // Noticeable Difference.
+        // TODO: Is the JND optimization worth the added complexity? If this is
+        // the only reason we track the event time, then probably not.
+        // Consider removing.
+
+        const mostRecentEventTime = getMostRecentEventTime(root, lanes);
+        const eventTimeMs = mostRecentEventTime;
+        const timeElapsedMs = now() - eventTimeMs;
+        const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
+
+        // Don't bother with a very short suspense time.
+        if (msUntilTimeout > 10) {
+          // Instead of committing the fallback immediately, wait for more data
+          // to arrive.
+          root.timeoutHandle = scheduleTimeout(
+            commitRootWhenReady.bind(
+              null,
+              root,
+              finishedWork,
+              workInProgressRootRecoverableErrors,
+              workInProgressTransitions,
+              lanes,
+            ),
+            msUntilTimeout,
+          );
+          break;
+        }
+      }
+
       // Commit the placeholder.
       commitRootWhenReady(
         root,
@@ -3545,6 +3580,31 @@ export function resolveRetryWakeable(boundaryFiber: Fiber, wakeable: Wakeable) {
   retryTimedOutBoundary(boundaryFiber, retryLane);
 }
 
+// Computes the next Just Noticeable Difference (JND) boundary.
+// The theory is that a person can't tell the difference between small differences in time.
+// Therefore, if we wait a bit longer than necessary that won't translate to a noticeable
+// difference in the experience. However, waiting for longer might mean that we can avoid
+// showing an intermediate loading state. The longer we have already waited, the harder it
+// is to tell small differences in time. Therefore, the longer we've already waited,
+// the longer we can wait additionally. At some point we have to give up though.
+// We pick a train model where the next boundary commits at a consistent schedule.
+// These particular numbers are vague estimates. We expect to adjust them based on research.
+function jnd(timeElapsed: number) {
+  return timeElapsed < 120
+    ? 120
+    : timeElapsed < 480
+    ? 480
+    : timeElapsed < 1080
+    ? 1080
+    : timeElapsed < 1920
+    ? 1920
+    : timeElapsed < 3000
+    ? 3000
+    : timeElapsed < 4320
+    ? 4320
+    : ceil(timeElapsed / 1960) * 1960;
+}
+
 export function throwIfInfiniteUpdateLoopDetected() {
   if (nestedUpdateCount > NESTED_UPDATE_LIMIT) {
     nestedUpdateCount = 0;

packages/react-reconciler/src/__tests__/ReactExpiration-test.js

@@ -732,9 +732,13 @@ describe('ReactExpiration', () => {
     expect(root).toMatchRenderedOutput('A0BC');
 
     await act(async () => {
-      React.startTransition(() => {
+      if (gate(flags => flags.enableSyncDefaultUpdates)) {
+        React.startTransition(() => {
+          root.render(<App step={1} />);
+        });
+      } else {
         root.render(<App step={1} />);
-      });
+      }
       await waitForAll(['Suspend! [A1]', 'Loading...']);
 
       // Lots of time elapses before the promise resolves

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

@@ -692,16 +692,24 @@ describe('ReactHooksWithNoopRenderer', () => {
       await waitForAll([0]);
       expect(root).toMatchRenderedOutput(<span prop={0} />);
 
-      React.startTransition(() => {
+      if (gate(flags => flags.enableSyncDefaultUpdates)) {
+        React.startTransition(() => {
+          root.render(<Foo signal={false} />);
+        });
+      } else {
         root.render(<Foo signal={false} />);
-      });
+      }
       await waitForAll(['Suspend!']);
       expect(root).toMatchRenderedOutput(<span prop={0} />);
 
       // Rendering again should suspend again.
-      React.startTransition(() => {
+      if (gate(flags => flags.enableSyncDefaultUpdates)) {
+        React.startTransition(() => {
+          root.render(<Foo signal={false} />);
+        });
+      } else {
         root.render(<Foo signal={false} />);
-      });
+      }
       await waitForAll(['Suspend!']);
     });
 
@@ -747,25 +755,38 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(root).toMatchRenderedOutput(<span prop="A:0" />);
 
       await act(async () => {
-        React.startTransition(() => {
+        if (gate(flags => flags.enableSyncDefaultUpdates)) {
+          React.startTransition(() => {
+            root.render(<Foo signal={false} />);
+            setLabel('B');
+          });
+        } else {
           root.render(<Foo signal={false} />);
           setLabel('B');
-        });
+        }
 
         await waitForAll(['Suspend!']);
         expect(root).toMatchRenderedOutput(<span prop="A:0" />);
 
         // Rendering again should suspend again.
-        React.startTransition(() => {
+        if (gate(flags => flags.enableSyncDefaultUpdates)) {
+          React.startTransition(() => {
+            root.render(<Foo signal={false} />);
+          });
+        } else {
           root.render(<Foo signal={false} />);
-        });
+        }
         await waitForAll(['Suspend!']);
 
         // Flip the signal back to "cancel" the update. However, the update to
         // label should still proceed. It shouldn't have been dropped.
-        React.startTransition(() => {
+        if (gate(flags => flags.enableSyncDefaultUpdates)) {
+          React.startTransition(() => {
+            root.render(<Foo signal={true} />);
+          });
+        } else {
           root.render(<Foo signal={true} />);
-        });
+        }
         await waitForAll(['B:0']);
         expect(root).toMatchRenderedOutput(<span prop="B:0" />);
       });

packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js

@@ -1414,12 +1414,10 @@ describe('ReactLazy', () => {
 
     // Swap the position of A and B
     root.update(<Parent swap={true} />);
-    await waitForAll([
-      'Init B2',
-      'Loading...',
-      'Did unmount: A',
-      'Did unmount: B',
-    ]);
+    await waitForAll(['Init B2', 'Loading...']);
+    jest.runAllTimers();
+
+    assertLog(['Did unmount: A', 'Did unmount: B']);
 
     // The suspense boundary should've triggered now.
     expect(root).toMatchRenderedOutput('Loading...');
@@ -1561,9 +1559,13 @@ describe('ReactLazy', () => {
     expect(root).toMatchRenderedOutput('AB');
 
     // Swap the position of A and B
-    React.startTransition(() => {
+    if (gate(flags => flags.enableSyncDefaultUpdates)) {
+      React.startTransition(() => {
+        root.update(<Parent swap={true} />);
+      });
+    } else {
       root.update(<Parent swap={true} />);
-    });
+    }
     await waitForAll(['Init B2', 'Loading...']);
     await resolveFakeImport(ChildB2);
     // We need to flush to trigger the second one to load.

packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js

@@ -9,7 +9,6 @@ let useState;
 let useEffect;
 let startTransition;
 let textCache;
-let waitFor;
 let waitForPaint;
 let assertLog;
 
@@ -29,7 +28,6 @@ describe('ReactOffscreen', () => {
     startTransition = React.startTransition;
 
     const InternalTestUtils = require('internal-test-utils');
-    waitFor = InternalTestUtils.waitFor;
     waitForPaint = InternalTestUtils.waitForPaint;
     assertLog = InternalTestUtils.assertLog;
 
@@ -409,6 +407,7 @@ describe('ReactOffscreen', () => {
     expect(root).toMatchRenderedOutput(<span hidden={true}>B1</span>);
   });
 
+  // Only works in new reconciler
   // @gate enableOffscreen
   test('detect updates to a hidden tree during a concurrent event', async () => {
     // This is a pretty complex test case. It relates to how we detect if an
@@ -443,17 +442,17 @@ describe('ReactOffscreen', () => {
       setOuter = _setOuter;
       return (
         <>
+          <span>
+            <Text text={'Outer: ' + outer} />
+          </span>
           <Offscreen mode={show ? 'visible' : 'hidden'}>
             <span>
               <Child outer={outer} />
             </span>
           </Offscreen>
-          <span>
-            <Text text={'Outer: ' + outer} />
-          </span>
           <Suspense fallback={<Text text="Loading..." />}>
             <span>
-              <Text text={'Sibling: ' + outer} />
+              <AsyncText text={'Async: ' + outer} />
             </span>
           </Suspense>
         </>
@@ -467,41 +466,50 @@ describe('ReactOffscreen', () => {
       root.render(<App show={true} />);
     });
     assertLog([
-      'Inner: 0',
       'Outer: 0',
-      'Sibling: 0',
+      'Inner: 0',
+      'Async: 0',
       'Inner and outer are consistent',
     ]);
     expect(root).toMatchRenderedOutput(
       <>
-        <span>Inner: 0</span>
         <span>Outer: 0</span>
-        <span>Sibling: 0</span>
+        <span>Inner: 0</span>
+        <span>Async: 0</span>
       </>,
     );
 
     await act(async () => {
       // Update a value both inside and outside the hidden tree. These values
       // must always be consistent.
-      startTransition(() => {
-        setOuter(1);
-        setInner(1);
-        // In the same render, also hide the offscreen tree.
-        root.render(<App show={false} />);
-      });
+      setOuter(1);
+      setInner(1);
+      // In the same render, also hide the offscreen tree.
+      root.render(<App show={false} />);
 
-      await waitFor([
+      await waitForPaint([
         // The outer update will commit, but the inner update is deferred until
         // a later render.
         'Outer: 1',
+
+        // Something suspended. This means we won't commit immediately; there
+        // will be an async gap between render and commit. In this test, we will
+        // use this property to schedule a concurrent update. The fact that
+        // we're using Suspense to schedule a concurrent update is not directly
+        // relevant to the test — we could also use time slicing, but I've
+        // chosen to use Suspense the because implementation details of time
+        // slicing are more volatile.
+        'Suspend! [Async: 1]',
+
+        'Loading...',
       ]);
 
       // Assert that we haven't committed quite yet
       expect(root).toMatchRenderedOutput(
         <>
-          <span>Inner: 0</span>
           <span>Outer: 0</span>
-          <span>Sibling: 0</span>
+          <span>Inner: 0</span>
+          <span>Async: 0</span>
         </>,
       );
 
@@ -512,13 +520,14 @@ describe('ReactOffscreen', () => {
         setInner(2);
       });
 
-      // Finish rendering and commit the in-progress render.
-      await waitForPaint(['Sibling: 1']);
+      // Commit the previous render.
+      jest.runAllTimers();
       expect(root).toMatchRenderedOutput(
         <>
-          <span hidden={true}>Inner: 0</span>
           <span>Outer: 1</span>
-          <span>Sibling: 1</span>
+          <span hidden={true}>Inner: 0</span>
+          <span hidden={true}>Async: 0</span>
+          Loading...
         </>,
       );
 
@@ -527,27 +536,32 @@ describe('ReactOffscreen', () => {
         root.render(<App show={true} />);
       });
       assertLog([
+        'Outer: 1',
+
         // There are two pending updates on Inner, but only the first one
         // is processed, even though they share the same lane. If the second
         // update were erroneously processed, then Inner would be inconsistent
         // with Outer.
         'Inner: 1',
-        'Outer: 1',
-        'Sibling: 1',
+
+        'Suspend! [Async: 1]',
+        'Loading...',
         'Inner and outer are consistent',
       ]);
     });
     assertLog([
-      'Inner: 2',
       'Outer: 2',
-      'Sibling: 2',
+      'Inner: 2',
+      'Suspend! [Async: 2]',
+      'Loading...',
       'Inner and outer are consistent',
     ]);
     expect(root).toMatchRenderedOutput(
       <>
-        <span>Inner: 2</span>
         <span>Outer: 2</span>
-        <span>Sibling: 2</span>
+        <span>Inner: 2</span>
+        <span hidden={true}>Async: 0</span>
+        Loading...
       </>,
     );
   });