Skip to content

Commit

Permalink
Bugfix: Suspended update must finish to unhide
Browse files Browse the repository at this point in the history
  • Loading branch information
@acdlite
acdlite committed Mar 27, 2020
1 parent 35a2f74 commit da27d9e
Show file tree
Hide file tree
Showing 5 changed files with 240 additions and 31 deletions.
1 change: 1 addition & 0 deletions packages/react-debug-tools/src/__tests__/ReactDevToolsHooksIntegration-test.js
View file
Expand Up @@ -265,6 +265,7 @@ describe('React hooks DevTools integration', () => {
// Release the lock
setSuspenseHandler(() => false);
scheduleUpdate(fiber); // Re-render
Scheduler.unstable_flushAll();
expect(renderer.toJSON().children).toEqual(['Done']);
scheduleUpdate(fiber); // Re-render
expect(renderer.toJSON().children).toEqual(['Done']);
Expand Down
113 changes: 85 additions & 28 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Expand Up @@ -1570,37 +1570,88 @@ function validateFunctionComponentInDev(workInProgress: Fiber, Component: any) {
}
}

const SUSPENDED_MARKER: SuspenseState = {
dehydrated: null,
retryTime: NoWork,
};
function mountSuspenseState(
renderExpirationTime: ExpirationTime,
): SuspenseState {
return {
dehydrated: null,
skippedTime: renderExpirationTime,
retryTime: NoWork,
};
}

function updateSuspenseState(
prevSuspenseState: SuspenseState,
renderExpirationTime: ExpirationTime,
): SuspenseState {
const prevSuspendedTime = prevSuspenseState.skippedTime;
return {
dehydrated: null,
skippedTime:
// Choose whichever time is a superset of the other one
prevSuspendedTime !== NoWork && prevSuspendedTime < renderExpirationTime
? prevSuspendedTime
: renderExpirationTime,
retryTime: NoWork,
};
}

function shouldRemainOnFallback(
suspenseContext: SuspenseContext,
current: null | Fiber,
workInProgress: Fiber,
renderExpirationTime: ExpirationTime,
) {
// If the context is telling us that we should show a fallback, and we're not
// already showing content, then we should show the fallback instead.
return (
hasSuspenseContext(
if (current === null) {
return hasSuspenseContext(
suspenseContext,
(ForceSuspenseFallback: SuspenseContext),
) &&
(current === null || current.memoizedState !== null)
);
}

const suspenseState: SuspenseState = current.memoizedState;
if (suspenseState === null) {
// Not already suspended.
return false;
}

const skippedTime = suspenseState.skippedTime;
if (skippedTime !== NoWork && skippedTime < renderExpirationTime) {
return true;
}

return hasSuspenseContext(
suspenseContext,
(ForceSuspenseFallback: SuspenseContext),
);
}

function getRemainingWorkInPrimaryTree(
workInProgress,
currentChildExpirationTime,
current: Fiber,
workInProgress: Fiber,
currentPrimaryChildFragment: Fiber | null,
renderExpirationTime,
) {
const currentSuspenseState: SuspenseState = current.memoizedState;
if (currentSuspenseState !== null) {
const skippedTime = currentSuspenseState.skippedTime;
if (skippedTime !== NoWork && skippedTime < renderExpirationTime) {
return skippedTime;
}
}

const currentParentOfPrimaryChildren =
currentPrimaryChildFragment !== null
? currentPrimaryChildFragment
: current;
const currentChildExpirationTime =
currentParentOfPrimaryChildren.childExpirationTime;
if (currentChildExpirationTime < renderExpirationTime) {
// The highest priority remaining work is not part of this render. So the
// remaining work has not changed.
return currentChildExpirationTime;
}

if ((workInProgress.mode & BlockingMode) !== NoMode) {
// The highest priority remaining work is part of this render. Since we only
// keep track of the highest level, we don't know if there's a lower
Expand Down Expand Up @@ -1642,7 +1693,12 @@ function updateSuspenseComponent(

if (
didSuspend ||
shouldRemainOnFallback(suspenseContext, current, workInProgress)
shouldRemainOnFallback(
suspenseContext,
current,
workInProgress,
renderExpirationTime,
)
) {
// Something in this boundary's subtree already suspended. Switch to
// rendering the fallback children.
Expand Down Expand Up @@ -1758,7 +1814,7 @@ function updateSuspenseComponent(
primaryChildFragment.sibling = fallbackChildFragment;
// Skip the primary children, and continue working on the
// fallback children.
workInProgress.memoizedState = SUSPENDED_MARKER;
workInProgress.memoizedState = mountSuspenseState(renderExpirationTime);
workInProgress.child = primaryChildFragment;
return fallbackChildFragment;
} else {
Expand Down Expand Up @@ -1862,15 +1918,15 @@ function updateSuspenseComponent(
primaryChildFragment.sibling = fallbackChildFragment;
fallbackChildFragment.effectTag |= Placement;
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
// This argument represents the remaining work in the current
// primary tree. Since the current tree did not already time out
// the direct parent of the primary children is the Suspense
// fiber, not a fragment.
current.childExpirationTime,
null,
renderExpirationTime,
);
workInProgress.memoizedState = updateSuspenseState(
current.memoizedState,
renderExpirationTime,
);
workInProgress.memoizedState = SUSPENDED_MARKER;
workInProgress.child = primaryChildFragment;

// Skip the primary children, and continue working on the
Expand Down Expand Up @@ -1933,13 +1989,17 @@ function updateSuspenseComponent(
fallbackChildFragment.return = workInProgress;
primaryChildFragment.sibling = fallbackChildFragment;
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
currentPrimaryChildFragment.childExpirationTime,
currentPrimaryChildFragment,
renderExpirationTime,
);
// Skip the primary children, and continue working on the
// fallback children.
workInProgress.memoizedState = SUSPENDED_MARKER;
workInProgress.memoizedState = updateSuspenseState(
current.memoizedState,
renderExpirationTime,
);
workInProgress.child = primaryChildFragment;
return fallbackChildFragment;
} else {
Expand Down Expand Up @@ -2031,17 +2091,14 @@ function updateSuspenseComponent(
primaryChildFragment.sibling = fallbackChildFragment;
fallbackChildFragment.effectTag |= Placement;
primaryChildFragment.childExpirationTime = getRemainingWorkInPrimaryTree(
current,
workInProgress,
// This argument represents the remaining work in the current
// primary tree. Since the current tree did not already time out
// the direct parent of the primary children is the Suspense
// fiber, not a fragment.
current.childExpirationTime,
null,
renderExpirationTime,
);
// Skip the primary children, and continue working on the
// fallback children.
workInProgress.memoizedState = SUSPENDED_MARKER;
workInProgress.memoizedState = mountSuspenseState(renderExpirationTime);
workInProgress.child = primaryChildFragment;
return fallbackChildFragment;
} else {
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberHydrationContext.js
View file
Expand Up @@ -55,7 +55,7 @@ import {
didNotFindHydratableSuspenseInstance,
} from './ReactFiberHostConfig';
import {enableSuspenseServerRenderer} from 'shared/ReactFeatureFlags';
import {Never} from './ReactFiberExpirationTime';
import {Never, NoWork} from './ReactFiberExpirationTime';

// The deepest Fiber on the stack involved in a hydration context.
// This may have been an insertion or a hydration.
Expand Down Expand Up @@ -231,6 +231,7 @@ function tryHydrate(fiber, nextInstance) {
if (suspenseInstance !== null) {
const suspenseState: SuspenseState = {
dehydrated: suspenseInstance,
skippedTime: NoWork,
retryTime: Never,
};
fiber.memoizedState = suspenseState;
Expand Down
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberSuspenseComponent.js
View file
Expand Up @@ -35,6 +35,7 @@ export type SuspenseState = {|
// here to indicate that it is dehydrated (flag) and for quick access
// to check things like isSuspenseInstancePending.
dehydrated: null | SuspenseInstance,
skippedTime: ExpirationTime,
// Represents the earliest expiration time we should attempt to hydrate
// a dehydrated boundary at.
// Never is the default for dehydrated boundaries.
Expand Down
153 changes: 151 additions & 2 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
View file
Expand Up @@ -3168,9 +3168,14 @@ describe('ReactSuspenseWithNoopRenderer', () => {
});

expect(Scheduler).toHaveYielded([
// First try to update the suspended tree. It's still suspended.
'Suspend! [C]',
// First try to render the high pri update. We won't try to re-render
// the suspended tree during this pass, because it still has unfinished
// updates at a lower priority.
'Loading...',

// Now try the suspended update again. It's still suspended.
'Suspend! [C]',

// Then complete the update to the fallback.
'Still loading...',
]);
Expand Down Expand Up @@ -3260,4 +3265,148 @@ describe('ReactSuspenseWithNoopRenderer', () => {
expect(root).toMatchRenderedOutput(<span prop="D" />);
},
);

it(
'after showing fallback, should not flip back to primary content until ' +
'the update that suspended finishes',
async () => {
const {useState, useEffect} = React;
const root = ReactNoop.createRoot();

let setOuterText;
function Parent({step}) {
const [text, _setText] = useState('A');
setOuterText = _setText;
return (
<>
<Text text={'Outer text: ' + text} />
<Text text={'Outer step: ' + step} />
<Suspense fallback={<Text text="Loading..." />}>
<Child step={step} outerText={text} />
</Suspense>
</>
);
}

let setInnerText;
function Child({step, outerText}) {
const [text, _setText] = useState('A');
setInnerText = _setText;

// This will log if the component commits in an inconsistent state
useEffect(() => {
if (text === outerText) {
Scheduler.unstable_yieldValue('Commit Child');
} else {
Scheduler.unstable_yieldValue(
'FIXME: Texts are inconsistent (tearing)',
);
}
}, [text, outerText]);

return (
<>
<AsyncText text={'Inner text: ' + text} />
<Text text={'Inner step: ' + step} />
</>
);
}

// These always update simultaneously. They must be consistent.
function setText(text) {
setOuterText(text);
setInnerText(text);
}

// Mount an initial tree. Resolve A so that it doesn't suspend.
await resolveText('Inner text: A');
await ReactNoop.act(async () => {
root.render(<Parent step={0} />);
});
expect(Scheduler).toHaveYielded([
'Outer text: A',
'Outer step: 0',
'Inner text: A',
'Inner step: 0',
'Commit Child',
]);
expect(root).toMatchRenderedOutput(
<>
<span prop="Outer text: A" />
<span prop="Outer step: 0" />
<span prop="Inner text: A" />
<span prop="Inner step: 0" />
</>,
);

// Update. This causes the inner component to suspend.
await ReactNoop.act(async () => {
setText('B');
});
expect(Scheduler).toHaveYielded([
'Outer text: B',
'Outer step: 0',
'Suspend! [Inner text: B]',
'Inner step: 0',
'Loading...',
]);
// Commit the placeholder
await advanceTimers(250);
expect(root).toMatchRenderedOutput(
<>
<span prop="Outer text: B" />
<span prop="Outer step: 0" />
<span hidden={true} prop="Inner text: A" />
<span hidden={true} prop="Inner step: 0" />
<span prop="Loading..." />
</>,
);

// Schedule a high pri update on the parent.
await ReactNoop.act(async () => {
ReactNoop.discreteUpdates(() => {
root.render(<Parent step={1} />);
});
});
// Only the outer part can update. The inner part should still show a
// fallback because we haven't finished loading B yet. Otherwise, the
// inner text would be inconsistent with the outer text.
expect(Scheduler).toHaveYielded([
'Outer text: B',
'Outer step: 1',
'Loading...',

'Suspend! [Inner text: B]',
'Inner step: 1',
]);
expect(root).toMatchRenderedOutput(
<>
<span prop="Outer text: B" />
<span prop="Outer step: 1" />
<span hidden={true} prop="Inner text: A" />
<span hidden={true} prop="Inner step: 0" />
<span prop="Loading..." />
</>,
);

// Now finish resolving the inner text
await ReactNoop.act(async () => {
await resolveText('Inner text: B');
});
expect(Scheduler).toHaveYielded([
'Promise resolved [Inner text: B]',
'Inner text: B',
'Inner step: 1',
'Commit Child',
]);
expect(root).toMatchRenderedOutput(
<>
<span prop="Outer text: B" />
<span prop="Outer step: 1" />
<span prop="Inner text: B" />
<span prop="Inner step: 1" />
</>,
);
},
);
});

0 comments on commit da27d9e

Please sign in to comment.