From a63b8ff98710e0595da5fc6c666e72923c9f157c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Apr 2021 16:50:19 -0500 Subject: [PATCH] Continuous updates should interrupt transitions Even when updates are sync by default. Discovered this quirk while working on #21322. Previously, when sync default updates are enabled, continuous updates are treated like default updates. We implemented this by assigning DefaultLane to continous updates. However, an unintended consequence of that approach is that continuous updates would no longer interrupt transitions, because default updates are not supposed to interrupt transitions. To fix this, I changed the implementation to always assign separate lanes for default and continuous updates. Then I entangle the lanes together. --- .../src/ReactFiberLane.new.js | 29 ++++++++-- .../src/ReactFiberLane.old.js | 29 ++++++++-- .../src/ReactFiberWorkLoop.new.js | 31 ++--------- .../src/ReactFiberWorkLoop.old.js | 31 ++--------- .../ReactHooksWithNoopRenderer-test.js | 12 ----- .../src/__tests__/ReactUpdatePriority-test.js | 54 +++++++++++++++++++ .../SchedulingProfilerLabels-test.internal.js | 18 ++----- 7 files changed, 121 insertions(+), 83 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index ced32541ee8df..501af9e8b12a7 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -39,6 +39,7 @@ import { enableCache, enableSchedulingProfiler, enableUpdaterTracking, + enableSyncDefaultUpdates, } from 'shared/ReactFeatureFlags'; import {isDevToolsPresent} from './ReactFiberDevToolsHook.new'; @@ -263,9 +264,6 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { // 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. - // TODO: This applies to sync default updates, too. Which is probably what - // we want for default priority events, but not for continuous priority - // events like hover. (nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes) ) { // Keep working on the existing in-progress tree. Do not interrupt. @@ -273,6 +271,18 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { } } + if ( + // TODO: Check for root override, once that lands + enableSyncDefaultUpdates && + (nextLanes & InputContinuousLane) !== NoLanes + ) { + // When updates are sync by default, we entangle continous priority updates + // and default updates, so they render in the same batch. The only reason + // they use separate lanes is because continuous updates should interrupt + // transitions, but default updates should not. + nextLanes |= pendingLanes & DefaultLane; + } + // Check for entangled lanes and add them to the batch. // // A lane is said to be entangled with another when it's not allowed to render @@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) { return (lanes & TransitionLanes) === lanes; } +export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) { + if (!enableSyncDefaultUpdates) { + return true; + } + const SyncDefaultLanes = + InputContinuousHydrationLane | + InputContinuousLane | + DefaultHydrationLane | + DefaultLane; + // TODO: Check for root override, once that lands + return (lanes & SyncDefaultLanes) === NoLanes; +} + export function isTransitionLane(lane: Lane) { return (lane & TransitionLanes) !== 0; } diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index 5af142edbbae9..701d4c46cf727 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -39,6 +39,7 @@ import { enableCache, enableSchedulingProfiler, enableUpdaterTracking, + enableSyncDefaultUpdates, } from 'shared/ReactFeatureFlags'; import {isDevToolsPresent} from './ReactFiberDevToolsHook.old'; @@ -263,9 +264,6 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { // 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. - // TODO: This applies to sync default updates, too. Which is probably what - // we want for default priority events, but not for continuous priority - // events like hover. (nextLane === DefaultLane && (wipLane & TransitionLanes) !== NoLanes) ) { // Keep working on the existing in-progress tree. Do not interrupt. @@ -273,6 +271,18 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes { } } + if ( + // TODO: Check for root override, once that lands + enableSyncDefaultUpdates && + (nextLanes & InputContinuousLane) !== NoLanes + ) { + // When updates are sync by default, we entangle continous priority updates + // and default updates, so they render in the same batch. The only reason + // they use separate lanes is because continuous updates should interrupt + // transitions, but default updates should not. + nextLanes |= pendingLanes & DefaultLane; + } + // Check for entangled lanes and add them to the batch. // // A lane is said to be entangled with another when it's not allowed to render @@ -467,6 +477,19 @@ export function includesOnlyTransitions(lanes: Lanes) { return (lanes & TransitionLanes) === lanes; } +export function shouldTimeSlice(root: FiberRoot, lanes: Lanes) { + if (!enableSyncDefaultUpdates) { + return true; + } + const SyncDefaultLanes = + InputContinuousHydrationLane | + InputContinuousLane | + DefaultHydrationLane | + DefaultLane; + // TODO: Check for root override, once that lands + return (lanes & SyncDefaultLanes) === NoLanes; +} + export function isTransitionLane(lane: Lane) { return (lane & TransitionLanes) !== 0; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 665df80b13347..9ebace0c72010 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -32,7 +32,6 @@ import { disableSchedulerTimeoutInWorkLoop, enableStrictEffects, skipUnmountedBoundaries, - enableSyncDefaultUpdates, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; @@ -139,10 +138,6 @@ import { NoLanes, NoLane, SyncLane, - DefaultLane, - DefaultHydrationLane, - InputContinuousLane, - InputContinuousHydrationLane, NoTimestamp, claimNextTransitionLane, claimNextRetryLane, @@ -154,6 +149,7 @@ import { includesNonIdleWork, includesOnlyRetries, includesOnlyTransitions, + shouldTimeSlice, getNextLanes, markStarvedLanesAsExpired, getLanesToRetrySynchronouslyOnError, @@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane { // TODO: Move this type conversion to the event priority module. const updateLane: Lane = (getCurrentUpdatePriority(): any); if (updateLane !== NoLane) { - if ( - enableSyncDefaultUpdates && - (updateLane === InputContinuousLane || - updateLane === InputContinuousHydrationLane) - ) { - return DefaultLane; - } return updateLane; } @@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane { // use that directly. // TODO: Move this type conversion to the event priority module. const eventLane: Lane = (getCurrentEventPriority(): any); - if ( - enableSyncDefaultUpdates && - (eventLane === InputContinuousLane || - eventLane === InputContinuousHydrationLane) - ) { - return DefaultLane; - } return eventLane; } @@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } - let exitStatus = - enableSyncDefaultUpdates && - (includesSomeLane(lanes, DefaultLane) || - includesSomeLane(lanes, DefaultHydrationLane)) - ? // Time slicing is disabled for default updates in this root. - renderRootSync(root, lanes) - : renderRootConcurrent(root, lanes); + let exitStatus = shouldTimeSlice(root, lanes) + ? renderRootConcurrent(root, lanes) + : // Time slicing is disabled for default updates in this root. + renderRootSync(root, lanes); if (exitStatus !== RootIncomplete) { if (exitStatus === RootErrored) { executionContext |= RetryAfterError; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index baebee3dc0490..09a116c97bd7d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -32,7 +32,6 @@ import { disableSchedulerTimeoutInWorkLoop, enableStrictEffects, skipUnmountedBoundaries, - enableSyncDefaultUpdates, enableUpdaterTracking, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; @@ -139,10 +138,6 @@ import { NoLanes, NoLane, SyncLane, - DefaultLane, - DefaultHydrationLane, - InputContinuousLane, - InputContinuousHydrationLane, NoTimestamp, claimNextTransitionLane, claimNextRetryLane, @@ -154,6 +149,7 @@ import { includesNonIdleWork, includesOnlyRetries, includesOnlyTransitions, + shouldTimeSlice, getNextLanes, markStarvedLanesAsExpired, getLanesToRetrySynchronouslyOnError, @@ -437,13 +433,6 @@ export function requestUpdateLane(fiber: Fiber): Lane { // TODO: Move this type conversion to the event priority module. const updateLane: Lane = (getCurrentUpdatePriority(): any); if (updateLane !== NoLane) { - if ( - enableSyncDefaultUpdates && - (updateLane === InputContinuousLane || - updateLane === InputContinuousHydrationLane) - ) { - return DefaultLane; - } return updateLane; } @@ -454,13 +443,6 @@ export function requestUpdateLane(fiber: Fiber): Lane { // use that directly. // TODO: Move this type conversion to the event priority module. const eventLane: Lane = (getCurrentEventPriority(): any); - if ( - enableSyncDefaultUpdates && - (eventLane === InputContinuousLane || - eventLane === InputContinuousHydrationLane) - ) { - return DefaultLane; - } return eventLane; } @@ -811,13 +793,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) { return null; } - let exitStatus = - enableSyncDefaultUpdates && - (includesSomeLane(lanes, DefaultLane) || - includesSomeLane(lanes, DefaultHydrationLane)) - ? // Time slicing is disabled for default updates in this root. - renderRootSync(root, lanes) - : renderRootConcurrent(root, lanes); + let exitStatus = shouldTimeSlice(root, lanes) + ? renderRootConcurrent(root, lanes) + : // Time slicing is disabled for default updates in this root. + renderRootSync(root, lanes); if (exitStatus !== RootIncomplete) { if (exitStatus === RootErrored) { executionContext |= RetryAfterError; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 485d477ed7e8e..07d3f7ff526aa 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -1405,18 +1405,6 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => { setParentState(false); }); - if (gate(flags => flags.enableSyncDefaultUpdates)) { - // TODO: Default updates do not interrupt transition updates, to - // prevent starvation. However, when sync default updates are enabled, - // continuous updates are treated like default updates. In this case, - // we probably don't want this behavior; continuous should be allowed - // to interrupt. - expect(Scheduler).toFlushUntilNextPaint([ - 'Child two render', - 'Child one commit', - 'Child two commit', - ]); - } expect(Scheduler).toFlushUntilNextPaint([ 'Parent false render', 'Parent false commit', diff --git a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js index 0aee7df66df4a..116cc4c4d8e00 100644 --- a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js +++ b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js @@ -1,6 +1,8 @@ let React; let ReactNoop; let Scheduler; +let ContinuousEventPriority; +let startTransition; let useState; let useEffect; @@ -11,6 +13,9 @@ describe('ReactUpdatePriority', () => { React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); + ContinuousEventPriority = require('react-reconciler/constants') + .ContinuousEventPriority; + startTransition = React.unstable_startTransition; useState = React.useState; useEffect = React.useEffect; }); @@ -78,4 +83,53 @@ describe('ReactUpdatePriority', () => { // Now the idle update has flushed expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']); }); + + // @gate experimental + test('continuous updates should interrupt transisions', async () => { + const root = ReactNoop.createRoot(); + + let setCounter; + let setIsHidden; + function App() { + const [counter, _setCounter] = useState(1); + const [isHidden, _setIsHidden] = useState(false); + setCounter = _setCounter; + setIsHidden = _setIsHidden; + if (isHidden) { + return ; + } + return ( + <> + + + + + ); + } + + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['A1', 'B1', 'C1']); + expect(root).toMatchRenderedOutput('A1B1C1'); + + await ReactNoop.act(async () => { + startTransition(() => { + setCounter(2); + }); + expect(Scheduler).toFlushAndYieldThrough(['A2']); + ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => { + setIsHidden(true); + }); + }); + expect(Scheduler).toHaveYielded([ + // Because the hide update has continous priority, it should interrupt the + // in-progress transition + '(hidden)', + // When the transition resumes, it's a no-op because the children are + // now hidden. + '(hidden)', + ]); + expect(root).toMatchRenderedOutput('(hidden)'); + }); }); diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js index da59c153f43dd..6474be855b38f 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfilerLabels-test.internal.js @@ -168,18 +168,10 @@ describe('SchedulingProfiler labels', () => { event.initEvent('mouseover', true, true); dispatchAndSetCurrentEvent(targetRef.current, event); }); - if (gate(flags => flags.enableSyncDefaultUpdates)) { - expect(clearedMarks).toContain( - `--schedule-state-update-${formatLanes( - ReactFiberLane.DefaultLane, - )}-App`, - ); - } else { - expect(clearedMarks).toContain( - `--schedule-state-update-${formatLanes( - ReactFiberLane.InputContinuousLane, - )}-App`, - ); - } + expect(clearedMarks).toContain( + `--schedule-state-update-${formatLanes( + ReactFiberLane.InputContinuousLane, + )}-App`, + ); }); });