Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try rendering again if a timed out tree receives an update #13921

Merged
merged 1 commit into from
Oct 23, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions packages/jest-react/src/JestReact.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,17 @@ function assertYieldsWereCleared(root) {
}

export function toFlushAndYield(root, expectedYields) {
assertYieldsWereCleared(root);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved these outside because captureAssertion was swallowing the stack trace.

const actualYields = root.unstable_flushAll();
return captureAssertion(() => {
assertYieldsWereCleared(root);
const actualYields = root.unstable_flushAll();
expect(actualYields).toEqual(expectedYields);
});
}

export function toFlushAndYieldThrough(root, expectedYields) {
assertYieldsWereCleared(root);
const actualYields = root.unstable_flushNumberOfYields(expectedYields.length);
return captureAssertion(() => {
assertYieldsWereCleared(root);
const actualYields = root.unstable_flushNumberOfYields(
expectedYields.length,
);
expect(actualYields).toEqual(expectedYields);
});
}
Expand Down Expand Up @@ -76,8 +74,8 @@ export function toHaveYielded(ReactTestRenderer, expectedYields) {
}

export function toFlushAndThrow(root, ...rest) {
assertYieldsWereCleared(root);
return captureAssertion(() => {
assertYieldsWereCleared(root);
expect(() => {
root.unstable_flushAll();
}).toThrow(...rest);
Expand Down
59 changes: 39 additions & 20 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1018,20 +1018,16 @@ function updateSuspenseComponent(
// We should attempt to render the primary children unless this boundary
// already suspended during this render (`alreadyCaptured` is true).
let nextState: SuspenseState | null = workInProgress.memoizedState;
let nextDidTimeout;
if (nextState === null) {
// An empty suspense state means this boundary has not yet timed out.
nextDidTimeout = false;
} else {
if (!nextState.alreadyCaptured) {
// Since we haven't already suspended during this commit, clear the
// existing suspense state. We'll try rendering again.
nextDidTimeout = false;
nextState = null;
} else {
// Something in this boundary's subtree already suspended. Switch to
// rendering the fallback children. Set `alreadyCaptured` to true.
nextDidTimeout = true;
if (current !== null && nextState === current.memoizedState) {
// Create a new suspense state to avoid mutating the current tree's.
nextState = {
Expand All @@ -1046,6 +1042,7 @@ function updateSuspenseComponent(
}
}
}
const nextDidTimeout = nextState !== null && nextState.didTimeout;

// This next part is a bit confusing. If the children timeout, we switch to
// showing the fallback children in place of the "primary" children.
Expand Down Expand Up @@ -1127,8 +1124,8 @@ function updateSuspenseComponent(
// its fragment. We're going to skip over these entirely.
const nextFallbackChildren = nextProps.fallback;
const primaryChildFragment = createWorkInProgress(
currentFallbackChildFragment,
currentFallbackChildFragment.pendingProps,
currentPrimaryChildFragment,
currentPrimaryChildFragment.pendingProps,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a fix for an oopsies that could have been its own PR, but it was required to get the new test I wrote to pass. I can split it out if I need to.

NoWork,
);
primaryChildFragment.effectTag |= Placement;
Expand Down Expand Up @@ -1481,23 +1478,45 @@ function beginWork(
}
break;
case SuspenseComponent: {
const child = bailoutOnAlreadyFinishedWork(
current,
workInProgress,
renderExpirationTime,
);
if (child !== null) {
const nextState = workInProgress.memoizedState;
const nextDidTimeout = nextState !== null && nextState.didTimeout;
if (nextDidTimeout) {
child.childExpirationTime = NoWork;
return child.sibling;
const state: SuspenseState | null = workInProgress.memoizedState;
const didTimeout = state !== null && state.didTimeout;
if (didTimeout) {
// If this boundary is currently timed out, we need to decide
// whether to retry the primary children, or to skip over it and
// go straight to the fallback. Check the priority of the primary
// child fragment.
const primaryChildFragment: Fiber = (workInProgress.child: any);
const primaryChildExpirationTime =
primaryChildFragment.childExpirationTime;
if (
primaryChildExpirationTime !== NoWork &&
primaryChildExpirationTime <= renderExpirationTime
) {
// The primary children have pending work. Use the normal path
// to attempt to render the primary children again.
return updateSuspenseComponent(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main fix here.

current,
workInProgress,
renderExpirationTime,
);
} else {
return child;
// The primary children do not have pending work with sufficient
// priority. Bailout.
const child = bailoutOnAlreadyFinishedWork(
current,
workInProgress,
renderExpirationTime,
);
if (child !== null) {
// The fallback children have pending work. Skip over the
// primary children and work on the fallback.
return child.sibling;
} else {
return null;
}
}
} else {
return null;
}
break;
}
}
return bailoutOnAlreadyFinishedWork(
Expand Down
15 changes: 15 additions & 0 deletions packages/react-reconciler/src/ReactFiberSuspenseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* @flow
*/

import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';

export type SuspenseState = {|
Expand All @@ -23,3 +24,17 @@ export type SuspenseState = {|
// `didTimeout` because it's not set unless the boundary actually commits.
timedOutAt: ExpirationTime,
|};

export function shouldCaptureSuspense(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split this out because it was getting hard to follow all the nested branches in the throwException function

current: Fiber | null,
workInProgress: Fiber,
): boolean {
// In order to capture, the Suspense component must have a fallback prop.
if (workInProgress.memoizedProps.fallback === undefined) {
return false;
}
// If it was the primary children that just suspended, capture and render the
// fallback. Otherwise, don't capture and bubble to the next boundary.
const nextState: SuspenseState | null = workInProgress.memoizedState;
return nextState === null || !nextState.didTimeout;
}
Loading