Skip to content

Commit 5cedfd2

Browse files
acdliteacdlite
committed
Never attach ping listeners in legacy Suspense
I noticed a weird branch where we attach a ping listener even in legacy mode. It's weird because this shouldn't be necessary. Fallbacks always synchronously commit in legacy mode, so pings never happen. (A "ping" is when a suspended promise resolves before the fallback has committed.) It took me a moment to remember why this case exists, but it's related to React.lazy. There's a special case where we suspend 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. However, even though we're switching to the concurrent mode behavior, we don't need to attach a ping listener. So I refactored the logic so that it doesn't escape back into the regular path. It's not really a big deal that we attach an unncessary ping listener, since this case is so unusual. The motivation is not performance related — it's to make the logic clearer, because I'm about to add another case where we trigger a Suspense boundary without attaching a ping listener.
1 parent d174d06 commit 5cedfd2

File tree

2 files changed

+134
-124
lines changed

2 files changed

+134
-124
lines changed

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

+67-62
Original file line numberDiff line numberDiff line change
@@ -297,76 +297,82 @@ function throwException(
297297
wakeables.add(wakeable);
298298
}
299299

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

320-
// We're going to commit this fiber even though it didn't complete.
321-
// But we shouldn't call any lifecycle methods or callbacks. Remove
322-
// all lifecycle effect tags.
323-
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
328+
// We're going to commit this fiber even though it didn't complete.
329+
// But we shouldn't call any lifecycle methods or callbacks. Remove
330+
// all lifecycle effect tags.
331+
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
324332

325-
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
326-
// Another legacy Suspense quirk. In persistent mode, if this is the
327-
// initial mount, override the props of the host container to hide
328-
// its contents.
329-
const currentSuspenseBoundary = workInProgress.alternate;
330-
if (currentSuspenseBoundary === null) {
331-
const offscreenFiber: Fiber = (workInProgress.child: any);
332-
const offscreenContainer = offscreenFiber.child;
333-
if (offscreenContainer !== null) {
334-
const children = offscreenContainer.memoizedProps.children;
335-
const containerProps = getOffscreenContainerProps(
336-
'hidden',
337-
children,
338-
);
339-
offscreenContainer.pendingProps = containerProps;
340-
offscreenContainer.memoizedProps = containerProps;
333+
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
334+
// Another legacy Suspense quirk. In persistent mode, if this is the
335+
// initial mount, override the props of the host container to hide
336+
// its contents.
337+
const currentSuspenseBoundary = workInProgress.alternate;
338+
if (currentSuspenseBoundary === null) {
339+
const offscreenFiber: Fiber = (workInProgress.child: any);
340+
const offscreenContainer = offscreenFiber.child;
341+
if (offscreenContainer !== null) {
342+
const children = offscreenContainer.memoizedProps.children;
343+
const containerProps = getOffscreenContainerProps(
344+
'hidden',
345+
children,
346+
);
347+
offscreenContainer.pendingProps = containerProps;
348+
offscreenContainer.memoizedProps = containerProps;
349+
}
341350
}
342351
}
343-
}
344352

345-
if (sourceFiber.tag === ClassComponent) {
346-
const currentSourceFiber = sourceFiber.alternate;
347-
if (currentSourceFiber === null) {
348-
// This is a new mount. Change the tag so it's not mistaken for a
349-
// completed class component. For example, we should not call
350-
// componentWillUnmount if it is deleted.
351-
sourceFiber.tag = IncompleteClassComponent;
352-
} else {
353-
// When we try rendering again, we should not reuse the current fiber,
354-
// since it's known to be in an inconsistent state. Use a force update to
355-
// prevent a bail out.
356-
const update = createUpdate(NoTimestamp, SyncLane);
357-
update.tag = ForceUpdate;
358-
enqueueUpdate(sourceFiber, update, SyncLane);
353+
if (sourceFiber.tag === ClassComponent) {
354+
const currentSourceFiber = sourceFiber.alternate;
355+
if (currentSourceFiber === null) {
356+
// This is a new mount. Change the tag so it's not mistaken for a
357+
// completed class component. For example, we should not call
358+
// componentWillUnmount if it is deleted.
359+
sourceFiber.tag = IncompleteClassComponent;
360+
} else {
361+
// When we try rendering again, we should not reuse the current fiber,
362+
// since it's known to be in an inconsistent state. Use a force update to
363+
// prevent a bail out.
364+
const update = createUpdate(NoTimestamp, SyncLane);
365+
update.tag = ForceUpdate;
366+
enqueueUpdate(sourceFiber, update, SyncLane);
367+
}
359368
}
360-
}
361-
362-
// The source fiber did not complete. Mark it with Sync priority to
363-
// indicate that it still has pending work.
364-
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
365369

366-
// Exit without suspending.
370+
// The source fiber did not complete. Mark it with Sync priority to
371+
// indicate that it still has pending work.
372+
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
373+
}
367374
return;
368375
}
369-
370376
// Confirmed that the boundary is in a concurrent mode tree. Continue
371377
// with the normal suspend path.
372378
//
@@ -415,7 +421,6 @@ function throwException(
415421
// TODO: I think we can remove this, since we now use `DidCapture` in
416422
// the begin phase to prevent an early bailout.
417423
workInProgress.lanes = rootRenderLanes;
418-
419424
return;
420425
}
421426
// This boundary already captured during this render. Continue to the next

