From 2c169a56803e31eda9fb8ca6661aa2487a3517c3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 20 Nov 2019 17:56:11 -0800 Subject: [PATCH] Only show most recent transition, per queue When multiple transitions update the same queue, only the most recent one should be allowed to finish. Do not display intermediate states. For example, if you click on multiple tabs in quick succession, we should not switch to any tab that isn't the last one you clicked. --- .../react-reconciler/src/ReactFiberHooks.js | 39 +++- .../react-reconciler/src/ReactFiberThrow.js | 8 + .../src/ReactFiberWorkLoop.js | 47 +++-- .../ReactTransition-test.internal.js | 176 ++++++++++++++++++ 4 files changed, 255 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 6ee4fec28858a..ee1fd3ae08966 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -52,6 +52,7 @@ import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; +import {SuspendOnTask} from './ReactFiberThrow'; import {requestCurrentSuspenseConfig} from './ReactFiberSuspenseConfig'; import { startTransition, @@ -761,7 +762,10 @@ function updateReducer( let prevUpdate = baseUpdate; let update = first; let didSkip = false; + let lastProcessedTransitionTime = NoWork; + let lastSkippedTransitionTime = NoWork; do { + const suspenseConfig = update.suspenseConfig; const updateExpirationTime = update.expirationTime; if (updateExpirationTime < renderExpirationTime) { // Priority is insufficient. Skip this update. If this is the first @@ -777,6 +781,16 @@ function updateReducer( remainingExpirationTime = updateExpirationTime; markUnprocessedUpdateTime(remainingExpirationTime); } + + if (suspenseConfig !== null) { + // This update is part of a transition + if ( + lastSkippedTransitionTime === NoWork || + lastSkippedTransitionTime > updateExpirationTime + ) { + lastSkippedTransitionTime = updateExpirationTime; + } + } } else { // This update does have sufficient priority. // Mark the event time of this update as relevant to this render pass. @@ -785,10 +799,7 @@ function updateReducer( // TODO: We should skip this update if it was already committed but currently // we have no way of detecting the difference between a committed and suspended // update here. - markRenderEventTimeAndConfig( - updateExpirationTime, - update.suspenseConfig, - ); + markRenderEventTimeAndConfig(updateExpirationTime, suspenseConfig); // Process this update. if (update.eagerReducer === reducer) { @@ -799,12 +810,32 @@ function updateReducer( const action = update.action; newState = reducer(newState, action); } + + if (suspenseConfig !== null) { + // This update is part of a transition + if ( + lastProcessedTransitionTime === NoWork || + lastProcessedTransitionTime > updateExpirationTime + ) { + lastProcessedTransitionTime = updateExpirationTime; + } + } } prevUpdate = update; update = update.next; } while (update !== null && update !== first); + if ( + lastProcessedTransitionTime !== NoWork && + lastSkippedTransitionTime !== NoWork + ) { + // There are multiple updates scheduled on this queue, but only some of + // them were processed. To avoid showing an intermediate state, abort + // the current render and restart at a level that includes them all. + throw SuspendOnTask(lastSkippedTransitionTime); + } + if (!didSkip) { newBaseUpdate = prevUpdate; newBaseState = newState; diff --git a/packages/react-reconciler/src/ReactFiberThrow.js b/packages/react-reconciler/src/ReactFiberThrow.js index 0205814d5b5ec..418b73d7e39c7 100644 --- a/packages/react-reconciler/src/ReactFiberThrow.js +++ b/packages/react-reconciler/src/ReactFiberThrow.js @@ -62,6 +62,14 @@ import {Sync} from './ReactFiberExpirationTime'; const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; +// Throw an object with this type to abort the current render and restart at +// a different level. +export function SuspendOnTask(expirationTime: ExpirationTime) { + return { + _retryTime: expirationTime, + }; +} + function createRootErrorUpdate( fiber: Fiber, errorInfo: CapturedValue, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 954d646bd789f..69c01bc1baa21 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -200,13 +200,14 @@ const LegacyUnbatchedContext = /* */ 0b001000; const RenderContext = /* */ 0b010000; const CommitContext = /* */ 0b100000; -type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5; +type RootExitStatus = 0 | 1 | 2 | 3 | 4 | 5 | 6; const RootIncomplete = 0; const RootFatalErrored = 1; -const RootErrored = 2; -const RootSuspended = 3; -const RootSuspendedWithDelay = 4; -const RootCompleted = 5; +const RootSuspendedOnTask = 2; +const RootErrored = 3; +const RootSuspended = 4; +const RootSuspendedWithDelay = 5; +const RootCompleted = 6; export type Thenable = { then(resolve: () => mixed, reject?: () => mixed): Thenable | void, @@ -237,7 +238,7 @@ let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null; // The work left over by components that were visited during this render. Only // includes unprocessed updates, not work in bailed out children. let workInProgressRootNextUnprocessedUpdateTime: ExpirationTime = NoWork; - +let workInProgressRootRestartTime: ExpirationTime = NoWork; // If we're pinged while rendering we don't always restart immediately. // This flag determines if it might be worthwhile to restart if an opportunity // happens latere. @@ -697,7 +698,12 @@ function performConcurrentWorkOnRoot(root, didTimeout) { throw fatalError; } - if (workInProgress !== null) { + if (workInProgressRootExitStatus === RootSuspendedOnTask) { + // Can't finish rendering at this level. Exit early and restart at the + // specified time. + markRootSuspendedAtTime(root, expirationTime); + root.nextKnownPendingLevel = workInProgressRootRestartTime; + } else if (workInProgress !== null) { // There's still work left over. Exit without committing. stopInterruptedWorkLoopTimer(); } else { @@ -738,7 +744,8 @@ function finishConcurrentRender( switch (exitStatus) { case RootIncomplete: - case RootFatalErrored: { + case RootFatalErrored: + case RootSuspendedOnTask: { invariant(false, 'Root did not complete. This is a bug in React.'); } // Flow knows about invariant, so it complains if I add a break @@ -1262,6 +1269,7 @@ function prepareFreshStack(root, expirationTime) { workInProgressRootLatestSuspenseTimeout = Sync; workInProgressRootCanSuspendUsingConfig = null; workInProgressRootNextUnprocessedUpdateTime = NoWork; + workInProgressRootRestartTime = NoWork; workInProgressRootHasPendingPing = false; if (enableSchedulerTracing) { @@ -1299,6 +1307,21 @@ function handleError(root, thrownValue) { stopProfilerTimerIfRunningAndRecordDelta(workInProgress, true); } + // Check if this is a SuspendOnTask exception. + // TODO: Should this be a brand check? + if ( + thrownValue !== null && + typeof thrownValue === 'object' && + typeof thrownValue._retryTime === 'number' + ) { + // Can't finish rendering at this level. Exit early and restart at + // the specified time. + workInProgressRootExitStatus = RootSuspendedOnTask; + workInProgressRootRestartTime = thrownValue._retryTime; + workInProgress = null; + return; + } + throwException( root, workInProgress.return, @@ -2623,15 +2646,17 @@ if (__DEV__ && replayFailedUnitOfWorkWithInvokeGuardedCallback) { try { return originalBeginWork(current, unitOfWork, expirationTime); } catch (originalError) { + // Filter out special exception types if ( originalError !== null && typeof originalError === 'object' && - typeof originalError.then === 'function' + // Promise + (typeof originalError.then === 'function' || + // SuspendOnTask exception + typeof originalError._retryTime === 'number') ) { - // Don't replay promises. Treat everything else like an error. throw originalError; } - // Keep this code in sync with handleError; any changes here must have // corresponding changes there. resetContextDependencies(); diff --git a/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js b/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js index 9ccf72784b184..27ca9a9ebf21d 100644 --- a/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactTransition-test.internal.js @@ -27,6 +27,7 @@ describe('ReactTransition', () => { ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; ReactFeatureFlags.enableSchedulerTracing = true; ReactFeatureFlags.flushSuspenseFallbacksInTests = false; + ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; React = require('react'); ReactNoop = require('react-noop-renderer'); Scheduler = require('scheduler'); @@ -472,4 +473,179 @@ describe('ReactTransition', () => { ); }, ); + + // TODO: Same behavior for classes + it.experimental( + 'when multiple transitions update the same queue, only the most recent ' + + 'one is allowed to finish (no intermediate states)', + async () => { + const CONFIG = { + timeoutMs: 100000, + }; + + const Tab = React.forwardRef(({label, setTab}, ref) => { + const [startTransition, isPending] = useTransition(CONFIG); + + React.useImperativeHandle( + ref, + () => ({ + go() { + startTransition(() => setTab(label)); + }, + }), + [label], + ); + + return ( + + ); + }); + + const tabButtonA = React.createRef(null); + const tabButtonB = React.createRef(null); + const tabButtonC = React.createRef(null); + + const ContentA = createAsyncText('A'); + const ContentB = createAsyncText('B'); + const ContentC = createAsyncText('C'); + + function App() { + Scheduler.unstable_yieldValue('App'); + + const [tab, setTab] = useState('A'); + + let content; + switch (tab) { + case 'A': + content = ; + break; + case 'B': + content = ; + break; + case 'C': + content = ; + break; + default: + content = ; + break; + } + + return ( + <> +
    +
  • + +
    • +
  • + +
    • +
  • + +
    • +
