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.

Sign up for GitHub

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

Never attach ping listeners in legacy Suspense #22407

Merged
merged 1 commit into from
Sep 23, 2021
Merged

Never attach ping listeners in legacy Suspense #22407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
129 changes: 67 additions & 62 deletions packages/react-reconciler/src/ReactFiberThrow.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,76 +297,82 @@ function throwException(
wakeables.add(wakeable);
}

// If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. In the commit phase, we'll schedule a
// subsequent synchronous update to re-render the Suspense.
//
// Note: It doesn't matter whether the component that suspended was
// inside a concurrent mode tree. If the Suspense is outside of it, we
// should *not* suspend the commit.
//
// If the suspense boundary suspended itself suspended, we don't have to
// do this trick because nothing was partially started. We can just
// directly do a second pass over the fallback in this render and
// pretend we meant to render that directly.
if (
(workInProgress.mode & ConcurrentMode) === NoMode &&
workInProgress !== returnFiber
) {
workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense;
if ((workInProgress.mode & ConcurrentMode) === NoMode) {
// Legacy Mode Suspense
//
// If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. When the Suspense boundary completes,
// we'll do a second pass to render the fallback.
if (workInProgress === returnFiber) {
// Special case where we suspended while reconciling the children of
// a Suspense boundary's inner Offscreen wrapper fiber. This happens
// when a React.lazy component is a direct child of a
// Suspense boundary.
//
// Suspense boundaries are implemented as multiple fibers, but they
// are a single conceptual unit. The legacy mode behavior where we
// pretend the suspended fiber committed as `null` won't work,
// because in this case the "suspended" fiber is the inner
// Offscreen wrapper.
//
// Because the contents of the boundary haven't started rendering
// yet (i.e. nothing in the tree has partially rendered) we can
// switch to the regular, concurrent mode behavior: mark the
// boundary with ShouldCapture and enter the unwind phase.
workInProgress.flags |= ShouldCapture;
Copy link
Contributor

Choose a reason for hiding this comment

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

So it seems the only place where we do something for the ShouldCapture flag is here: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberUnwindWork.new.js#L64

So am I right in understanding that your change is just signaling that we haven't done the work for this fiber yet and that this flag is only used for attributing time spent doing work to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The profiler call is a red herring. The relevant part of this branch is that it marks a DidCapture flag:

react/packages/react-reconciler/src/ReactFiberUnwindWork.new.js

Line 117 in d174d06

workInProgress.flags = (flags & ~ShouldCapture) | DidCapture;

and it returns the work-in-progress fiber (which in this case is a Suspense boundary) instead of returning null:

react/packages/react-reconciler/src/ReactFiberUnwindWork.new.js

Lines 125 to 127 in d174d06

return workInProgress;
}
return null;

That will tell the work loop to render the Suspense boundary again:

react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js

Lines 1700 to 1708 in d174d06

if (next !== null) {
// If completing this work spawned new work, do that next. We'll come
// back here again.
// Since we're restarting, remove anything that is not a host effect
// from the effect tag.
next.flags &= HostEffectMask;
workInProgress = next;
return;
}

And since we set a DidCapture flag, we'll render the fallback this time instead of the regular children:

react/packages/react-reconciler/src/ReactFiberBeginWork.new.js

Line 1936 in d174d06

const didSuspend = (workInProgress.flags & DidCapture) !== NoFlags;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the old code, though, we were already doing this because line 315 here causes us to fall through to the regular concurrent mode path:

react/packages/react-reconciler/src/ReactFiberThrow.new.js

Lines 313 to 316 in d174d06

if (
(workInProgress.mode & ConcurrentMode) === NoMode &&
workInProgress !== returnFiber
) {

which looks like this:

react/packages/react-reconciler/src/ReactFiberThrow.new.js

Lines 412 to 414 in d174d06

attachPingListener(root, wakeable, rootRenderLanes);
workInProgress.flags |= ShouldCapture;

So the net change for the branch I added is:

- attachPingListener(root, wakeable, rootRenderLanes);
workInProgress.flags |= ShouldCapture;

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh okay makes sense!

} else {
workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense;

// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);

if (supportsPersistence && enablePersistentOffscreenHostContainer) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
}
}
}
}

if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
// This is a new mount. Change the tag so it's not mistaken for a
// completed class component. For example, we should not call
// componentWillUnmount if it is deleted.
sourceFiber.tag = IncompleteClassComponent;
} else {
// When we try rendering again, we should not reuse the current fiber,
// since it's known to be in an inconsistent state. Use a force update to
// prevent a bail out.
const update = createUpdate(NoTimestamp, SyncLane);
update.tag = ForceUpdate;
enqueueUpdate(sourceFiber, update, SyncLane);
if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
// This is a new mount. Change the tag so it's not mistaken for a
// completed class component. For example, we should not call
// componentWillUnmount if it is deleted.
sourceFiber.tag = IncompleteClassComponent;
} else {
// When we try rendering again, we should not reuse the current fiber,
// since it's known to be in an inconsistent state. Use a force update to
// prevent a bail out.
const update = createUpdate(NoTimestamp, SyncLane);
update.tag = ForceUpdate;
enqueueUpdate(sourceFiber, update, SyncLane);
}
}
}

