[DevTools][Transition Tracing] Added support for Suspense Boundaries #23365

lunaruan · 2022-02-25T22:48:00Z

This PR:

During the passive effect complete phase for Offscreen, we add all the transitions that were added to the update queue in the render phase to the transitions set on the memoziedState. We also add the stateNode for the Offscreen component to the root pendingSuspenseBoundaries map if the suspense boundary has gone from shown to fallback. We remove the stateNode if the boundary goes from fallback to shown.
During the passive effect complete phase for the HostRoot, for each transition that was initiated during this commit, we add a pending transitionStart callback. We also add them to the transition set on the memoizedState for the HostRoot. If the root pendingSuspenseBoundaries is empty, we add a pending transitionComplete callback.

sizebot · 2022-02-25T22:52:30Z

Comparing: c5e039d...bc535a8

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	131.48 kB	131.49 kB	=	42.14 kB	42.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.72 kB	136.73 kB	=	43.69 kB	43.69 kB
facebook-www/ReactDOM-prod.classic.js	=	441.11 kB	441.13 kB	=	80.41 kB	80.41 kB
facebook-www/ReactDOM-prod.modern.js	=	426.32 kB	426.34 kB	=	78.23 kB	78.23 kB
facebook-www/ReactDOMForked-prod.classic.js	=	441.11 kB	441.13 kB	=	80.41 kB	80.42 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against bc535a8

acdlite · 2022-02-28T16:45:27Z

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