‎packages/react-reconciler/src/ReactFiberThrow.old.js

+67-62
Original file line numberDiff line numberDiff line change
@@ -297,76 +297,82 @@ function throwException(
297297
wakeables.add(wakeable);
298298
}
299299

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

320-
// We're going to commit this fiber even though it didn't complete.
321-
// But we shouldn't call any lifecycle methods or callbacks. Remove
322-
// all lifecycle effect tags.
323-
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
328+
// We're going to commit this fiber even though it didn't complete.
329+
// But we shouldn't call any lifecycle methods or callbacks. Remove
330+
// all lifecycle effect tags.
331+
sourceFiber.flags &= ~(LifecycleEffectMask | Incomplete);
324332

325-
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
326-
// Another legacy Suspense quirk. In persistent mode, if this is the
327-
// initial mount, override the props of the host container to hide
328-
// its contents.
329-
const currentSuspenseBoundary = workInProgress.alternate;
330-
if (currentSuspenseBoundary === null) {
331-
const offscreenFiber: Fiber = (workInProgress.child: any);
332-
const offscreenContainer = offscreenFiber.child;
333-
if (offscreenContainer !== null) {
334-
const children = offscreenContainer.memoizedProps.children;
335-
const containerProps = getOffscreenContainerProps(
336-
'hidden',
337-
children,
338-
);
339-
offscreenContainer.pendingProps = containerProps;
340-
offscreenContainer.memoizedProps = containerProps;
333+
if (supportsPersistence && enablePersistentOffscreenHostContainer) {
334+
// Another legacy Suspense quirk. In persistent mode, if this is the
335+
// initial mount, override the props of the host container to hide
336+
// its contents.
337+
const currentSuspenseBoundary = workInProgress.alternate;
338+
if (currentSuspenseBoundary === null) {
339+
const offscreenFiber: Fiber = (workInProgress.child: any);
340+
const offscreenContainer = offscreenFiber.child;
341+
if (offscreenContainer !== null) {
342+
const children = offscreenContainer.memoizedProps.children;
343+
const containerProps = getOffscreenContainerProps(
344+
'hidden',
345+
children,
346+
);
347+
offscreenContainer.pendingProps = containerProps;
348+
offscreenContainer.memoizedProps = containerProps;
349+
}
341350
}
342351
}
343-
}
344352

345-
if (sourceFiber.tag === ClassComponent) {
346-
const currentSourceFiber = sourceFiber.alternate;
347-
if (currentSourceFiber === null) {
348-
// This is a new mount. Change the tag so it's not mistaken for a
349-
// completed class component. For example, we should not call
350-
// componentWillUnmount if it is deleted.
351-
sourceFiber.tag = IncompleteClassComponent;
352-
} else {
353-
// When we try rendering again, we should not reuse the current fiber,
354-
// since it's known to be in an inconsistent state. Use a force update to
355-
// prevent a bail out.
356-
const update = createUpdate(NoTimestamp, SyncLane);
357-
update.tag = ForceUpdate;
358-
enqueueUpdate(sourceFiber, update, SyncLane);
353+
if (sourceFiber.tag === ClassComponent) {
354+
const currentSourceFiber = sourceFiber.alternate;
355+
if (currentSourceFiber === null) {
356+
// This is a new mount. Change the tag so it's not mistaken for a
357+
// completed class component. For example, we should not call
358+
// componentWillUnmount if it is deleted.
359+
sourceFiber.tag = IncompleteClassComponent;
360+
} else {
361+
// When we try rendering again, we should not reuse the current fiber,
362+
// since it's known to be in an inconsistent state. Use a force update to
363+
// prevent a bail out.
364+
const update = createUpdate(NoTimestamp, SyncLane);
365+
update.tag = ForceUpdate;
366+
enqueueUpdate(sourceFiber, update, SyncLane);
367+
}
359368
}
360-
}
361-
362-
// The source fiber did not complete. Mark it with Sync priority to
363-
// indicate that it still has pending work.
364-
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
365369

366-
// Exit without suspending.
370+
// The source fiber did not complete. Mark it with Sync priority to
371+
// indicate that it still has pending work.
372+
sourceFiber.lanes = mergeLanes(sourceFiber.lanes, SyncLane);
373+
}
367374
return;
368375
}
369-
370376
// Confirmed that the boundary is in a concurrent mode tree. Continue
371377
// with the normal suspend path.
372378
//
@@ -415,7 +421,6 @@ function throwException(
415421
// TODO: I think we can remove this, since we now use `DidCapture` in
416422
// the begin phase to prevent an early bailout.
417423
workInProgress.lanes = rootRenderLanes;
418-
419424
return;
420425
}
421426
// This boundary already captured during this render. Continue to the next

0 commit comments

Comments
 (0)