// The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);

// Exit without suspending.
// The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
}
return;
}

// Confirmed that the boundary is in a concurrent mode tree. Continue
// with the normal suspend path.
//
Expand Down Expand Up @@ -415,7 +421,6 @@ function throwException(
// TODO: I think we can remove this, since we now use `DidCapture` in
// the begin phase to prevent an early bailout.
workInProgress.lanes = rootRenderLanes;

return;
}
// This boundary already captured during this render. Continue to the next
Expand Down
129 changes: 67 additions & 62 deletions packages/react-reconciler/src/ReactFiberThrow.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,76 +297,82 @@ function throwException(
wakeables.add(wakeable);
}

// If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. In the commit phase, we'll schedule a
// subsequent synchronous update to re-render the Suspense.
//
// Note: It doesn't matter whether the component that suspended was
// inside a concurrent mode tree. If the Suspense is outside of it, we
// should *not* suspend the commit.
//
// If the suspense boundary suspended itself suspended, we don't have to
// do this trick because nothing was partially started. We can just
// directly do a second pass over the fallback in this render and
// pretend we meant to render that directly.
if (
(workInProgress.mode & ConcurrentMode) === NoMode &&
workInProgress !== returnFiber
) {
workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense;
if ((workInProgress.mode & ConcurrentMode) === NoMode) {
// Legacy Mode Suspense
//
// If the boundary is in legacy mode, we should *not*
// suspend the commit. Pretend as if the suspended component rendered
// null and keep rendering. When the Suspense boundary completes,
// we'll do a second pass to render the fallback.
if (workInProgress === returnFiber) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the special branch that I moved. The rest of the diff is updating comments that referred to outdated implementation details.

// Special case where we suspended while reconciling the children of
// a Suspense boundary's inner Offscreen wrapper fiber. This happens
// when a React.lazy component is a direct child of a
// Suspense boundary.
//
// Suspense boundaries are implemented as multiple fibers, but they
// are a single conceptual unit. The legacy mode behavior where we
// pretend the suspended fiber committed as `null` won't work,
// because in this case the "suspended" fiber is the inner
// Offscreen wrapper.
//
// Because the contents of the boundary haven't started rendering
// yet (i.e. nothing in the tree has partially rendered) we can
// switch to the regular, concurrent mode behavior: mark the
// boundary with ShouldCapture and enter the unwind phase.
workInProgress.flags |= ShouldCapture;
} else {
workInProgress.flags |= DidCapture;
sourceFiber.flags |= ForceUpdateForLegacySuspense;

// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
// We're going to commit this fiber even though it didn't complete.
// But we shouldn't call any lifecycle methods or callbacks. Remove
// all lifecycle effect tags.
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);

if (supportsPersistence && enablePersistentOffscreenHostContainer) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
// Another legacy Suspense quirk. In persistent mode, if this is the
// initial mount, override the props of the host container to hide
// its contents.
const currentSuspenseBoundary = workInProgress.alternate;
if (currentSuspenseBoundary === null) {
const offscreenFiber: Fiber = (workInProgress.child: any);
const offscreenContainer = offscreenFiber.child;
if (offscreenContainer !== null) {
const children = offscreenContainer.memoizedProps.children;
const containerProps = getOffscreenContainerProps(
'hidden',
children,
);
offscreenContainer.pendingProps = containerProps;
offscreenContainer.memoizedProps = containerProps;
}
}
}
}

if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
// This is a new mount. Change the tag so it's not mistaken for a
// completed class component. For example, we should not call
// componentWillUnmount if it is deleted.
sourceFiber.tag = IncompleteClassComponent;
} else {
// When we try rendering again, we should not reuse the current fiber,
// since it's known to be in an inconsistent state. Use a force update to
// prevent a bail out.
const update = createUpdate(NoTimestamp, SyncLane);
update.tag = ForceUpdate;
enqueueUpdate(sourceFiber, update, SyncLane);
if (sourceFiber.tag === ClassComponent) {
const currentSourceFiber = sourceFiber.alternate;
if (currentSourceFiber === null) {
// This is a new mount. Change the tag so it's not mistaken for a
// completed class component. For example, we should not call
// componentWillUnmount if it is deleted.
sourceFiber.tag = IncompleteClassComponent;
} else {
// When we try rendering again, we should not reuse the current fiber,
// since it's known to be in an inconsistent state. Use a force update to
// prevent a bail out.
const update = createUpdate(NoTimestamp, SyncLane);
update.tag = ForceUpdate;
enqueueUpdate(sourceFiber, update, SyncLane);
}
}
}

// The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);

// Exit without suspending.
// The source fiber did not complete. Mark it with Sync priority to
// indicate that it still has pending work.
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
}
return;
}

// Confirmed that the boundary is in a concurrent mode tree. Continue
// with the normal suspend path.
//
Expand Down Expand Up @@ -415,7 +421,6 @@ function throwException(
// TODO: I think we can remove this, since we now use `DidCapture` in
// the begin phase to prevent an early bailout.
workInProgress.lanes = rootRenderLanes;

return;
}
// This boundary already captured during this render. Continue to the next
Expand Down