Skip to content

Commit

Permalink
Only show most recent transition, per queue
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Nov 21, 2019
1 parent 10ea752 commit 683087f
Show file tree
Hide file tree
Showing 4 changed files with 240 additions and 7 deletions.
39 changes: 35 additions & 4 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -761,7 +762,10 @@ function updateReducer<S, I, A>(
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
Expand All @@ -777,6 +781,16 @@ function updateReducer<S, I, A>(
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.
Expand All @@ -785,10 +799,7 @@ function updateReducer<S, I, A>(
// 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) {
Expand All @@ -799,12 +810,32 @@ function updateReducer<S, I, A>(
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 new SuspendOnTask(lastSkippedTransitionTime);
}
if (!didSkip) {
newBaseUpdate = prevUpdate;
newBaseState = newState;
Expand Down
6 changes: 6 additions & 0 deletions packages/react-reconciler/src/ReactFiberThrow.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ 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) {
this.expirationTime = expirationTime;
}

function createRootErrorUpdate(
fiber: Fiber,
errorInfo: CapturedValue<mixed>,
Expand Down
26 changes: 23 additions & 3 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ import {
throwException,
createRootErrorUpdate,
createClassErrorUpdate,
SuspendOnTask,
} from './ReactFiberThrow';
import {
commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber,
Expand Down Expand Up @@ -200,13 +201,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 = 5;
const RootCompleted = 6;

export type Thenable = {
then(resolve: () => mixed, reject?: () => mixed): Thenable | void,
Expand Down Expand Up @@ -237,7 +239,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.
Expand Down Expand Up @@ -933,6 +935,13 @@ function finishConcurrentRender(
commitRoot(root);
break;
}
case RootSuspendedOnTask: {
// Can't finish rendering at this level. Exit early and restart at the
// specified time.
markRootSuspendedAtTime(root, expirationTime);
root.nextKnownPendingLevel = workInProgressRootRestartTime;
break;
}
case RootCompleted: {
// The work completed. Ready to commit.
if (
Expand Down Expand Up @@ -1262,6 +1271,7 @@ function prepareFreshStack(root, expirationTime) {
workInProgressRootLatestSuspenseTimeout = Sync;
workInProgressRootCanSuspendUsingConfig = null;
workInProgressRootNextUnprocessedUpdateTime = NoWork;
workInProgressRootRestartTime = NoWork;
workInProgressRootHasPendingPing = false;

if (enableSchedulerTracing) {
Expand Down Expand Up @@ -1299,6 +1309,16 @@ function handleError(root, thrownValue) {
stopProfilerTimerIfRunningAndRecordDelta(workInProgress, true);
}

// TODO: I think it's fine to use instanceof here because this exception
// is always thrown by the renderer? Or use a brand check.
if (thrownValue instanceof SuspendOnTask) {
// Can't finish rendering at this level. Exit early and restart at
// the specified time.
workInProgressRootExitStatus = RootSuspendedOnTask;
workInProgressRootRestartTime = thrownValue.expirationTime;
return null;
}

throwException(
root,
workInProgress.return,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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 (
<Text text={'Tab ' + label + (isPending ? ' (pending...)' : '')} />
);
});

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 = <ContentA />;
break;
case 'B':
content = <ContentB />;
break;
case 'C':
content = <ContentC />;
break;
default:
content = <ContentA />;
break;
}

return (
<>
<ul>
<li>
<Tab ref={tabButtonA} label="A" setTab={setTab} />
</li>
<li>
<Tab ref={tabButtonB} label="B" setTab={setTab} />
</li>
<li>
<Tab ref={tabButtonC} label="C" setTab={setTab} />
</li>
</ul>
<Suspense fallback={<Text text="Loading..." />}>{content}</Suspense>
</>
);
}

// Initial render
const root = ReactNoop.createRoot();
await act(async () => {
root.render(<App />);
await ContentA.resolve();
});
expect(Scheduler).toHaveYielded(['App', 'Tab A', 'Tab B', 'Tab C', 'A']);
expect(root).toMatchRenderedOutput(
<>
<ul>
<li>Tab A</li>
<li>Tab B</li>
<li>Tab C</li>
</ul>
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(
<>
<ul>
<li>Tab A</li>
<li>Tab B</li>
<li>Tab C (pending...)</li>
</ul>
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(
<>
<ul>
<li>Tab A</li>
<li>Tab B</li>
<li>Tab C (pending...)</li>
</ul>
A
</>,
);

// Finish loading C
await act(async () => {
ContentC.resolve();
});
expect(Scheduler).toHaveYielded(['App', 'Tab A', 'Tab B', 'Tab C', 'C']);
expect(root).toMatchRenderedOutput(
<>
<ul>
<li>Tab A</li>
<li>Tab B</li>
<li>Tab C</li>
</ul>
C
</>,
);
},
);
});

0 comments on commit 683087f

Please sign in to comment.