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

Add tail="collapsed" option to SuspenseList #16007

Merged
merged 2 commits into from
Jul 2, 2019
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
39 changes: 39 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {
SuspenseState,
SuspenseListRenderState,
SuspenseListTailMode,
} from './ReactFiberSuspenseComponent';
import type {SuspenseContext} from './ReactFiberSuspenseContext';

Expand Down Expand Up @@ -188,6 +189,7 @@ let didWarnAboutFunctionRefs;
export let didWarnAboutReassigningProps;
let didWarnAboutMaxDuration;
let didWarnAboutRevealOrder;
let didWarnAboutTailOptions;

if (__DEV__) {
didWarnAboutBadClass = {};
Expand All @@ -198,6 +200,7 @@ if (__DEV__) {
didWarnAboutReassigningProps = false;
didWarnAboutMaxDuration = false;
didWarnAboutRevealOrder = {};
didWarnAboutTailOptions = {};
}

export function reconcileChildren(
Expand Down Expand Up @@ -2063,11 +2066,40 @@ function validateRevealOrder(revealOrder: SuspenseListRevealOrder) {
}
}

function validateTailOptions(
tailMode: SuspenseListTailMode,
revealOrder: SuspenseListRevealOrder,
) {
if (__DEV__) {
if (tailMode !== undefined && !didWarnAboutTailOptions[tailMode]) {
if (tailMode !== 'collapsed') {
didWarnAboutTailOptions[tailMode] = true;
warning(
false,
'"%s" is not a supported value for tail on <SuspenseList />. ' +
'Did you mean "collapsed"?',
tailMode,
);
} else if (revealOrder !== 'forwards' && revealOrder !== 'backwards') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why else if?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So that you don't get both warnings at once. Seems more actionable to pick a legit value first. E.g. if you specified something like "none", it would be misleading to recommend a revealOrder.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. I guess that makes sense. I was worried it could feel a bit like warning-whack-a-mole if you fixed one just to see another pop up.

The revealOrder would still be invalid if you specified tail="none" though so wouldn't we still want to warn about it?

I dunno. I don't feel strongly here 😄 Was just asking.

didWarnAboutTailOptions[tailMode] = true;
warning(
false,
'<SuspenseList tail="%s" /> is only valid if revealOrder is ' +
'"forwards" or "backwards". ' +
'Did you mean to specify revealOrder="forwards"?',
tailMode,
);
}
}
}
}

function initSuspenseListRenderState(
workInProgress: Fiber,
isBackwards: boolean,
tail: null | Fiber,
lastContentRow: null | Fiber,
tailMode: SuspenseListTailMode,
): void {
let renderState: null | SuspenseListRenderState =
workInProgress.memoizedState;
Expand All @@ -2078,6 +2110,7 @@ function initSuspenseListRenderState(
last: lastContentRow,
tail: tail,
tailExpiration: 0,
tailMode: tailMode,
};
} else {
// We can reuse the existing object from previous renders.
Expand All @@ -2086,6 +2119,7 @@ function initSuspenseListRenderState(
renderState.last = lastContentRow;
renderState.tail = tail;
renderState.tailExpiration = 0;
renderState.tailMode = tailMode;
}
}

Expand All @@ -2103,9 +2137,11 @@ function updateSuspenseListComponent(
) {
const nextProps = workInProgress.pendingProps;
const revealOrder: SuspenseListRevealOrder = nextProps.revealOrder;
const tailMode: SuspenseListTailMode = nextProps.tail;
const newChildren = nextProps.children;

validateRevealOrder(revealOrder);
validateTailOptions(tailMode, revealOrder);

reconcileChildren(current, workInProgress, newChildren, renderExpirationTime);

Expand Down Expand Up @@ -2163,6 +2199,7 @@ function updateSuspenseListComponent(
false, // isBackwards
tail,
lastContentRow,
tailMode,
);
break;
}
Expand Down Expand Up @@ -2193,6 +2230,7 @@ function updateSuspenseListComponent(
true, // isBackwards
tail,
null, // last
tailMode,
);
break;
}
Expand All @@ -2202,6 +2240,7 @@ function updateSuspenseListComponent(
false, // isBackwards
null, // tail
null, // last
undefined,
);
break;
}
Expand Down
55 changes: 51 additions & 4 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,46 @@ if (supportsMutation) {
};
}

function cutOffTailIfNeeded(
renderState: SuspenseListRenderState,
hasRenderedATailFallback: boolean,
) {
switch (renderState.tailMode) {
case 'collapsed': {
// Any insertions at the end of the tail list after this point
// should be invisible. If there are already mounted boundaries
// anything before them are not considered for collapsing.
// Therefore we need to go through the whole tail to find if
// there are any.
let tailNode = renderState.tail;
let lastTailNode = null;
while (tailNode !== null) {
if (tailNode.alternate !== null) {
lastTailNode = tailNode;
}
tailNode = tailNode.sibling;
}
// Next we're simply going to delete all insertions after the
// last rendered item.
if (lastTailNode === null) {
// All remaining items in the tail are insertions.
if (!hasRenderedATailFallback && renderState.tail !== null) {
// We suspended during the head. We want to show at least one
// row at the tail. So we'll keep on and cut off the rest.
renderState.tail.sibling = null;
} else {
renderState.tail = null;
}
} else {
// Detach the insertion after the last node that was already
// inserted.
lastTailNode.sibling = null;
}
break;
}
}
}

// Note this, might mutate the workInProgress passed in.
function hasSuspendedChildrenAndNewContent(
workInProgress: Fiber,
Expand Down Expand Up @@ -991,11 +1031,18 @@ function completeWork(
didSuspendAlready =
(workInProgress.effectTag & DidCapture) !== NoEffect;
}
if (didSuspendAlready) {
cutOffTailIfNeeded(renderState, false);
}
// Next we're going to render the tail.
} else {
// Append the rendered row to the child list.
if (!didSuspendAlready) {
if (
if (isShowingAnyFallbacks(renderedTail)) {
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;
cutOffTailIfNeeded(renderState, true);
} else if (
now() > renderState.tailExpiration &&
renderExpirationTime > Never
) {
Expand All @@ -1004,6 +1051,9 @@ function completeWork(
// The assumption is that this is usually faster.
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;

cutOffTailIfNeeded(renderState, false);

// Since nothing actually suspended, there will nothing to ping this
// to get it started back up to attempt the next item. If we can show
// them, then they really have the same priority as this render.
Expand All @@ -1015,9 +1065,6 @@ function completeWork(
if (enableSchedulerTracing) {
markSpawnedWork(nextPriority);
}
} else if (isShowingAnyFallbacks(renderedTail)) {
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;
}
}
if (renderState.isBackwards) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-reconciler/src/ReactFiberSuspenseComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import {SuspenseComponent} from 'shared/ReactWorkTags';
// Alternatively we can make this use an effect tag similar to SuspenseList.
export type SuspenseState = {||};

export type SuspenseListTailMode = 'collapsed' | void;

export type SuspenseListRenderState = {|
isBackwards: boolean,
// The currently rendering tail row.
Expand All @@ -24,6 +26,8 @@ export type SuspenseListRenderState = {|
tail: null | Fiber,
// The absolute time in ms that we'll expire the tail rendering.
tailExpiration: number,
// Tail insertions setting.
tailMode: SuspenseListTailMode,
|};

export function shouldCaptureSuspense(
Expand Down
Loading