-
Notifications
You must be signed in to change notification settings - Fork 47.2k
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
Never attach ping listeners in legacy Suspense #22407
Never attach ping listeners in legacy Suspense #22407
Conversation
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.
19c7fb4
to
5cedfd2
Compare
// 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) { |
There was a problem hiding this comment.
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.
Comparing: d174d06...5cedfd2 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
// 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
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:
const didSuspend = (workInProgress.flags & DidCapture) !== NoFlags; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh okay makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough explanation looks good
Summary: This sync includes the following changes: - **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>// - **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>// - **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>// - **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>// - **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>// - **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>// - **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>// - **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>// - **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>// - **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>// - **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>// - **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous ([#22444](facebook/react#22444)) //<Andrew Clark>// - **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>// - **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>// - **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>// - **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>// - **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>// - **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>// - **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>// - **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>// - **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>// - **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>// - **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>// - **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>// - **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>// - **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>// - **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>// - **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>// - **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>// - **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>// - **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>// - **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>// - **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>// - **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions e8feb11...afcb9cd jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D31541359 fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
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.
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 unnecessary 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.
To confirm we have coverage for this branch, I commented it out and ran the test suite. The relevant tests are in ReactLazy-test.js.
I suggest reviewing without whitespace changes: https://github.com/facebook/react/pull/22407/files?diff=split&w=1