@samitouri / QOS-React-2 / commits / 3a50d95573

Never attach ping listeners in legacy Suspense (#22407)

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.

Andrew Clark committed Sep 23, 2021 at 14:07 UTC 3a50d95573b7ebdf723e08f6f78c99faf811da0b

2 files changed +134 -124

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

+67 -62

@@ -297,76 +297,82 @@ function throwException(

297 wakeables.add(wakeable);

298 }

299

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;

327

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);

332

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 + }

350 }

351 }

343 - }

352

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 + }

368 }

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);

369

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 + }

374 return;

375 }

369 -

376 // Confirmed that the boundary is in a concurrent mode tree. Continue

377 // with the normal suspend path.

378 //

@@ -415,7 +421,6 @@ function throwException(

421 // TODO: I think we can remove this, since we now use `DidCapture` in

422 // the begin phase to prevent an early bailout.

423 workInProgress.lanes = rootRenderLanes;

418 -

424 return;

425 }

426 // This boundary already captured during this render. Continue to the next

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