From 536e60f3cf154c42336d3f7fcc9ca7078c825987 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 28 Jan 2021 02:39:07 -0600 Subject: [PATCH] Don't shift interleaved updates to separate lane Now that interleaved updates are added to a special queue, we no longer need to shift them into their own lane. We can add to a lane that's already in the middle of rendering without risk of tearing. See #20615 for more background. I've only changed this in the new fork, and only behind the enableTransitionEntanglements flag. Most of this commit involves updating tests. The "shift-to-a-new" lane trick was intentionally used in a handful of tests where two or more updates need to be scheduled in different lanes. Most of these tests were written before `startTransition` existed, and all of them were written before transitions were assigned arbitrary lanes. So I ported these tests to use `startTransition` instead, which is the idiomatic way to mark an update as parallel. I didn't change the old fork at all. Writing these tests in such a way that they also pass in the old fork actually revealed a few flaws in the current implementation regarding interrupting a suspended refresh transition early, which is a good reminder that we should be writing our tests using idiomatic patterns as much as we possibly can. --- .../src/ReactFiberLane.new.js | 118 +++++---- .../src/ReactFiberWorkLoop.new.js | 3 + .../__tests__/DebugTracing-test.internal.js | 3 + .../src/__tests__/ReactExpiration-test.js | 51 ++-- .../ReactSchedulerIntegration-test.js | 18 +- .../__tests__/ReactSuspense-test.internal.js | 203 ---------------- .../ReactSuspenseWithNoopRenderer-test.js | 62 +++-- .../src/__tests__/ReactTransition-test.js | 227 ++++++++++++++++++ .../SchedulingProfiler-test.internal.js | 3 + scripts/jest/TestFlags.js | 8 + 10 files changed, 405 insertions(+), 291 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberLane.new.js b/packages/react-reconciler/src/ReactFiberLane.new.js index ddb93470efc33..39498df936e45 100644 --- a/packages/react-reconciler/src/ReactFiberLane.new.js +++ b/packages/react-reconciler/src/ReactFiberLane.new.js @@ -512,56 +512,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, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 9908c2a8a607d..7c97ba0c1d6be 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'; @@ -809,6 +810,7 @@ function performConcurrentWorkOnRoot(root, didTimeout) { let exitStatus = renderRootConcurrent(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, @@ -1014,6 +1016,7 @@ function performSyncWorkOnRoot(root) { lanes = workInProgressRootRenderLanes; exitStatus = renderRootSync(root, lanes); if ( + !enableTransitionEntanglement && includesSomeLane( workInProgressRootIncludedLanes, workInProgressRootUpdatedLanes, diff --git a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js index 2df04c8c88544..8547261fbff55 100644 --- a/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js +++ b/packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js @@ -327,6 +327,9 @@ describe('DebugTracing', () => { ]); }); + // 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 !enableParallelTransitions // @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 c28a3ba155824..77b38ef646695 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,30 @@ 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. - expect(Scheduler).toFlushExpired(['Sibling']); + if (gate(flags => flags.enableParallelTransitions)) { + // 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', + ]); + } else { + // In the old implementation, all updates get entangled together. + expect(Scheduler).toFlushExpired([ + 'High pri: 1', + 'Normal pri: 2', + 'Sibling', + ]); + expect(Scheduler).toFlushAndYield([]); + } }); - 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..6a6dce7de04af 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,11 +541,17 @@ 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 - expect(Scheduler).toFlushExpired(['B', 'C']); + if (gate(flags => flags.enableParallelTransitions)) { + expect(Scheduler).toFlushExpired(['B', 'C']); + } else { + expect(Scheduler).toFlushExpired(['B', 'C', 'A', 'B', 'C']); + } }); }); }, 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 5021c32c8b11e..146e75b3a79ac 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 || enableParallelTransitions 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.enableParallelTransitions)) { + 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.enableParallelTransitions)) { + 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..c9ae1d9b6121e 100644 --- a/packages/react-reconciler/src/__tests__/ReactTransition-test.js +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.js @@ -548,4 +548,231 @@ describe('ReactTransition', () => { expect(root).toMatchRenderedOutput('C'); }, ); + + // @gate enableParallelTransitions + // @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 enableParallelTransitions + // @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..55d9ca4309f23 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 !enableParallelTransitions // @gate enableSchedulingProfiler it('should mark cascading passive updates', () => { function Example() { diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index d46a26fb942e9..2f7406fc67427 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -89,6 +89,14 @@ function getTestFlags() { // tests, Jest doesn't expose the API correctly. Fix then remove // this override. enableCache: __EXPERIMENTAL__, + + // enableTransitionEntanglement has a slightly different meaning in the + // new fork. This is temporary, but I did it this way to avoid creating an + // entirely new GK and to have fewer combinations of flags. The two forks + // will converge soon so I'll remove this once they do. + enableParallelTransitions: + featureFlags.enableTransitionEntanglement && + featureFlags.enableNewReconciler, }, { get(flags, flagName) {