[Transition Tracing] Tracing Marker Name Change in Update Warning (#2…

…4873) We should only support Tracing Marker's name field during component mount. This PR adds a warning if the Tracing Marker's name changes during an update.
facebook · Jul 8, 2022 · dd2d652 · dd2d652
1 parent 80208e7
commit dd2d652
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 14 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -981,6 +981,15 @@ function updateTracingMarkerComponent(
       };
       workInProgress.stateNode = markerInstance;
     }
+  } else {
+    if (__DEV__) {
+      if (current.memoizedProps.name !== workInProgress.pendingProps.name) {
+        console.error(
+          'Changing the name of a tracing marker after mount is not supported. ' +
+            'To remount the tracing marker, pass it a new key.',
+        );
+      }
+    }
   }
 
   const instance: TracingMarkerInstance | null = workInProgress.stateNode;

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -981,6 +981,15 @@ function updateTracingMarkerComponent(
       };
       workInProgress.stateNode = markerInstance;
     }
+  } else {
+    if (__DEV__) {
+      if (current.memoizedProps.name !== workInProgress.pendingProps.name) {
+        console.error(
+          'Changing the name of a tracing marker after mount is not supported. ' +
+            'To remount the tracing marker, pass it a new key.',
+        );
+      }
+    }
   }
 
   const instance: TracingMarkerInstance | null = workInProgress.stateNode;

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -3044,14 +3044,16 @@ function commitPassiveMountOnFiber(
           instance.pendingSuspenseBoundaries === null ||
           instance.pendingSuspenseBoundaries.size === 0
         ) {
-          instance.transitions.forEach(transition => {
-            addMarkerCompleteCallbackToPendingTransition({
-              transition,
-              name: finishedWork.memoizedProps.name,
+          if (instance.transitions !== null) {
+            instance.transitions.forEach(transition => {
+              addMarkerCompleteCallbackToPendingTransition({
+                transition,
+                name: finishedWork.memoizedProps.name,
+              });
             });
-          });
-          instance.transitions = null;
-          instance.pendingSuspenseBoundaries = null;
+            instance.transitions = null;
+            instance.pendingSuspenseBoundaries = null;
+          }
         }
       }
       break;

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -3044,14 +3044,16 @@ function commitPassiveMountOnFiber(
           instance.pendingSuspenseBoundaries === null ||
           instance.pendingSuspenseBoundaries.size === 0
         ) {
-          instance.transitions.forEach(transition => {
-            addMarkerCompleteCallbackToPendingTransition({
-              transition,
-              name: finishedWork.memoizedProps.name,
+          if (instance.transitions !== null) {
+            instance.transitions.forEach(transition => {
+              addMarkerCompleteCallbackToPendingTransition({
+                transition,
+                name: finishedWork.memoizedProps.name,
+              });
             });
-          });
-          instance.transitions = null;
-          instance.pendingSuspenseBoundaries = null;
+            instance.transitions = null;
+            instance.pendingSuspenseBoundaries = null;
+          }
         }
       }
       break;

diff --git a/packages/react-reconciler/src/__tests__/ReactTransitionTracing-test.js b/packages/react-reconciler/src/__tests__/ReactTransitionTracing-test.js
@@ -1059,6 +1059,86 @@ describe('ReactInteractionTracing', () => {
     });
   });
 
+  // @gate enableTransitionTracing
+  it('warns when marker name changes', async () => {
+    const transitionCallbacks = {
+      onTransitionStart: (name, startTime) => {
+        Scheduler.unstable_yieldValue(
+          `onTransitionStart(${name}, ${startTime})`,
+        );
+      },
+      onTransitionComplete: (name, startTime, endTime) => {
+        Scheduler.unstable_yieldValue(
+          `onTransitionComplete(${name}, ${startTime}, ${endTime})`,
+        );
+      },
+      onMarkerComplete: (transitioName, markerName, startTime, endTime) => {
+        Scheduler.unstable_yieldValue(
+          `onMarkerComplete(${transitioName}, ${markerName}, ${startTime}, ${endTime})`,
+        );
+      },
+    };
+    function App({markerName, markerKey}) {
+      return (
+        <React.unstable_TracingMarker name={markerName} key={markerKey}>
+          <Text text={markerName} />
+        </React.unstable_TracingMarker>
+      );
+    }
+
+    const root = ReactNoop.createRoot({transitionCallbacks});
+    await act(async () => {
+      startTransition(
+        () => root.render(<App markerName="one" markerKey="key" />),
+        {
+          name: 'transition one',
+        },
+      );
+      ReactNoop.expire(1000);
+      await advanceTimers(1000);
+      expect(Scheduler).toFlushAndYield([
+        'one',
+        'onTransitionStart(transition one, 0)',
+        'onMarkerComplete(transition one, one, 0, 1000)',
+        'onTransitionComplete(transition one, 0, 1000)',
+      ]);
+      startTransition(
+        () => root.render(<App markerName="two" markerKey="key" />),
+        {
+          name: 'transition two',
+        },
+      );
+      ReactNoop.expire(1000);
+      await advanceTimers(1000);
+      expect(() => {
+        // onMarkerComplete shouldn't be called for transitions with
+        // new keys
+        expect(Scheduler).toFlushAndYield([
+          'two',
+          'onTransitionStart(transition two, 1000)',
+          'onTransitionComplete(transition two, 1000, 2000)',
+        ]);
+      }).toErrorDev(
+        'Changing the name of a tracing marker after mount is not supported.',
+      );
+      startTransition(
+        () => root.render(<App markerName="three" markerKey="new key" />),
+        {
+          name: 'transition three',
+        },
+      );
+      ReactNoop.expire(1000);
+      await advanceTimers(1000);
+      // This should not warn and onMarkerComplete should be called
+      expect(Scheduler).toFlushAndYield([
+        'three',
+        'onTransitionStart(transition three, 2000)',
+        'onMarkerComplete(transition three, three, 2000, 3000)',
+        'onTransitionComplete(transition three, 2000, 3000)',
+      ]);
+    });
+  });
+
   // @gate enableTransitionTracing
   it.skip('marker interaction cancelled when name changes', async () => {
     const transitionCallbacks = {