diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 46c6b5af0e96f..cc5a8b00563b2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -2029,10 +2029,11 @@ function dispatchAction( // Entangle the new transition lane with the other transition lanes. const newQueueLanes = mergeLanes(queueLanes, lane); - if (newQueueLanes !== queueLanes) { - queue.lanes = newQueueLanes; - markRootEntangled(root, newQueueLanes); - } + queue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); } } diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 17038f14b0904..b5eefde63f715 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -2029,10 +2029,11 @@ function dispatchAction( // Entangle the new transition lane with the other transition lanes. const newQueueLanes = mergeLanes(queueLanes, lane); - if (newQueueLanes !== queueLanes) { - queue.lanes = newQueueLanes; - markRootEntangled(root, newQueueLanes); - } + queue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); } } diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index afa54b7b7b117..315541115c37e 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -36,7 +36,10 @@ export type Lane = number; export type LaneMap = Array; import invariant from 'shared/invariant'; -import {enableCache} from 'shared/ReactFeatureFlags'; +import { + enableCache, + enableTransitionEntanglement, +} from 'shared/ReactFeatureFlags'; import { ImmediatePriority as ImmediateSchedulerPriority, @@ -92,11 +95,12 @@ export const DefaultLanes: Lanes = /* */ 0b0000000000000000000 const TransitionHydrationLane: Lane = /* */ 0b0000000000000000001000000000000; const TransitionLanes: Lanes = /* */ 0b0000000001111111110000000000000; -const SomeTransitionLane: Lane = /* */ 0b0000000000000000010000000000000; +const FirstTransitionLane: Lane = /* */ 0b0000000000000000010000000000000; const RetryLanes: Lanes = /* */ 0b0000011110000000000000000000000; -export const SomeRetryLane: Lanes = /* */ 0b0000010000000000000000000000000; +const FirstRetryLane: Lanes = /* */ 0b0000000010000000000000000000000; +export const SomeRetryLane: Lane = FirstRetryLane; export const SelectiveHydrationLane: Lane = /* */ 0b0000100000000000000000000000000; @@ -111,8 +115,8 @@ export const NoTimestamp = -1; let currentUpdateLanePriority: LanePriority = NoLanePriority; -let nextTransitionLane: Lane = SomeTransitionLane; -let nextRetryLane: Lane = SomeRetryLane; +let nextTransitionLane: Lane = FirstTransitionLane; +let nextRetryLane: Lane = FirstRetryLane; export function getCurrentUpdateLanePriority(): LanePriority { return currentUpdateLanePriority; @@ -498,56 +502,88 @@ export function findUpdateLane( lanePriority: LanePriority, wipLanes: Lanes, ): Lane { - switch (lanePriority) { - case NoLanePriority: - break; - case SyncLanePriority: - return SyncLane; - case SyncBatchedLanePriority: - return SyncBatchedLane; - case InputDiscreteLanePriority: { - const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes); - if (lane === NoLane) { - // Shift to the next priority level - return findUpdateLane(InputContinuousLanePriority, wipLanes); + if (enableTransitionEntanglement) { + // Ignore wipLanes. Always assign to the same bit per priority. + switch (lanePriority) { + case NoLanePriority: + break; + case SyncLanePriority: + return SyncLane; + case SyncBatchedLanePriority: + return SyncBatchedLane; + case InputDiscreteLanePriority: { + return pickArbitraryLane(InputDiscreteLanes); } - return lane; - } - case InputContinuousLanePriority: { - const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes); - if (lane === NoLane) { - // Shift to the next priority level - return findUpdateLane(DefaultLanePriority, wipLanes); + case InputContinuousLanePriority: { + return pickArbitraryLane(InputContinuousLanes); + } + case DefaultLanePriority: { + return pickArbitraryLane(DefaultLanes); } - return lane; + case TransitionPriority: // Should be handled by findTransitionLane instead + case RetryLanePriority: // Should be handled by findRetryLane instead + break; + case IdleLanePriority: + return pickArbitraryLane(IdleLanes); + default: + // The remaining priorities are not valid for updates + break; } - case DefaultLanePriority: { - let lane = pickArbitraryLane(DefaultLanes & ~wipLanes); - if (lane === NoLane) { - // If all the default lanes are already being worked on, look for a - // lane in the transition range. - lane = pickArbitraryLane(TransitionLanes & ~wipLanes); + } else { + // Old behavior that uses wipLanes to shift interleaved updates into a + // separate lane. This is no longer needed because we put interleaved + // updates on a special queue. + switch (lanePriority) { + case NoLanePriority: + break; + case SyncLanePriority: + return SyncLane; + case SyncBatchedLanePriority: + return SyncBatchedLane; + case InputDiscreteLanePriority: { + const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes); if (lane === NoLane) { - // All the transition lanes are taken, too. This should be very - // rare, but as a last resort, pick a default lane. This will have - // the effect of interrupting the current work-in-progress render. - lane = pickArbitraryLane(DefaultLanes); + // Shift to the next priority level + return findUpdateLane(InputContinuousLanePriority, wipLanes); } + return lane; } - return lane; - } - case TransitionPriority: // Should be handled by findTransitionLane instead - case RetryLanePriority: // Should be handled by findRetryLane instead - break; - case IdleLanePriority: - let lane = pickArbitraryLane(IdleLanes & ~wipLanes); - if (lane === NoLane) { - lane = pickArbitraryLane(IdleLanes); + case InputContinuousLanePriority: { + const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes); + if (lane === NoLane) { + // Shift to the next priority level + return findUpdateLane(DefaultLanePriority, wipLanes); + } + return lane; } - return lane; - default: - // The remaining priorities are not valid for updates - break; + case DefaultLanePriority: { + let lane = pickArbitraryLane(DefaultLanes & ~wipLanes); + if (lane === NoLane) { + // If all the default lanes are already being worked on, look for a + // lane in the transition range. + lane = pickArbitraryLane(TransitionLanes & ~wipLanes); + if (lane === NoLane) { + // All the transition lanes are taken, too. This should be very + // rare, but as a last resort, pick a default lane. This will have + // the effect of interrupting the current work-in-progress render. + lane = pickArbitraryLane(DefaultLanes); + } + } + return lane; + } + case TransitionPriority: // Should be handled by findTransitionLane instead + case RetryLanePriority: // Should be handled by findRetryLane instead + break; + case IdleLanePriority: + let lane = pickArbitraryLane(IdleLanes & ~wipLanes); + if (lane === NoLane) { + lane = pickArbitraryLane(IdleLanes); + } + return lane; + default: + // The remaining priorities are not valid for updates + break; + } } invariant( false, @@ -563,7 +599,7 @@ export function claimNextTransitionLane(): Lane { const lane = nextTransitionLane; nextTransitionLane <<= 1; if ((nextTransitionLane & TransitionLanes) === 0) { - nextTransitionLane = SomeTransitionLane; + nextTransitionLane = FirstTransitionLane; } return lane; } @@ -572,7 +608,7 @@ export function claimNextRetryLane(): Lane { const lane = nextRetryLane; nextRetryLane <<= 1; if ((nextRetryLane & RetryLanes) === 0) { - nextRetryLane = SomeRetryLane; + nextRetryLane = FirstRetryLane; } return lane; } diff --git a/packages/react-reconciler/src/ReactFiberLane.old.js b/packages/react-reconciler/src/ReactFiberLane.old.js index 3a0cdc3af2b6b..e34c8ac0514fc 100644 --- a/packages/react-reconciler/src/ReactFiberLane.old.js +++ b/packages/react-reconciler/src/ReactFiberLane.old.js @@ -36,7 +36,10 @@ export type Lane = number; export type LaneMap = Array; import invariant from 'shared/invariant'; -import {enableCache} from 'shared/ReactFeatureFlags'; +import { + enableCache, + enableTransitionEntanglement, +} from 'shared/ReactFeatureFlags'; import { ImmediatePriority as ImmediateSchedulerPriority, @@ -92,11 +95,12 @@ export const DefaultLanes: Lanes = /* */ 0b0000000000000000000 const TransitionHydrationLane: Lane = /* */ 0b0000000000000000001000000000000; const TransitionLanes: Lanes = /* */ 0b0000000001111111110000000000000; -const SomeTransitionLane: Lane = /* */ 0b0000000000000000010000000000000; +const FirstTransitionLane: Lane = /* */ 0b0000000000000000010000000000000; const RetryLanes: Lanes = /* */ 0b0000011110000000000000000000000; -export const SomeRetryLane: Lanes = /* */ 0b0000010000000000000000000000000; +const FirstRetryLane: Lanes = /* */ 0b0000000010000000000000000000000; +export const SomeRetryLane: Lane = FirstRetryLane; export const SelectiveHydrationLane: Lane = /* */ 0b0000100000000000000000000000000; @@ -111,8 +115,8 @@ export const NoTimestamp = -1; let currentUpdateLanePriority: LanePriority = NoLanePriority; -let nextTransitionLane: Lane = SomeTransitionLane; -let nextRetryLane: Lane = SomeRetryLane; +let nextTransitionLane: Lane = FirstTransitionLane; +let nextRetryLane: Lane = FirstRetryLane; export function getCurrentUpdateLanePriority(): LanePriority { return currentUpdateLanePriority; @@ -498,56 +502,88 @@ export function findUpdateLane( lanePriority: LanePriority, wipLanes: Lanes, ): Lane { - switch (lanePriority) { - case NoLanePriority: - break; - case SyncLanePriority: - return SyncLane; - case SyncBatchedLanePriority: - return SyncBatchedLane; - case InputDiscreteLanePriority: { - const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes); - if (lane === NoLane) { - // Shift to the next priority level - return findUpdateLane(InputContinuousLanePriority, wipLanes); + if (enableTransitionEntanglement) { + // Ignore wipLanes. Always assign to the same bit per priority. + switch (lanePriority) { + case NoLanePriority: + break; + case SyncLanePriority: + return SyncLane; + case SyncBatchedLanePriority: + return SyncBatchedLane; + case InputDiscreteLanePriority: { + return pickArbitraryLane(InputDiscreteLanes); } - return lane; - } - case InputContinuousLanePriority: { - const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes); - if (lane === NoLane) { - // Shift to the next priority level - return findUpdateLane(DefaultLanePriority, wipLanes); + case InputContinuousLanePriority: { + return pickArbitraryLane(InputContinuousLanes); + } + case DefaultLanePriority: { + return pickArbitraryLane(DefaultLanes); } - return lane; + case TransitionPriority: // Should be handled by findTransitionLane instead + case RetryLanePriority: // Should be handled by findRetryLane instead + break; + case IdleLanePriority: + return pickArbitraryLane(IdleLanes); + default: + // The remaining priorities are not valid for updates + break; } - case DefaultLanePriority: { - let lane = pickArbitraryLane(DefaultLanes & ~wipLanes); - if (lane === NoLane) { - // If all the default lanes are already being worked on, look for a - // lane in the transition range. - lane = pickArbitraryLane(TransitionLanes & ~wipLanes); + } else { + // Old behavior that uses wipLanes to shift interleaved updates into a + // separate lane. This is no longer needed because we put interleaved + // updates on a special queue. + switch (lanePriority) { + case NoLanePriority: + break; + case SyncLanePriority: + return SyncLane; + case SyncBatchedLanePriority: + return SyncBatchedLane; + case InputDiscreteLanePriority: { + const lane = pickArbitraryLane(InputDiscreteLanes & ~wipLanes); if (lane === NoLane) { - // All the transition lanes are taken, too. This should be very - // rare, but as a last resort, pick a default lane. This will have - // the effect of interrupting the current work-in-progress render. - lane = pickArbitraryLane(DefaultLanes); + // Shift to the next priority level + return findUpdateLane(InputContinuousLanePriority, wipLanes); } + return lane; } - return lane; - } - case TransitionPriority: // Should be handled by findTransitionLane instead - case RetryLanePriority: // Should be handled by findRetryLane instead - break; - case IdleLanePriority: - let lane = pickArbitraryLane(IdleLanes & ~wipLanes); - if (lane === NoLane) { - lane = pickArbitraryLane(IdleLanes); + case InputContinuousLanePriority: { + const lane = pickArbitraryLane(InputContinuousLanes & ~wipLanes); + if (lane === NoLane) { + // Shift to the next priority level + return findUpdateLane(DefaultLanePriority, wipLanes); + } + return lane; } - return lane; - default: - // The remaining priorities are not valid for updates - break; + case DefaultLanePriority: { + let lane = pickArbitraryLane(DefaultLanes & ~wipLanes); + if (lane === NoLane) { + // If all the default lanes are already being worked on, look for a + // lane in the transition range. + lane = pickArbitraryLane(TransitionLanes & ~wipLanes); + if (lane === NoLane) { + // All the transition lanes are taken, too. This should be very + // rare, but as a last resort, pick a default lane. This will have + // the effect of interrupting the current work-in-progress render. + lane = pickArbitraryLane(DefaultLanes); + } + } + return lane; + } + case TransitionPriority: // Should be handled by findTransitionLane instead + case RetryLanePriority: // Should be handled by findRetryLane instead + break; + case IdleLanePriority: + let lane = pickArbitraryLane(IdleLanes & ~wipLanes); + if (lane === NoLane) { + lane = pickArbitraryLane(IdleLanes); + } + return lane; + default: + // The remaining priorities are not valid for updates + break; + } } invariant( false, @@ -563,7 +599,7 @@ export function claimNextTransitionLane(): Lane { const lane = nextTransitionLane; nextTransitionLane <<= 1; if ((nextTransitionLane & TransitionLanes) === 0) { - nextTransitionLane = SomeTransitionLane; + nextTransitionLane = FirstTransitionLane; } return lane; } @@ -572,7 +608,7 @@ export function claimNextRetryLane(): Lane { const lane = nextRetryLane; nextRetryLane <<= 1; if ((nextRetryLane & RetryLanes) === 0) { - nextRetryLane = SomeRetryLane; + nextRetryLane = FirstRetryLane; } return lane; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index f148e2bd46b9d..f2bcc19557ea5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -34,6 +34,7 @@ import { disableSchedulerTimeoutInWorkLoop, enableDoubleInvokingEffects, skipUnmountedBoundaries, + enableTransitionEntanglement, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -832,6 +833,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = renderRootConcurrent(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, @@ -1037,6 +1039,7 @@ function performSyncWorkOnRoot(root) { lanes = workInProgressRootRenderLanes; exitStatus = renderRootSync(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 3589bff2c6be3..7ca3f9032f002 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -34,6 +34,7 @@ import { disableSchedulerTimeoutInWorkLoop, enableDoubleInvokingEffects, skipUnmountedBoundaries, + enableTransitionEntanglement, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; import invariant from 'shared/invariant'; @@ -832,6 +833,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = renderRootConcurrent(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, @@ -1037,6 +1039,7 @@ function performSyncWorkOnRoot(root) { lanes = workInProgressRootRenderLanes; exitStatus = renderRootSync(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, diff --git a/packages/react-reconciler/src/ReactUpdateQueue.new.js b/packages/react-reconciler/src/ReactUpdateQueue.new.js index 205d7d21d501d..bb4e9e1bc52dc 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.new.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.new.js @@ -285,10 +285,11 @@ export function entangleTransitions(root: FiberRoot, fiber: Fiber, lane: Lane) { // Entangle the new transition lane with the other transition lanes. const newQueueLanes = mergeLanes(queueLanes, lane); - if (newQueueLanes !== queueLanes) { - sharedQueue.lanes = newQueueLanes; - markRootEntangled(root, newQueueLanes); - } + sharedQueue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); } } diff --git a/packages/react-reconciler/src/ReactUpdateQueue.old.js b/packages/react-reconciler/src/ReactUpdateQueue.old.js index d1e82e10d7c39..6b578ef9c80d6 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.old.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.old.js @@ -285,10 +285,11 @@ export function entangleTransitions(root: FiberRoot, fiber: Fiber, lane: Lane) { // Entangle the new transition lane with the other transition lanes. const newQueueLanes = mergeLanes(queueLanes, lane); - if (newQueueLanes !== queueLanes) { - sharedQueue.lanes = newQueueLanes; - markRootEntangled(root, newQueueLanes); - } + sharedQueue.lanes = newQueueLanes; + // Even if queue.lanes already include lane, we don't know for certain if + // the lane finished since the last time we entangled it. So we need to + // entangle it again, just to be sure. + markRootEntangled(root, newQueueLanes); } } diff --git a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js index 2df04c8c88544..2cf0837cfef6f 100644 --- a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js +++ b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js @@ -129,9 +129,9 @@ describe('DebugTracing', () => { expect(Scheduler).toFlushUntilNextPaint([]); expect(logs).toEqual([ - 'group: ⚛️ render (0b0000010000000000000000000000000)', + 'group: ⚛️ render (0b0000000010000000000000000000000)', 'log: ', - 'groupEnd: ⚛️ render (0b0000010000000000000000000000000)', + 'groupEnd: ⚛️ render (0b0000000010000000000000000000000)', ]); }); @@ -209,9 +209,9 @@ describe('DebugTracing', () => { expect(Scheduler).toFlushUntilNextPaint([]); expect(logs).toEqual([ - 'group: ⚛️ render (0b0000010000000000000000000000000)', + 'group: ⚛️ render (0b0000000010000000000000000000000)', 'log: ', - 'groupEnd: ⚛️ render (0b0000010000000000000000000000000)', + 'groupEnd: ⚛️ render (0b0000000010000000000000000000000)', ]); }); @@ -327,6 +327,9 @@ describe('DebugTracing', () => { ]); }); + // This test is coupled to lane implementation details, so I'm disabling it + // until it stabilizes so we don't have to repeatedly update it. + // @gate !enableTransitionEntanglement // @gate experimental && build === 'development' && enableDebugTracing it('should log cascading passive updates', () => { function Example() { diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index c416d2404856c..590761dbc8cd7 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -14,6 +14,7 @@ let ReactNoop; let Scheduler; let readText; let resolveText; +let startTransition; describe('ReactExpiration', () => { beforeEach(() => { @@ -22,6 +23,7 @@ describe('ReactExpiration', () => { React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); + startTransition = React.unstable_startTransition; const textCache = new Map(); @@ -610,6 +612,7 @@ describe('ReactExpiration', () => { expect(root).toMatchRenderedOutput('Sync pri: 2, Idle pri: 2'); }); + // @gate experimental it('a single update can expire without forcing all other updates to expire', async () => { const {useState} = React; @@ -648,12 +651,18 @@ describe('ReactExpiration', () => { await ReactNoop.act(async () => { // Partially render an update - updateNormalPri(); + startTransition(() => { + updateNormalPri(); + }); expect(Scheduler).toFlushAndYieldThrough(['High pri: 0']); - // Some time goes by. In an interleaved event, schedule another update. + + // Some time goes by. Schedule another update. // This will be placed into a separate batch. Scheduler.unstable_advanceTime(4000); - updateNormalPri(); + + startTransition(() => { + updateNormalPri(); + }); // Keep rendering the first update expect(Scheduler).toFlushAndYieldThrough(['Normal pri: 1']); // More time goes by. Enough to expire the first batch, but not the @@ -662,20 +671,20 @@ describe('ReactExpiration', () => { // Attempt to interrupt with a high pri update. updateHighPri(); - // The first update expired, so first will finish it without interrupting. - // But not the second update, which hasn't expired yet. + // The first update expired, so first will finish it without + // interrupting. But not the second update, which hasn't expired yet. expect(Scheduler).toFlushExpired(['Sibling']); + expect(Scheduler).toFlushAndYield([ + // Then render the high pri update + 'High pri: 1', + 'Normal pri: 1', + 'Sibling', + // Then the second normal pri update + 'High pri: 1', + 'Normal pri: 2', + 'Sibling', + ]); }); - expect(Scheduler).toHaveYielded([ - // Then render the high pri update - 'High pri: 1', - 'Normal pri: 1', - 'Sibling', - // Then the second normal pri update - 'High pri: 1', - 'Normal pri: 2', - 'Sibling', - ]); }); it('detects starvation in multiple batches', async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js index aca8679b7cf3b..95c721b87a7c3 100644 --- a/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.js @@ -19,6 +19,7 @@ let NormalPriority; let LowPriority; let IdlePriority; let runWithPriority; +let startTransition; describe('ReactSchedulerIntegration', () => { beforeEach(() => { @@ -33,6 +34,7 @@ describe('ReactSchedulerIntegration', () => { LowPriority = Scheduler.unstable_LowPriority; IdlePriority = Scheduler.unstable_IdlePriority; runWithPriority = Scheduler.unstable_runWithPriority; + startTransition = React.unstable_startTransition; }); function getCurrentPriorityAsString() { @@ -446,6 +448,7 @@ describe( React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); + startTransition = React.unstable_startTransition; }); afterEach(() => { @@ -494,6 +497,7 @@ describe( }); }); + // @gate experimental it('mock Scheduler module to check if `shouldYield` is called', async () => { // This test reproduces a bug where React's Scheduler task timed out but // the `shouldYield` method returned true. Usually we try not to mock @@ -518,7 +522,9 @@ describe( await ReactNoop.act(async () => { // Partially render the tree, then yield - ReactNoop.render(); + startTransition(() => { + ReactNoop.render(); + }); expect(Scheduler).toFlushAndYieldThrough(['A']); // Start logging whenever shouldYield is called @@ -535,7 +541,9 @@ describe( // We only check before yielding to the main thread (to avoid starvation // by other main thread work) or when receiving an update (to avoid // starvation by incoming updates). - ReactNoop.render(); + startTransition(() => { + ReactNoop.render(); + }); // Because the render expired, React should finish the tree without // consulting `shouldYield` again diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 54806dba475e9..6e724fd0a5679 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -270,58 +270,6 @@ describe('ReactSuspense', () => { expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling'); }); - it( - 'interrupts current render if something already suspended with a ' + - "delay, and then subsequently there's a lower priority update", - () => { - const root = ReactTestRenderer.create( - <> - } /> - - , - { - unstable_isConcurrent: true, - }, - ); - expect(Scheduler).toFlushAndYield(['Initial']); - expect(root).toMatchRenderedOutput('Initial'); - - // The update will suspend. - root.update( - <> - }> - - - - - , - ); - - // Yield past the Suspense boundary but don't complete the last sibling. - expect(Scheduler).toFlushAndYieldThrough([ - 'Suspend! [Async]', - 'Loading...', - 'After Suspense', - ]); - - // Receives a lower priority update before the current render phase - // has completed. - Scheduler.unstable_advanceTime(1000); - root.update( - <> - } /> - - , - ); - expect(Scheduler).toHaveYielded([]); - expect(root).toMatchRenderedOutput('Initial'); - - // Render the update, instead of continuing - expect(Scheduler).toFlushAndYield(['Updated']); - expect(root).toMatchRenderedOutput('Updated'); - }, - ); - // @gate experimental it( 'interrupts current render when something suspends with a ' + @@ -392,157 +340,6 @@ describe('ReactSuspense', () => { }, ); - // @gate experimental - it( - 'interrupts current render when something suspends with a ' + - "delay and we've already bailed out lower priority update in " + - 'a parent', - async () => { - // This is similar to the previous test case, except this covers when - // React completely bails out on the parent component, without processing - // the update queue. - - const {useState} = React; - - function interrupt() { - // React has a heuristic to batch all updates that occur within the same - // event. This is a trick to circumvent that heuristic. - ReactTestRenderer.create('whatever'); - } - - let setShouldSuspend; - function Async() { - const [shouldSuspend, _setShouldSuspend] = useState(false); - setShouldSuspend = _setShouldSuspend; - return ( - <> - - }> - {shouldSuspend ? : null} - - - - - ); - } - - let setShouldHideInParent; - function App() { - const [shouldHideInParent, _setShouldHideInParent] = useState(false); - setShouldHideInParent = _setShouldHideInParent; - Scheduler.unstable_yieldValue( - 'shouldHideInParent: ' + shouldHideInParent, - ); - return shouldHideInParent ? : ; - } - - const root = ReactTestRenderer.create(null, { - unstable_isConcurrent: true, - }); - - await act(async () => { - root.update(); - expect(Scheduler).toFlushAndYield([ - 'shouldHideInParent: false', - 'A', - 'B', - 'C', - ]); - expect(root).toMatchRenderedOutput('ABC'); - - // This update will suspend. - setShouldSuspend(true); - - // Need to move into the next async bucket. - // Do a bit of work, then interrupt to trigger a restart. - expect(Scheduler).toFlushAndYieldThrough(['A']); - interrupt(); - // Should not have committed loading state - expect(root).toMatchRenderedOutput('ABC'); - - // Schedule another update. This will have lower priority because it's - // a transition. - React.unstable_startTransition(() => { - setShouldHideInParent(true); - }); - - expect(Scheduler).toFlushAndYieldThrough([ - // Should have restarted the first update, because of the interruption - 'A', - 'Suspend! [Async]', - 'Loading...', - 'B', - ]); - - // Should not have committed loading state - expect(root).toMatchRenderedOutput('ABC'); - - // After suspending, should abort the first update and switch to the - // second update. - expect(Scheduler).toFlushAndYield([ - 'shouldHideInParent: true', - '(empty)', - ]); - - expect(root).toMatchRenderedOutput('(empty)'); - }); - }, - ); - - it( - 'interrupts current render when something suspends with a ' + - 'delay, and a parent received an update after it completed', - () => { - function App({shouldSuspend, step}) { - return ( - <> - - }> - {shouldSuspend ? : null} - - - - - ); - } - - const root = ReactTestRenderer.create(null, { - unstable_isConcurrent: true, - }); - - root.update(); - expect(Scheduler).toFlushAndYield(['A0', 'B0', 'C0']); - expect(root).toMatchRenderedOutput('A0B0C0'); - - // This update will suspend. - root.update(); - // Flush past the root, but stop before the async component. - expect(Scheduler).toFlushAndYieldThrough(['A1']); - - // Schedule an update on the root, which already completed. - root.update(); - // We'll keep working on the existing update. - expect(Scheduler).toFlushAndYieldThrough([ - // Now the async component suspends - 'Suspend! [Async]', - 'Loading...', - 'B1', - ]); - - // Should not have committed loading state - expect(root).toMatchRenderedOutput('A0B0C0'); - - // After suspending, should abort the first update and switch to the - // second update. So, C1 should not appear in the log. - // TODO: This should work even if React does not yield to the main - // thread. Should use same mechanism as selective hydration to interrupt - // the render before the end of the current slice of work. - expect(Scheduler).toFlushAndYield(['A2', 'B2', 'C2']); - - expect(root).toMatchRenderedOutput('A2B2C2'); - }, - ); - it('mounts a lazy class component in non-concurrent mode', async () => { class Class extends React.Component { componentDidMount() { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js index 5a9c94d3a3acc..18042bb4bfb55 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js @@ -589,7 +589,7 @@ describe('ReactSuspenseWithNoopRenderer', () => { // Note: This test was written to test a heuristic used in the expiration // times model. Might not make sense in the new model. - // @gate enableCache + // @gate enableCache || enableTransitionEntanglement it('tries each subsequent level after suspending', async () => { const root = ReactNoop.createRoot(); @@ -642,23 +642,26 @@ describe('ReactSuspenseWithNoopRenderer', () => { root.render(); }); - expect(Scheduler).toHaveYielded([ - // The new reconciler batches everything together, so it finishes without - // suspending again. - 'Sibling', - - // NOTE: The final of the update got pushed into a lower priority range of - // lanes, leading to the extra intermediate render. This is because when - // we schedule the fourth update, we're already in the middle of rendering - // the three others. Since there are only three lanes in the default - // range, the fourth lane is shifted to slightly lower priority. This - // could easily change when we tweak our batching heuristics. Ideally, - // they'd all have default priority and render in a single batch. - 'Suspend! [Step 3]', - 'Sibling', - - 'Step 4', - ]); + if (gate(flags => flags.enableTransitionEntanglement)) { + expect(Scheduler).toHaveYielded(['Sibling', 'Step 4']); + } else { + // Old implementation + expect(Scheduler).toHaveYielded([ + 'Sibling', + + // NOTE: The final of the update got pushed into a lower priority range of + // lanes, leading to the extra intermediate render. This is because when + // we schedule the fourth update, we're already in the middle of rendering + // the three others. Since there are only three lanes in the default + // range, the fourth lane is shifted to slightly lower priority. This + // could easily change when we tweak our batching heuristics. Ideally, + // they'd all have default priority and render in a single batch. + 'Suspend! [Step 3]', + 'Sibling', + + 'Step 4', + ]); + } }); // @gate enableCache @@ -2797,14 +2800,21 @@ describe('ReactSuspenseWithNoopRenderer', () => { foo.setState({suspend: false}); }); - expect(Scheduler).toHaveYielded([ - // First setState - 'Foo', - // Second setState. This update was scheduled while we were in the - // middle of rendering the previous update, so it was pushed to a separate - // batch to avoid invalidating the work-in-progress tree. - 'Foo', - ]); + if (gate(flags => flags.enableTransitionEntanglement)) { + expect(Scheduler).toHaveYielded([ + // First setState + 'Foo', + ]); + } else { + expect(Scheduler).toHaveYielded([ + // First setState + 'Foo', + // Second setState. This update was scheduled while we were in the + // middle of rendering the previous update, so it was pushed to a separate + // batch to avoid invalidating the work-in-progress tree. + 'Foo', + ]); + } expect(root).toMatchRenderedOutput(); }); diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.js index 0b9084356c710..ffd2c9e19ebc7 100644 --- a/packages/react-reconciler/src/__tests__/ReactTransition-test.js +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.js @@ -548,4 +548,229 @@ describe('ReactTransition', () => { expect(root).toMatchRenderedOutput('C'); }, ); + + // @gate experimental + // @gate enableCache + test('interrupt a refresh transition if a new transition is scheduled', async () => { + const root = ReactNoop.createRoot(); + + await ReactNoop.act(async () => { + root.render( + <> + } /> + + , + ); + }); + expect(Scheduler).toHaveYielded(['Initial']); + expect(root).toMatchRenderedOutput('Initial'); + + await ReactNoop.act(async () => { + // Start a refresh transition + startTransition(() => { + root.render( + <> + }> + + + + + , + ); + }); + + // Partially render it. + expect(Scheduler).toFlushAndYieldThrough([ + // Once we the update suspends, we know it's a refresh transition, + // because the Suspense boundary has already mounted. + 'Suspend! [Async]', + 'Loading...', + 'After Suspense', + ]); + + // Schedule a new transition + startTransition(async () => { + root.render( + <> + } /> + + , + ); + }); + }); + + // Because the first one is going to suspend regardless, we should + // immediately switch to rendering the new transition. + expect(Scheduler).toHaveYielded(['Updated']); + expect(root).toMatchRenderedOutput('Updated'); + }); + + // @gate experimental + // @gate enableCache + test( + "interrupt a refresh transition when something suspends and we've " + + 'already bailed out on another transition in a parent', + async () => { + let setShouldSuspend; + + function Parent({children}) { + const [shouldHideInParent, _setShouldHideInParent] = useState(false); + setShouldHideInParent = _setShouldHideInParent; + Scheduler.unstable_yieldValue( + 'shouldHideInParent: ' + shouldHideInParent, + ); + if (shouldHideInParent) { + return ; + } + return children; + } + + let setShouldHideInParent; + function App() { + const [shouldSuspend, _setShouldSuspend] = useState(false); + setShouldSuspend = _setShouldSuspend; + return ( + <> + + + }> + {shouldSuspend ? : null} + + + + + + ); + } + + const root = ReactNoop.createRoot(); + + await act(async () => { + root.render(); + expect(Scheduler).toFlushAndYield([ + 'A', + 'shouldHideInParent: false', + 'B', + 'C', + ]); + expect(root).toMatchRenderedOutput('ABC'); + + // Schedule an update + startTransition(() => { + setShouldSuspend(true); + }); + + // Now we need to trigger schedule another transition in a different + // lane from the first one. At the time this was written, all transitions are worked on + // simultaneously, unless a transition was already in progress when a + // new one was scheduled. So, partially render the first transition. + expect(Scheduler).toFlushAndYieldThrough(['A']); + + // Now schedule a second transition. We won't interrupt the first one. + React.unstable_startTransition(() => { + setShouldHideInParent(true); + }); + // Continue rendering the first transition. + expect(Scheduler).toFlushAndYieldThrough([ + 'shouldHideInParent: false', + 'Suspend! [Async]', + 'Loading...', + 'B', + ]); + // Should not have committed loading state + expect(root).toMatchRenderedOutput('ABC'); + + // At this point, we've processed the parent update queue, so we know + // that it has a pending update from the second transition, even though + // we skipped it during this render. And we know this is a refresh + // transition, because we had to render a loading state. So the next + // time we re-enter the work loop (we don't interrupt immediately, we + // just wait for the next time slice), we should throw out the + // suspended first transition and try the second one. + expect(Scheduler).toFlushUntilNextPaint([ + 'shouldHideInParent: true', + '(empty)', + ]); + expect(root).toMatchRenderedOutput('A(empty)BC'); + + // Since the two transitions are not entangled, we then later go back + // and finish retry the first transition. Not really relevant to this + // test but I'll assert the result anyway. + expect(Scheduler).toFlushAndYield([ + 'A', + 'shouldHideInParent: true', + '(empty)', + 'B', + 'C', + ]); + expect(root).toMatchRenderedOutput('A(empty)BC'); + }); + }, + ); + + // @gate experimental + // @gate enableCache + test( + 'interrupt a refresh transition when something suspends and a parent ' + + 'component received an interleaved update after its queue was processed', + async () => { + // Title is confusing so I'll try to explain further: This is similar to + // the previous test, except instead of skipped over a transition update + // in a parent, the parent receives an interleaved update *after* its + // begin phase has already finished. + + function App({shouldSuspend, step}) { + return ( + <> + + }> + {shouldSuspend ? : null} + + + + + ); + } + + const root = ReactNoop.createRoot(); + + await ReactNoop.act(async () => { + root.render(); + }); + expect(Scheduler).toHaveYielded(['A0', 'B0', 'C0']); + expect(root).toMatchRenderedOutput('A0B0C0'); + + await ReactNoop.act(async () => { + // This update will suspend. + startTransition(() => { + root.render(); + }); + // Flush past the root, but stop before the async component. + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + // Schedule another transition on the root, which already completed. + startTransition(() => { + root.render(); + }); + // We'll keep working on the first update. + expect(Scheduler).toFlushAndYieldThrough([ + // Now the async component suspends + 'Suspend! [Async]', + 'Loading...', + 'B1', + ]); + // Should not have committed loading state + expect(root).toMatchRenderedOutput('A0B0C0'); + + // After suspending, should abort the first update and switch to the + // second update. So, C1 should not appear in the log. + // TODO: This should work even if React does not yield to the main + // thread. Should use same mechanism as selective hydration to interrupt + // the render before the end of the current slice of work. + expect(Scheduler).toFlushAndYield(['A2', 'B2', 'C2']); + + expect(root).toMatchRenderedOutput('A2B2C2'); + }); + }, + ); }); diff --git a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js index 35c854e4b2e77..65459ecd33681 100644 --- a/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js +++ b/packages/react-reconciler/src/__tests__/SchedulingProfiler-test.internal.js @@ -474,6 +474,9 @@ describe('SchedulingProfiler', () => { ]); }); + // This test is coupled to lane implementation details, so I'm disabling it in + // the new fork until it stabilizes so we don't have to repeatedly update it. + // @gate !enableTransitionEntanglement // @gate enableSchedulingProfiler it('should mark cascading passive updates', () => { function Example() {