Don't group Idle/Offscreen work with other work (#17456)

When we suspend we always try a lower level but we shouldn't try offscreen.
facebook · Dec 3, 2019 · dc18b8b · dc18b8b
1 parent f523b2e
commit dc18b8b
Show file tree

Hide file tree

Showing 9 changed files with 137 additions and 5 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -25,6 +25,7 @@ import {
   warnAboutUnmockedScheduler,
   flushSuspenseFallbacksInTests,
   disableSchedulerTimeoutBasedOnReactExpirationTime,
+  enableTrainModelFix,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import invariant from 'shared/invariant';
@@ -539,9 +540,19 @@ function getNextRootExpirationTimeToWorkOn(root: FiberRoot): ExpirationTime {
   // on whichever is higher priority.
   const lastPingedTime = root.lastPingedTime;
   const nextKnownPendingLevel = root.nextKnownPendingLevel;
-  return lastPingedTime > nextKnownPendingLevel
-    ? lastPingedTime
-    : nextKnownPendingLevel;
+  const nextLevel =
+    lastPingedTime > nextKnownPendingLevel
+      ? lastPingedTime
+      : nextKnownPendingLevel;
+  if (
+    enableTrainModelFix &&
+    nextLevel <= Idle &&
+    firstPendingTime !== nextLevel
+  ) {
+    // Don't work on Idle/Never priority unless everything else is committed.
+    return NoWork;
+  }
+  return nextLevel;
 }
 
 // Use this function to schedule a task for a root. There's only one task per
@@ -2362,7 +2373,7 @@ export function pingSuspendedRoot(
   // Mark the time at which this ping was scheduled.
   root.lastPingedTime = suspendedTime;
 
-  if (root.finishedExpirationTime === suspendedTime) {
+  if (!enableTrainModelFix && root.finishedExpirationTime === suspendedTime) {
     // If there's a pending fallback waiting to commit, throw it away.
     root.finishedExpirationTime = NoWork;
     root.finishedWork = null;

diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
@@ -2222,7 +2222,8 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () =>
       ReactNoop.render(<Foo renderContent={2} />),
     );
-    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']);
+    // We won't even work on Idle priority.
+    expect(Scheduler).toFlushAndYield([]);
 
     // We're still suspended.
     expect(ReactNoop.getChildren()).toEqual([]);
@@ -2789,4 +2790,116 @@ describe('ReactSuspenseWithNoopRenderer', () => {
 
     expect(root).toMatchRenderedOutput(<span prop="Foo" />);
   });
+
+  it('should not render hidden content while suspended on higher pri', async () => {
+    function Offscreen() {
+      Scheduler.unstable_yieldValue('Offscreen');
+      return 'Offscreen';
+    }
+    function App({showContent}) {
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+      return (
+        <>
+          <div hidden={true}>
+            <Offscreen />
+          </div>
+          <Suspense fallback={<Text text="Loading..." />}>
+            {showContent ? <AsyncText text="A" ms={2000} /> : null}
+          </Suspense>
+        </>
+      );
+    }
+
+    // Initial render.
+    ReactNoop.render(<App showContent={false} />);
+    expect(Scheduler).toFlushAndYieldThrough(['Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);
+
+    // Start transition.
+    React.unstable_withSuspenseConfig(
+      () => {
+        ReactNoop.render(<App showContent={true} />);
+      },
+      {timeoutMs: 2000},
+    );
+
+    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
+    Scheduler.unstable_advanceTime(2000);
+    await advanceTimers(2000);
+    expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
+    expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true} />
+        <span prop="A" />
+      </>,
+    );
+    expect(Scheduler).toFlushAndYield(['Offscreen']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true}>Offscreen</div>
+        <span prop="A" />
+      </>,
+    );
+  });
+
+  it('should be able to unblock higher pri content before suspended hidden', async () => {
+    function Offscreen() {
+      Scheduler.unstable_yieldValue('Offscreen');
+      return 'Offscreen';
+    }
+    function App({showContent}) {
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <div hidden={true}>
+            <AsyncText text="A" ms={2000} />
+            <Offscreen />
+          </div>
+          {showContent ? <AsyncText text="A" ms={2000} /> : null}
+        </Suspense>
+      );
+    }
+
+    // Initial render.
+    ReactNoop.render(<App showContent={false} />);
+    expect(Scheduler).toFlushAndYieldThrough(['Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);
+
+    // Partially render through the hidden content.
+    expect(Scheduler).toFlushAndYieldThrough(['Suspend! [A]']);
+
+    // Start transition.
+    React.unstable_withSuspenseConfig(
+      () => {
+        ReactNoop.render(<App showContent={true} />);
+      },
+      {timeoutMs: 5000},
+    );
+
+    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
+    Scheduler.unstable_advanceTime(2000);
+    await advanceTimers(2000);
+    expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
+    expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true} />
+        <span prop="A" />
+      </>,
+    );
+    expect(Scheduler).toFlushAndYield(['A', 'Offscreen']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true}>
+          <span prop="A" />Offscreen
+        </div>
+        <span prop="A" />
+      </>,
+    );
+  });
 });
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -88,6 +88,8 @@ export const disableLegacyContext = false;
 
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
+export const enableTrainModelFix = __EXPERIMENTAL__;
+
 export const enableTrustedTypesIntegration = false;
 
 // Flag to turn event.target and event.currentTarget in ReactNative from a reactTag to a component instance

diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -42,6 +42,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 
 // Only used in www builds.

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js
@@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -34,6 +34,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -16,6 +16,7 @@ export const {
   disableInputAttributeSyncing,
   enableTrustedTypesIntegration,
   enableSelectiveHydration,
+  enableTrainModelFix,
 } = require('ReactFeatureFlags');
 
 // In www, we have experimental support for gathering data