Default updates should not interrupt transitions (#20771)

The only difference between default updates and transition updates is that default updates do not support suspended refreshes — they will instantly display a fallback. Co-authored-by: Rick Hanlon <[email protected]>
facebook · Feb 10, 2021 · d1845ad · d1845ad
1 parent 3499c34
commit d1845ad
Show file tree

Hide file tree

Showing 14 changed files with 239 additions and 4 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js
@@ -39,6 +39,7 @@ import invariant from 'shared/invariant';
 import {
   enableCache,
   enableTransitionEntanglement,
+  enableNonInterruptingNormalPri,
 } from 'shared/ReactFeatureFlags';
 
 import {
@@ -327,7 +328,16 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
   ) {
     getHighestPriorityLanes(wipLanes);
     const wipLanePriority = return_highestLanePriority;
-    if (nextLanePriority <= wipLanePriority) {
+    if (
+      nextLanePriority <= wipLanePriority ||
+      // Default priority updates should not interrupt transition updates. The
+      // only difference between default updates and transition updates is that
+      // default updates do not support refresh transitions.
+      (enableNonInterruptingNormalPri &&
+        nextLanePriority === DefaultLanePriority &&
+        wipLanePriority === TransitionPriority)
+    ) {
+      // Keep working on the existing in-progress tree. Do not interrupt.
       return wipLanes;
     } else {
       return_highestLanePriority = nextLanePriority;

diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js
@@ -39,6 +39,7 @@ import invariant from 'shared/invariant';
 import {
   enableCache,
   enableTransitionEntanglement,
+  enableNonInterruptingNormalPri,
 } from 'shared/ReactFeatureFlags';
 
 import {
@@ -327,7 +328,16 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
   ) {
     getHighestPriorityLanes(wipLanes);
     const wipLanePriority = return_highestLanePriority;
-    if (nextLanePriority <= wipLanePriority) {
+    if (
+      nextLanePriority <= wipLanePriority ||
+      // Default priority updates should not interrupt transition updates. The
+      // only difference between default updates and transition updates is that
+      // default updates do not support refresh transitions.
+      (enableNonInterruptingNormalPri &&
+        nextLanePriority === DefaultLanePriority &&
+        wipLanePriority === TransitionPriority)
+    ) {
+      // Keep working on the existing in-progress tree. Do not interrupt.
       return wipLanes;
     } else {
       return_highestLanePriority = nextLanePriority;

diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js
@@ -1849,10 +1849,12 @@ describe('ReactIncrementalErrorHandling', () => {
       // the queue.
       expect(Scheduler).toFlushAndYieldThrough(['Everything is fine.']);
 
-      // Schedule a default pri update on a child that triggers an error.
+      // Schedule a discrete update on a child that triggers an error.
       // The root should capture this error. But since there's still a pending
       // update on the root, the error should be suppressed.
-      setShouldThrow(true);
+      ReactNoop.discreteUpdates(() => {
+        setShouldThrow(true);
+      });
     });
     // Should render the final state without throwing the error.
     expect(Scheduler).toHaveYielded(['Everything is fine.']);

diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.js
@@ -15,6 +15,7 @@ let ReactNoop;
 let Scheduler;
 let Suspense;
 let useState;
+let useLayoutEffect;
 let useTransition;
 let startTransition;
 let act;
@@ -30,6 +31,7 @@ describe('ReactTransition', () => {
     ReactNoop = require('react-noop-renderer');
     Scheduler = require('scheduler');
     useState = React.useState;
+    useLayoutEffect = React.useLayoutEffect;
     useTransition = React.unstable_useTransition;
     Suspense = React.Suspense;
     startTransition = React.unstable_startTransition;
@@ -773,4 +775,204 @@ describe('ReactTransition', () => {
       });
     },
   );
+
+  // @gate experimental
+  // @gate enableCache
+  it('should render normal pri updates scheduled after transitions before transitions', async () => {
+    let updateTransitionPri;
+    let updateNormalPri;
+    function App() {
+      const [normalPri, setNormalPri] = useState(0);
+      const [transitionPri, setTransitionPri] = useState(0);
+      updateTransitionPri = () =>
+        startTransition(() => setTransitionPri(n => n + 1));
+      updateNormalPri = () => setNormalPri(n => n + 1);
+
+      useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <Text text={'Transition pri: ' + transitionPri} />
+          {', '}
+          <Text text={'Normal pri: ' + normalPri} />
+        </Suspense>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(<App />);
+    });
+
+    // Initial render.
+    expect(Scheduler).toHaveYielded([
+      'Transition pri: 0',
+      'Normal pri: 0',
+      'Commit',
+    ]);
+    expect(root).toMatchRenderedOutput('Transition pri: 0, Normal pri: 0');
+
+    await act(async () => {
+      updateTransitionPri();
+      updateNormalPri();
+    });
+
+    expect(Scheduler).toHaveYielded([
+      // Normal update first.
+      'Transition pri: 0',
+      'Normal pri: 1',
+      'Commit',
+
+      // Then transition update.
+      'Transition pri: 1',
+      'Normal pri: 1',
+      'Commit',
+    ]);
+    expect(root).toMatchRenderedOutput('Transition pri: 1, Normal pri: 1');
+  });
+
+  // @gate experimental
+  // @gate enableCache
+  it('should render normal pri updates before transition suspense retries', async () => {
+    let updateTransitionPri;
+    let updateNormalPri;
+    function App() {
+      const [transitionPri, setTransitionPri] = useState(false);
+      const [normalPri, setNormalPri] = useState(0);
+
+      updateTransitionPri = () => startTransition(() => setTransitionPri(true));
+      updateNormalPri = () => setNormalPri(n => n + 1);
+
+      useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          {transitionPri ? <AsyncText text="Async" /> : <Text text="(empty)" />}
+          {', '}
+          <Text text={'Normal pri: ' + normalPri} />
+        </Suspense>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(<App />);
+    });
+
+    // Initial render.
+    expect(Scheduler).toHaveYielded(['(empty)', 'Normal pri: 0', 'Commit']);
+    expect(root).toMatchRenderedOutput('(empty), Normal pri: 0');
+
+    await act(async () => {
+      updateTransitionPri();
+    });
+
+    expect(Scheduler).toHaveYielded([
+      // Suspend.
+      'Suspend! [Async]',
+      'Normal pri: 0',
+      'Loading...',
+    ]);
+    expect(root).toMatchRenderedOutput('(empty), Normal pri: 0');
+
+    await act(async () => {
+      await resolveText('Async');
+      updateNormalPri();
+    });
+
+    expect(Scheduler).toHaveYielded([
+      // Normal pri update.
+      '(empty)',
+      'Normal pri: 1',
+      'Commit',
+
+      // Promise resolved, retry flushed.
+      'Async',
+      'Normal pri: 1',
+      'Commit',
+    ]);
+    expect(root).toMatchRenderedOutput('Async, Normal pri: 1');
+  });
+
+  // @gate experimental
+  // @gate enableCache
+  it('should not interrupt transitions with normal pri updates', async () => {
+    let updateNormalPri;
+    let updateTransitionPri;
+    function App() {
+      const [transitionPri, setTransitionPri] = useState(0);
+      const [normalPri, setNormalPri] = useState(0);
+      updateTransitionPri = () =>
+        startTransition(() => setTransitionPri(n => n + 1));
+      updateNormalPri = () => setNormalPri(n => n + 1);
+
+      useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+      return (
+        <>
+          <Text text={'Transition pri: ' + transitionPri} />
+          {', '}
+          <Text text={'Normal pri: ' + normalPri} />
+        </>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await ReactNoop.act(async () => {
+      root.render(<App />);
+    });
+    expect(Scheduler).toHaveYielded([
+      'Transition pri: 0',
+      'Normal pri: 0',
+      'Commit',
+    ]);
+    expect(root).toMatchRenderedOutput('Transition pri: 0, Normal pri: 0');
+
+    await ReactNoop.act(async () => {
+      updateTransitionPri();
+
+      expect(Scheduler).toFlushAndYieldThrough([
+        // Start transition update.
+        'Transition pri: 1',
+      ]);
+
+      // Schedule normal pri update during transition update.
+      // This should not interrupt.
+      updateNormalPri();
+    });
+
+    if (gate(flags => flags.enableNonInterruptingNormalPri)) {
+      expect(Scheduler).toHaveYielded([
+        // Finish transition update.
+        'Normal pri: 0',
+        'Commit',
+
+        // Normal pri update.
+        'Transition pri: 1',
+        'Normal pri: 1',
+        'Commit',
+      ]);
+
+      expect(root).toMatchRenderedOutput('Transition pri: 1, Normal pri: 1');
+    } else {
+      expect(Scheduler).toHaveYielded([
+        // Interrupt! Render normal pri update.
+        'Transition pri: 0',
+        'Normal pri: 1',
+        'Commit',
+
+        // Restart transition update.
+        'Transition pri: 1',
+        'Normal pri: 1',
+        'Commit',
+      ]);
+
+      expect(root).toMatchRenderedOutput('Transition pri: 1, Normal pri: 1');
+    }
+  });
 });
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -150,6 +150,8 @@ export const disableSchedulerTimeoutInWorkLoop = false;
 // Experiment to simplify/improve how transitions are scheduled
 export const enableTransitionEntanglement = false;
 
+export const enableNonInterruptingNormalPri = false;
+
 export const enableDiscreteEventMicroTasks = false;
 
 export const enableNativeEventPriorityInference = false;
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -58,6 +58,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js
@@ -57,6 +57,7 @@ export const enableUseRefAccessWarning = false;
 export const enableRecursiveCommitTraversal = false;
 export const disableSchedulerTimeoutInWorkLoop = false;
 export const enableTransitionEntanglement = false;
+export const enableNonInterruptingNormalPri = false;
 export const enableDiscreteEventMicroTasks = false;
 export const enableNativeEventPriorityInference = false;
 

diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -56,5 +56,6 @@ export const enableUseRefAccessWarning = __VARIANT__;
 export const enableProfilerNestedUpdateScheduledHook = __VARIANT__;
 export const disableSchedulerTimeoutInWorkLoop = __VARIANT__;
 export const enableTransitionEntanglement = __VARIANT__;
+export const enableNonInterruptingNormalPri = __VARIANT__;
 export const enableDiscreteEventMicroTasks = __VARIANT__;
 export const enableNativeEventPriorityInference = __VARIANT__;
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -32,6 +32,7 @@ export const {
   disableNativeComponentFrames,
   disableSchedulerTimeoutInWorkLoop,
   enableTransitionEntanglement,
+  enableNonInterruptingNormalPri,
   enableDiscreteEventMicroTasks,
   enableNativeEventPriorityInference,
 } = dynamicFeatureFlags;