Call Profiler onRender after mutations (facebook#13572)

jetoneza · Jan 23, 2019 · 34c5817 · 34c5817
1 parent 1b9fa05
commit 34c5817
Show file tree

Hide file tree

Showing 3 changed files with 61 additions and 35 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -326,7 +326,30 @@ function commitLifeCycles(
       return;
     }
     case Profiler: {
-      // We have no life-cycles associated with Profiler.
+      if (enableProfilerTimer) {
+        const onRender = finishedWork.memoizedProps.onRender;
+
+        if (enableSchedulerTracking) {
+          onRender(
+            finishedWork.memoizedProps.id,
+            current === null ? 'mount' : 'update',
+            finishedWork.actualDuration,
+            finishedWork.treeBaseDuration,
+            finishedWork.actualStartTime,
+            getCommitTime(),
+            finishedRoot.memoizedInteractions,
+          );
+        } else {
+          onRender(
+            finishedWork.memoizedProps.id,
+            current === null ? 'mount' : 'update',
+            finishedWork.actualDuration,
+            finishedWork.treeBaseDuration,
+            finishedWork.actualStartTime,
+            getCommitTime(),
+          );
+        }
+      }
       return;
     }
     case PlaceholderComponent: {
@@ -781,11 +804,7 @@ function commitDeletion(current: Fiber): void {
   detachFiber(current);
 }
 
-function commitWork(
-  root: FiberRoot,
-  current: Fiber | null,
-  finishedWork: Fiber,
-): void {
+function commitWork(current: Fiber | null, finishedWork: Fiber): void {
   if (!supportsMutation) {
     commitContainer(finishedWork);
     return;
@@ -842,30 +861,6 @@ function commitWork(
       return;
     }
     case Profiler: {
-      if (enableProfilerTimer) {
-        const onRender = finishedWork.memoizedProps.onRender;
-
-        if (enableSchedulerTracking) {
-          onRender(
-            finishedWork.memoizedProps.id,
-            current === null ? 'mount' : 'update',
-            finishedWork.actualDuration,
-            finishedWork.treeBaseDuration,
-            finishedWork.actualStartTime,
-            getCommitTime(),
-            root.memoizedInteractions,
-          );
-        } else {
-          onRender(
-            finishedWork.memoizedProps.id,
-            current === null ? 'mount' : 'update',
-            finishedWork.actualDuration,
-            finishedWork.treeBaseDuration,
-            finishedWork.actualStartTime,
-            getCommitTime(),
-          );
-        }
-      }
       return;
     }
     case PlaceholderComponent: {

diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js
@@ -370,7 +370,7 @@ function resetStack() {
   nextUnitOfWork = null;
 }
 
-function commitAllHostEffects(root: FiberRoot) {
+function commitAllHostEffects() {
   while (nextEffect !== null) {
     if (__DEV__) {
       ReactCurrentFiber.setCurrentFiber(nextEffect);
@@ -415,12 +415,12 @@ function commitAllHostEffects(root: FiberRoot) {
 
         // Update
         const current = nextEffect.alternate;
-        commitWork(root, current, nextEffect);
+        commitWork(current, nextEffect);
         break;
       }
       case Update: {
         const current = nextEffect.alternate;
-        commitWork(root, current, nextEffect);
+        commitWork(current, nextEffect);
         break;
       }
       case Deletion: {
@@ -652,14 +652,14 @@ function commitRoot(root: FiberRoot, finishedWork: Fiber): void {
     let didError = false;
     let error;
     if (__DEV__) {
-      invokeGuardedCallback(null, commitAllHostEffects, null, root);
+      invokeGuardedCallback(null, commitAllHostEffects, null);
       if (hasCaughtError()) {
         didError = true;
         error = clearCaughtError();
       }
     } else {
       try {
-        commitAllHostEffects(root);
+        commitAllHostEffects();
       } catch (e) {
         didError = true;
         error = e;

diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js
@@ -1092,6 +1092,37 @@ describe('Profiler', () => {
         expect(updateCall[3]).toBe(1); // base time
         expect(updateCall[4]).toBe(27); // start time
       });
+
+      it('should not be called until after mutations', () => {
+        let classComponentMounted = false;
+        const callback = jest.fn(
+          (id, phase, actualDuration, baseDuration, startTime, commitTime) => {
+            // Don't call this hook until after mutations
+            expect(classComponentMounted).toBe(true);
+            // But the commit time should reflect pre-mutation
+            expect(commitTime).toBe(2);
+          },
+        );
+
+        class ClassComponent extends React.Component {
+          componentDidMount() {
+            advanceTimeBy(5);
+            classComponentMounted = true;
+          }
+          render() {
+            advanceTimeBy(2);
+            return null;
+          }
+        }
+
+        ReactTestRenderer.create(
+          <React.unstable_Profiler id="test" onRender={callback}>
+            <ClassComponent />
+          </React.unstable_Profiler>,
+        );
+
+        expect(callback).toHaveBeenCalledTimes(1);
+      });
     });
   });