Support React.memo and timed-out Suspense trees

[bvaughn]

Support React.memo

• Resolves memo()-wrapped components don't have names #1193
• Commit 4d85b12

Remove React.pure (since it was never released in a stable) and add support for React.memo. Preserve previous custom displayName semantics.

Code

function CounterFunction({count}) {
  return <small>{count}</small>;
}
const A = React.memo(({count}) => <small>{count}</small>);
const B = React.memo(CounterFunction);
const C = React.memo(CounterFunction);
C.displayName = 'MemoCounterFunction';

DevTools

Support timed-out suspense subtrees

• Resolves ☂️ Suspense umbrella task #1189.
• Commit 34ed7be

Backstory: A special case (hack) was added to fiber to maintain state for components within timed-out suspended trees. In this event, React hides the original UI and shows a fallback UI. Rather than destroying the hidden UI, it holds onto it so the state isn't lost (e.g. component state, untracked input values).

DevTools mostly just worked with this hack, but use cases like this one left the Elements panel in a broken state after the suspended render completed– due to an incorrect child->parent pointer.

I'm not sure of a great way to model this within attachRendererFiber (where it probably belongs), but it seemed reasonably easy to handle as a generic reparenting check. So let's discuss? 😄

Misc

• Commit ef8c92d

Also fixed the Elements panel display name for IndeterminateComponent type components.

[gaearon]

Looks good except 34ed7be which worries me. I think this makes DevTools too loose about what is allowed to happen. Can we model it as mounting and unmounting of two different sets of children instead? You should be able to tell from the Suspense state whether it's timed out or not (just like React already does), and then branch somewhere in updateFiber in attachRendererFiber. This would likely mirror custom reconciliation logic. WDYT?

[bvaughn]