-            addTransitionStartCallbackToPendingTransition({
-              transitionName: transition.name,
-              startTime: transition.startTime,
+    if (flags & SuspenseToggle) {


We should be able to move all this to the passive phase, right?

Actually we can't because the end time might be incorrect because the passive phase could be called after or before paint and it's not guaranteed when, which is why it lives in the sync commit phase

The only thing you have to do in the commit phase is read the time, which you can then access in the passive phase

But how would you know which time you should use?

I don't understand the question. You would read the current time in the mutation phase, then pass that timestamp to the passive phase. Then all the logic after that is the same.

lunaruan · 2022-03-31T15:23:09Z

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

+          const currentTransitions = getSuspendedTransitions();
+          if (currentTransitions !== null) {
+            const primaryChildUpdateQueue: OffscreenQueue = {
+              transitions: currentTransitions,


Add pendingSuspenseBoundaries here and use fiberRoot.current

acdlite · 2022-03-31T14:44:51Z

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

+) {
+  if (enableTransitionTracing) {
+    const rootTransitions = getWorkInProgressTransitions();
+    push(transitionStack, [rootTransitions], workInProgress);


If rootTransitions is null it should push null, no need for the extra array

acdlite · 2022-03-31T14:45:48Z

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

@@ -28,18 +29,17 @@ export type Transition = {
  startTime: number,
 };

+export type Transitions = Array<Transition> | null;


Do we really need these type aliases?

acdlite · 2022-03-31T14:47:47Z

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

  }
+
+  if (enableTransitionTracing) {
+    if (current !== null) {


Nit: We shouldn't check current twice. Combine with check above.

acdlite · 2022-03-31T14:55:02Z

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

@@ -746,14 +750,14 @@ function updateOffscreenComponent(

      subtreeRenderLanes = mergeLanes(prevState.baseLanes, renderLanes);

-      if (enableCache) {
+      if (enableCache || enableTransitionTracing) {


Refactor to make two separate branches

acdlite · 2022-03-31T14:55:10Z

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

  cloneUpdateQueue(current, workInProgress);
  processUpdateQueue(workInProgress, nextProps, null, renderLanes);

  const nextState: RootState = workInProgress.memoizedState;
  const root: FiberRoot = workInProgress.stateNode;

+  if (enableCache || enableTransitionTracing) {


acdlite · 2022-04-01T18:13:42Z

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

@@ -655,13 +657,14 @@ function updateOffscreenComponent(
      const nextState: OffscreenState = {
        baseLanes: NoLanes,
        cachePool: null,
+        transitions: null,


Could you add a TODO here to consider how Offscreen should work with transitions?

acdlite · 2022-04-01T18:15:01Z

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

+function addOrRemovePendingBoundariesOnRoot(
+  finishedWork: Fiber,
+  name: string | null,
+  stateNode: OffscreenInstance,


Nit: Maybe call this offscreen instance instead

acdlite · 2022-04-01T18:24:11Z

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

+          case HostRoot: {
+            // Get the transitions that were initiatized during the render
+            // and add a start transition callback for each of them
+            const currentTransitions = getWorkInProgressTransitions();


Pass as argument to commitRoot

acdlite · 2022-04-01T18:27:54Z

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

+
+              addOrRemovePendingBoundariesOnRoot(
+                fiber,
+                props.name || null,


Use Suspense name

lunaruan · 2022-04-07T15:18:28Z

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

@@ -3572,14 +3637,13 @@ function attemptEarlyBailoutIfNoScheduledUpdate(
    case HostRoot:
      pushHostRootContext(workInProgress);
      const root: FiberRoot = workInProgress.stateNode;
+      pushRootTransition(workInProgress, root, renderLanes);


add if (enableTransitionTracing)

acdlite · 2022-04-07T15:19:34Z

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

-    // once we add tracing markers
-    setRootPendingSuspenseBoundaries(pendingSuspenseBoundaries);
-  }
+  const pendingSuspenseBoundaries = nextState.pendingSuspenseBoundaries || null;


Don't need this variable anymore

It's used below in the override states

lunaruan · 2022-04-07T15:20:52Z

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

+        if (currentTransitions !== null) {
+          const primaryChildUpdateQueue: OffscreenQueue = {
+            transitions: currentTransitions,
+            rootMemoizedState:


pass the whole root

acdlite · 2022-04-07T15:21:16Z

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

+        if (currentTransitions !== null) {
+          const primaryChildUpdateQueue: OffscreenQueue = {
+            transitions: currentTransitions,
+            rootMemoizedState:


Can put the whole root object in the queue

acdlite · 2022-04-07T15:30:58Z

packages/react-reconciler/src/ReactFiberOffscreenComponent.js

@@ -31,11 +31,12 @@ export type OffscreenState = {|
  // order to unhide the component.
  baseLanes: Lanes,
  cachePool: SpawnedCachePool | null,
-  transitions: LazyTransitions | null,
+  transitions: Array<Array<Transition> | null> | null,


This could be a Set instead

lunaruan · 2022-04-07T15:33:20Z

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

+// comparing the arrays of transitions when we combine them and storing them
+// and filtering out the duplicates, we will instead store the unprocessed transitions
+// in an array of arrays and actually filter them in the passive phase.
+const transitionStack: StackCursor<Array<Array<Transition> | null> | null> = createCursor(


Make this just an Array and make the transition field a Set

lunaruan · 2022-04-07T15:36:33Z

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

@@ -867,7 +867,8 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
  // Flush any pending passive effects before deciding which lanes to work on,
  // in case they schedule additional work.
  const originalCallbackNode = root.callbackNode;
-  const didFlushPassiveEffects = flushPassiveEffects();
+  // workInProgressTransitions should be what it in the previous render
+  const didFlushPassiveEffects = flushPassiveEffects(workInProgressTransitions);


Instead of doing this make it a work loop variable (pendingPassiveEffectsRemainingLanes)

acdlite · 2022-04-07T15:36:45Z

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

@@ -2089,7 +2104,7 @@ function commitRootImpl(
      rootDoesHavePassiveEffects = true;
      pendingPassiveEffectsRemainingLanes = remainingLanes;
      scheduleCallback(NormalSchedulerPriority, () => {
-        flushPassiveEffects();
+        flushPassiveEffects(transitions);


Nit: Use a work loop variable instead, just for consistency's sake

acdlite · 2022-04-13T17:02:58Z

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

+  // that doesnt change from render to render. This way we can
+  // distinguish between different Offscreen instances (vs. the same
+  // Offscreen instance with different fibers)
+  const offscreenInstance = finishedWork.stateNode;


Suggested change

const offscreenInstance = finishedWork.stateNode;

const offscreenInstance: OffscreenInstance = finishedWork.stateNode;

acdlite · 2022-04-13T17:09:54Z

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

+
+  let prevState: SuspenseState | null = null;
+  if (
+    finishedWork.alternate !== null &&


Nit: Assign the alternate to a variable, perhaps called previous or something. We try to minimize direct access to alternate since it's going to be refactored away in the future.

acdlite · 2022-04-13T17:10:39Z

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

+  }
+
+  if (rootPendingBoundaries !== null) {
+    if (finishedWork.alternate === null) {


same thing as above

acdlite · 2022-04-13T17:13:19Z

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

@@ -1067,6 +1069,77 @@ function reappearLayoutEffectsOnFiber(node: Fiber) {
  }
 }

+function addOrRemovePendingBoundariesOnRoot(


Can you rename this so it's clearer it's related to transitions? Maybe commitTransitionProgress or something

acdlite · 2022-04-13T17:13:41Z

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

@@ -1067,6 +1069,77 @@ function reappearLayoutEffectsOnFiber(node: Fiber) {
  }
 }

+function addOrRemovePendingBoundariesOnRoot(
+  finishedRoot: FiberRoot,
+  finishedWork: Fiber,


also rename this so it's more obvious it's an offscreen fiber

acdlite · 2022-04-13T17:16:48Z

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

+  const isHidden = nextState !== null;
+
+  const rootPendingBoundaries =
+    finishedRoot.current.memoizedState.pendingSuspenseBoundaries;


In general, whenever you access an untyped field like memoizedState make sure you cast it to the correct type before using it

Suggested change

finishedRoot.current.memoizedState.pendingSuspenseBoundaries;

const rootState: RootState = finishedRoot.current.memoizedState;

Also, this can be in a follow up, but we should consider moving the pending suspense boundaries to the FiberRoot instead of the HostRoot fiber. Just for consistency with the other mutable ref-like fields. Could add a TODO for this.

acdlite · 2022-04-13T17:18:55Z

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

+        // Get the transitions that were initiatized during the render
+        // and add a start transition callback for each of them
+        const state = finishedWork.memoizedState;
+        if (state.transitions === null) {


Since it's a mutable field, this should probably live on the FiberRoot, too. Can do in a follow up.

Nit: Could you add a TODO for this so we don't forget?

acdlite · 2022-04-13T17:21:49Z

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

+        }
+
+        const pendingSuspenseBoundaries = state.pendingSuspenseBoundaries;
+        const processedTransitions = new Set();


Why is this extra set necessary? Couldn't you check if it's already in pendingSuspenseBoundaries instead?

This was to avoid adding duplicate transitionComplete calls if the same transition is in pendingTransitions twice

acdlite · 2022-04-13T17:28:25Z

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

+          if (!processedTransitions.has(transition)) {
+            if (
+              pendingSuspenseBoundaries === null ||
+              pendingSuspenseBoundaries.size === 0


I think the other way this could work is to call addTransitionCompleteCallbackToPendingTransition whenever the last pending Suspense boundary is removed from the set, in addOrRemovePendingBoundariesOnRoot. Did you try that?

We could do that, but we'd have to know and get all the pendingTransitions and processedTransitions on the root, so it doesn't really save us any work. We can do that instead if you prefer. I don't feel strongly either way. Keeping this code in the Host Root seems more intuitive to me though.

acdlite · 2022-05-16T14:46:58Z

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

+      }
+    }
+
+    if (rootPendingBoundaries.size === 0 && rootTransitions !== null) {


Nit: Move this right after the .delete call above since logically that's the only case where the count could have reached 0.

acdlite · 2022-05-16T14:48:21Z

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

+        const pendingSuspenseBoundaries = state.pendingSuspenseBoundaries;
+
+        // Initial render
+        if (committedTransitions != null) {


Suggested change

if (committedTransitions != null) {

if (committedTransitions !== null) {

acdlite · 2022-05-16T14:52:49Z

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

+            // Add all the transitions saved in the update queue during
+            // the render phase (ie the transitions associated with this boundary)
+            // into the transitions set.
+            if (transitions != null) {


Suggested change

if (transitions != null) {

if (transitions !== null) {

acdlite · 2022-05-16T14:53:22Z

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

+function commitTransitionProgress(
+  finishedRoot: FiberRoot,
+  offscreenFiber: Fiber,
+) {


Nit: Wrap this in a feature flag

Summary: This sync includes the following changes: - **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>// - **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>// - **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>// - **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>// - **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>// - **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>// - **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>// - **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>// - **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>// - **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>// - **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>// - **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>// - **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>// - **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>// - **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>// - **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>// - **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>// - **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>// - **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>// - **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー中村 Bashurov>// Changelog: [General][Changed] - React Native sync for revisions bd4784c...d300ceb jest_e2e[run_all_tests] Reviewed By: cortinico, kacieb Differential Revision: D36874368 fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 25, 2022

acdlite reviewed Feb 28, 2022

View reviewed changes

lunaruan force-pushed the suspense_boundary branch 4 times, most recently from 5fe7487 to c7dd02c Compare March 1, 2022 21:37

lunaruan force-pushed the suspense_boundary branch 6 times, most recently from 88377a0 to 87dba57 Compare March 22, 2022 16:51

lunaruan commented Mar 31, 2022

View reviewed changes

acdlite reviewed Apr 1, 2022

View reviewed changes

lunaruan force-pushed the suspense_boundary branch from 5c65303 to f560514 Compare April 5, 2022 19:49

lunaruan commented Apr 7, 2022

View reviewed changes

acdlite reviewed Apr 7, 2022

View reviewed changes

lunaruan commented Apr 7, 2022

View reviewed changes

acdlite reviewed Apr 7, 2022

View reviewed changes

lunaruan commented Apr 7, 2022

View reviewed changes

acdlite reviewed Apr 7, 2022

View reviewed changes

lunaruan force-pushed the suspense_boundary branch 5 times, most recently from eed8fa3 to dd5768c Compare April 11, 2022 22:44

lunaruan force-pushed the suspense_boundary branch 2 times, most recently from 800c1d9 to 64e441f Compare April 12, 2022 16:35

acdlite reviewed Apr 13, 2022

View reviewed changes

lunaruan force-pushed the suspense_boundary branch from 64e441f to 65643b0 Compare April 14, 2022 20:47

lunaruan force-pushed the suspense_boundary branch from c17ca59 to a634f55 Compare May 12, 2022 21:31

acdlite reviewed May 16, 2022

View reviewed changes

acdlite approved these changes May 16, 2022

View reviewed changes

lunaruan added 3 commits May 16, 2022 13:07

commit work

50b43b0

test

221fb52

commit work

bc535a8

lunaruan force-pushed the suspense_boundary branch from a634f55 to bc535a8 Compare May 16, 2022 17:08

lunaruan merged commit 357a613 into facebook:main May 16, 2022

	const offscreenInstance = finishedWork.stateNode;
	const offscreenInstance: OffscreenInstance = finishedWork.stateNode;

	finishedRoot.current.memoizedState.pendingSuspenseBoundaries;
	const rootState: RootState = finishedRoot.current.memoizedState;

	if (committedTransitions != null) {
	if (committedTransitions !== null) {

[DevTools][Transition Tracing] Added support for Suspense Boundaries #23365

[DevTools][Transition Tracing] Added support for Suspense Boundaries #23365

Conversation

lunaruan commented Feb 25, 2022 • edited Loading

sizebot commented Feb 25, 2022 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

lunaruan Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunaruan commented Feb 25, 2022 •

edited

Loading

sizebot commented Feb 25, 2022 •

edited

Loading

lunaruan Feb 28, 2022 •

edited

Loading

acdlite Apr 7, 2022 •

edited

Loading