From d1845ad0ff6c69ea377d710d4053d9bd65e0d0ff Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 10 Feb 2021 02:00:24 -0600 Subject: [PATCH] Default updates should not interrupt transitions (#20771) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/ReactFiberLane.new.js | 12 +- .../src/ReactFiberLane.old.js | 12 +- ...tIncrementalErrorHandling-test.internal.js | 6 +- .../src/__tests__/ReactTransition-test.js | 202 ++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 14 files changed, 239 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index adb67b6e77dfe..309a5335b14ef 100644 --- 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 index ea02f3102c902..c62fa08c6c30b 100644 --- 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 index 4cdb4410ba592..f568191681b67 100644 --- 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 index ffd2c9e19ebc7..25072eb465ad9 100644 --- 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 ( + }> + + {', '} + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + + // 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 ( + }> + {transitionPri ? : } + {', '} + + + ); + } + + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + }); + + // 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 ( + <> + + {', '} + + + ); + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + 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 index ebd12c335f842..7cdc95842f31e 100644 --- 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 index 3cf047f78eaf9..f79ea1e94881b 100644 --- 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 index 56582bc85f8c3..cdb661749bf28 100644 --- 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 index 64a7493b11eaf..c1d46ee7cb5b9 100644 --- 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 index e10accde90683..e7a7b260a5735 100644 --- 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 index 3a7336db3bfaa..df8da1a9f50d8 100644 --- 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 index 4adeafbf30171..48f85d5c689d5 100644 --- 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 index 88ce477eea3e5..db706abb2e8f5 100644 --- 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 index f0cb3c5eb1ca5..0d1adfc4d41db 100644 --- 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 index a2970b0a166d4..760d8ca725c11 100644 --- 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;