+ }>{content} + + ); + } + + // Initial render + const root = ReactNoop.createRoot(); + await act(async () => { + root.render(); + await ContentA.resolve(); + }); + expect(Scheduler).toHaveYielded(['App', 'Tab A', 'Tab B', 'Tab C', 'A']); + expect(root).toMatchRenderedOutput( + <> +
    +
  • Tab A
    • +
  • Tab B
    • +
  • Tab C
    • +
+ A + , + ); + + // Navigate to tab B + await act(async () => { + tabButtonB.current.go(); + expect(Scheduler).toFlushAndYieldThrough([ + // Turn on B's pending state + 'Tab B (pending...)', + // Partially render B + 'App', + 'Tab A', + 'Tab B', + ]); + + // While we're still in the middle of rendering B, switch to C. + tabButtonC.current.go(); + }); + expect(Scheduler).toHaveYielded([ + // Toggle the pending flags + 'Tab B', + 'Tab C (pending...)', + + // Start rendering B... + 'App', + // ...but bail out, since C is more recent. These should not be logged: + // 'Tab A', + // 'Tab B', + // 'Tab C (pending...)', + // 'Suspend! [B]', + // 'Loading...', + + // Now render C + 'App', + 'Tab A', + 'Tab B', + 'Tab C', + 'Suspend! [C]', + 'Loading...', + ]); + expect(root).toMatchRenderedOutput( + <> +
    +
  • Tab A
    • +
  • Tab B
    • +
  • Tab C (pending...)
    • +
+ A + , + ); + + // Finish loading B + await act(async () => { + ContentB.resolve(); + }); + // Should not switch to tab B because we've since clicked on C. + expect(Scheduler).toHaveYielded([]); + expect(root).toMatchRenderedOutput( + <> +
    +
  • Tab A
    • +
  • Tab B
    • +
  • Tab C (pending...)
    • +
+ A + , + ); + + // Finish loading C + await act(async () => { + ContentC.resolve(); + }); + expect(Scheduler).toHaveYielded(['App', 'Tab A', 'Tab B', 'Tab C', 'C']); + expect(root).toMatchRenderedOutput( + <> +
    +
  • Tab A
    • +
  • Tab B
    • +
  • Tab C
    • +
+ C + , + ); + }, + ); });