Regarding 34ed7be, could you expand on your concern about making DevTools "too loose"? If this newly added code is ever executed, it means that React reparented a node– right? Is there any way it could trigger a false positive? (I'm not totally confident about this change either, just pushing back a little to make sure the initial concern is well founded.)

Can we model it as mounting and unmounting of two different sets of children instead?

This is kind of what I wanted to do initially, but I struggled to think of a way to detect this case in attachRendererFiber, since it isn't stateful.

Maybe I could detect it with something like tag === SuspenseComponent && memoizedState && memoizedState.didTimeout && hasChildOrderChanged but...that felt clunkier than my proposed solution in Store.

I'll give it another pass though.

[bvaughn]

Can we model it as mounting and unmounting of two different sets of children instead?

Another (small) downside of this is that unmounting and remounting would cause the profiler's "renders" count to be reset.

[gaearon]

If this newly added code is ever executed, it means that React reparented a node– right?

Or it could also mean that there is a bug in the attachRendererFiber traversal code. (I caught a few several times when I made changes in the past and I worry new behavior wouldn't have caught my bugs because it would just think we're doing reparenting.)

Since technically we don't support reparenting anywhere else I'm not sure it's worth it to add generic support for it. I'd rather see a targeted fix around this specific use case.

[bvaughn]

Thinking about this more, I think it's actually desirable to model this as a re-parent, since that's actually what's happening.

[bvaughn]

I'm also a little anxious about adding custom logic in updateFiber for this specific Suspense case, since I'm under the impression that it's a hack that we may not always do.

Even though we don't happen to support re-parenting in the general case, it feels like less of a hack to detect this case in a more suspense-and-version-agnostic way.

[gaearon]

¯\(ツ)/¯

I trust your judgement and won't block on this. Might be a good idea to check with @acdlite or @sebmarkbage about whether that hack is good to rely on or not.

Some comments might be helpful. A future reader might be surprised React supports reparenting.

[bvaughn]

Related: facebook/react/pull/14065

[bvaughn]

Here's a shitty, hacky way of not showing the timed out sub tree as thought it committed in the Profiler:

diff --git a/backend/attachRendererFiber.js b/backend/attachRendererFiber.js
index f3b72d5..3384b19 100644
--- a/backend/attachRendererFiber.js
+++ b/backend/attachRendererFiber.js
@@ -170,7 +170,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
 
   // TODO: we might want to change the data structure
   // once we no longer suppport Stack versions of `getData`.
-  function getDataFiber(fiber: Object): DataType {
+  function getDataFiber(fiber: Object, isHidden: boolean = false): DataType {
     var elementType = fiber.elementType;
     var type = fiber.type;
     var key = fiber.key;
@@ -192,6 +192,10 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     var treeBaseDuration = null;
     var memoizedInteractions = null;
 
+    // Flags a Suspense subtree as temporarily hidden.
+    // We do this so that it isn't shown in the Profiler.
+    var isTimedOutSuspense = false;
+
     var resolvedType = type;
     if (typeof type === 'object' && type !== null) {
       if (typeof type.then === 'function') {
@@ -287,7 +291,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
         break;
       case MemoComponent:
       case SimpleMemoComponent:
-        nodeType = 'Special';
+        nodeType = 'Composite';
         if (elementType.displayName) {
           name = elementType.displayName;
         } else {
@@ -342,6 +346,10 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
             name = 'Suspense';
             props = fiber.memoizedProps;
             children = [];
+
+            // If this is a timed-out Suspense component, React will have hidden the first subtree.
+            // We need to mark this, so that tree doesn't appear in Profiler as though it rendered.
+            isTimedOutSuspense = fiber.memoizedState != null && fiber.memoizedState.didTimeout;
             break;
           case PROFILER_NUMBER:
           case PROFILER_SYMBOL_STRING:
@@ -395,6 +403,10 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
       actualDuration,
       actualStartTime,
       treeBaseDuration,
+
+      // Suspense
+      isTimedOutSuspense,
+      isHidden,
     };
   }
 
@@ -491,10 +503,10 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     }
   }
 
-  function enqueueMount(fiber) {
+  function enqueueMount(fiber, isHidden) {
     pendingEvents.push({
       internalInstance: getOpaqueNode(fiber),
-      data: getDataFiber(fiber),
+      data: getDataFiber(fiber, isHidden),
       renderer: rid,
       type: 'mount',
     });
@@ -502,14 +514,14 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     const isRoot = fiber.tag === HostRoot;
     if (isRoot) {
       pendingEvents.push({
-        internalInstance: getOpaqueNode(fiber),
+        internalInstance: getOpaqueNode(fiber, isHidden),
         renderer: rid,
         type: 'root',
       });
     }
   }
 
-  function enqueueUpdateIfNecessary(fiber, hasChildOrderChanged) {
+  function enqueueUpdateIfNecessary(fiber, hasChildOrderChanged, isHidden = false) {
     if (
       !hasChildOrderChanged &&
       !hasDataChanged(fiber.alternate, fiber)
@@ -521,7 +533,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
       if (haveProfilerTimesChanged(fiber.alternate, fiber)) {
         pendingEvents.push({
           internalInstance: getOpaqueNode(fiber),
-          data: getDataFiber(fiber),
+          data: getDataFiber(fiber, isHidden),
           renderer: rid,
           type: 'updateProfileTimes',
         });
@@ -530,7 +542,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     }
     pendingEvents.push({
       internalInstance: getOpaqueNode(fiber),
-      data: getDataFiber(fiber),
+      data: getDataFiber(fiber, isHidden),
       renderer: rid,
       type: 'update',
     });
@@ -565,7 +577,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     });
   }
 
-  function mountFiber(fiber) {
+  function mountFiber(fiber, isHidden = false) {
     // Depth-first.
     // Logs mounting of children first, parents later.
     let node = fiber;
@@ -575,7 +587,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
         node = node.child;
         continue;
       }
-      enqueueMount(node);
+      enqueueMount(node, isHidden);
       if (node == fiber) {
         return;
       }
@@ -586,7 +598,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
       }
       while (node.return) {
         node = node.return;
-        enqueueMount(node);
+        enqueueMount(node, isHidden);
         if (node == fiber) {
           return;
         }
@@ -600,21 +612,32 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
     }
   }
 
-  function updateFiber(nextFiber, prevFiber) {
+  function updateFiber(nextFiber, prevFiber, isHidden) {
+    const isTimedOutSuspense = (
+      nextFiber.tag === ReactTypeOfWork.SuspenseComponent &&
+      nextFiber.memoizedState &&
+      nextFiber.memoizedState.didTimeout
+    );
+
     let hasChildOrderChanged = false;
     if (nextFiber.child !== prevFiber.child) {
+      let isFirstChild = true;
       // If the first child is different, we need to traverse them.
       // Each next child will be either a new child (mount) or an alternate (update).
       let nextChild = nextFiber.child;
       let prevChildAtSameIndex = prevFiber.child;
       while (nextChild) {
+        // React hides the first child of a timed-out Suspense component.
+        const isChildHidden = isHidden || isTimedOutSuspense && isFirstChild;
+        isFirstChild = false;
+
         // We already know children will be referentially different because
         // they are either new mounts or alternates of previous children.
         // Schedule updates and mounts depending on whether alternates exist.
         // We don't track deletions here because they are reported separately.
         if (nextChild.alternate) {
           const prevChild = nextChild.alternate;
-          updateFiber(nextChild, prevChild);
+          updateFiber(nextChild, prevChild, isChildHidden);
           // However we also keep track if the order of the children matches
           // the previous order. They are always different referentially, but
           // if the instances line up conceptually we'll want to know that.
@@ -622,7 +645,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
             hasChildOrderChanged = true;
           }
         } else {
-          mountFiber(nextChild);
+          mountFiber(nextChild, isChildHidden);
           if (!hasChildOrderChanged) {
             hasChildOrderChanged = true;
           }
@@ -640,7 +663,7 @@ function attachRendererFiber(hook: Hook, rid: string, renderer: ReactRenderer):
         hasChildOrderChanged = true;
       }
     }
-    enqueueUpdateIfNecessary(nextFiber, hasChildOrderChanged);
+    enqueueUpdateIfNecessary(nextFiber, hasChildOrderChanged, isHidden);
   }
 
   function walkTree() {
diff --git a/plugins/Profiler/ProfileCollector.js b/plugins/Profiler/ProfileCollector.js
index d802f08..b538d2e 100644
--- a/plugins/Profiler/ProfileCollector.js
+++ b/plugins/Profiler/ProfileCollector.js
@@ -91,7 +91,11 @@ class ProfileCollector {
       return;
     }
 
-    this._committedNodes.add(data.id);
+    // Don't count suspended, hidden components as part of the render.
+    if (!data.isHidden) {
+      this._committedNodes.add(data.id);
+    }
+
     this._maxActualDuration = Math.max(this._maxActualDuration, data.actualDuration);
   };

I'm going to hold off on committing this in hopes that something better will occur to me after I sleep on it.

[gaearon]

Note fiber.memoizedState.didTimeout was removed from state in facebook/react#14083.

[gaearon]

Related PR: #1218

[bvaughn]

Thanks for the pointer to 14083, Dan. Will need to chat with @acdlite and figure out a new heuristic for this I guess.

[bvaughn]

I think I'm going to go ahead and land this change as-is and circle back about the hidden subtree. I chatted with Andrew about the reparenting change. He's not familiar enough with DevTools to say definitively but didn't have any objections to the general concept.

[0xdevalias]

Curious if we know when this will hit release? Seems it isn't in 3.4.2?

• 3.4.2...master

Edit: So seems the 'tags' and 'releases' are out of date on this repo (#1273), but it theoretically should be in the latest version on the chrome store v3.6.

That doesn't seem to match what I am seeing in my project though:

These functional components are written in this style.. will that matter..?

const Foo = React.memo(
  ({
    csrfToken,
    ...otherProps
  }) => {

Edit: Reading this deeper.. I guess the correct way is actually to be manually setting Foo.displayName, which logically makes sense, though figuring out that as a requirement wasn't immediately obvious to me.

In case anyone else hits this:

• https://reactjs.org/docs/react-component.html#displayname