Fix useId in strict mode (facebook#22681)

* Fix: useId in strict mode In strict mode, `renderWithHooks` is called twice to flush out side effects. Modying the tree context (`pushTreeId` and `pushTreeFork`) is effectful, so before this fix, the tree context was allocating two slots for a materialized id instead of one. To address, I lifted those calls outside of `renderWithHooks`. This is how I had originally structured it, and it's how Fizz is structured, too. The other solution would be to reset the stack in between the calls but that's also a bit weird because we usually only ever reset the stack during unwind or complete. * Add test for render phase updates Noticed this while fixing the previous bug
zhengjitf · Apr 15, 2022 · c5930e8 · c5930e8
1 parent a29cedb
commit c5930e8
Show file tree

Hide file tree

Showing 8 changed files with 172 additions and 36 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMUseId-test.js b/packages/react-dom/src/__tests__/ReactDOMUseId-test.js
@@ -16,6 +16,7 @@ let ReactDOMFizzServer;
 let Stream;
 let Suspense;
 let useId;
+let useState;
 let document;
 let writable;
 let container;
@@ -35,6 +36,7 @@ describe('useId', () => {
     Stream = require('stream');
     Suspense = React.Suspense;
     useId = React.useId;
+    useState = React.useState;
 
     // Test Environment
     const jsdom = new JSDOM(
@@ -198,6 +200,35 @@ describe('useId', () => {
     `);
   });
 
+  test('StrictMode double rendering', async () => {
+    const {StrictMode} = React;
+
+    function App() {
+      return (
+        <StrictMode>
+          <DivWithId />
+        </StrictMode>
+      );
+    }
+
+    await serverAct(async () => {
+      const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
+      pipe(writable);
+    });
+    await clientAct(async () => {
+      ReactDOM.hydrateRoot(container, <App />);
+    });
+    expect(container).toMatchInlineSnapshot(`
+      <div
+        id="container"
+      >
+        <div
+          id="0"
+        />
+      </div>
+    `);
+  });
+
   test('empty (null) children', async () => {
     // We don't treat empty children different from non-empty ones, which means
     // they get allocated a slot when generating ids. There's no inherent reason
@@ -313,6 +344,32 @@ describe('useId', () => {
     `);
   });
 
+  test('local render phase updates', async () => {
+    function App({swap}) {
+      const [count, setCount] = useState(0);
+      if (count < 3) {
+        setCount(count + 1);
+      }
+      return useId();
+    }
+
+    await serverAct(async () => {
+      const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
+      pipe(writable);
+    });
+    await clientAct(async () => {
+      ReactDOM.hydrateRoot(container, <App />);
+    });
+    expect(container).toMatchInlineSnapshot(`
+      <div
+        id="container"
+      >
+        R:0
+        <!-- -->
+      </div>
+    `);
+  });
+
   test('basic incremental hydration', async () => {
     function App() {
       return (

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -174,7 +174,11 @@ import {
   prepareToReadContext,
   scheduleWorkOnParentPath,
 } from './ReactFiberNewContext.new';
-import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.new';
+import {
+  renderWithHooks,
+  checkDidRenderIdHook,
+  bailoutHooks,
+} from './ReactFiberHooks.new';
 import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.new';
 import {
   getMaskedContext,
@@ -240,6 +244,7 @@ import {
   getForksAtLevel,
   isForkedChild,
   pushTreeId,
+  pushMaterializedTreeId,
 } from './ReactFiberTreeContext.new';
 
 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@@ -365,6 +370,7 @@ function updateForwardRef(
 
   // The rest is a fork of updateFunctionComponent
   let nextChildren;
+  let hasId;
   prepareToReadContext(workInProgress, renderLanes);
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -380,6 +386,7 @@ function updateForwardRef(
       ref,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
       workInProgress.mode & StrictLegacyMode
@@ -394,6 +401,7 @@ function updateForwardRef(
           ref,
           renderLanes,
         );
+        hasId = checkDidRenderIdHook();
       } finally {
         setIsStrictModeForDevtools(false);
       }
@@ -408,6 +416,7 @@ function updateForwardRef(
       ref,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -418,6 +427,10 @@ function updateForwardRef(
     return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
   }
 
+  if (getIsHydrating() && hasId) {
+    pushMaterializedTreeId(workInProgress);
+  }
+
   // React DevTools reads this flag.
   workInProgress.flags |= PerformedWork;
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -970,6 +983,7 @@ function updateFunctionComponent(
   }
 
   let nextChildren;
+  let hasId;
   prepareToReadContext(workInProgress, renderLanes);
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -985,6 +999,7 @@ function updateFunctionComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
       workInProgress.mode & StrictLegacyMode
@@ -999,6 +1014,7 @@ function updateFunctionComponent(
           context,
           renderLanes,
         );
+        hasId = checkDidRenderIdHook();
       } finally {
         setIsStrictModeForDevtools(false);
       }
@@ -1013,6 +1029,7 @@ function updateFunctionComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -1023,6 +1040,10 @@ function updateFunctionComponent(
     return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
   }
 
+  if (getIsHydrating() && hasId) {
+    pushMaterializedTreeId(workInProgress);
+  }
+
   // React DevTools reads this flag.
   workInProgress.flags |= PerformedWork;
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(
 
   prepareToReadContext(workInProgress, renderLanes);
   let value;
+  let hasId;
 
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     setIsRendering(false);
   } else {
     value = renderWithHooks(
@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(
             context,
             renderLanes,
           );
+          hasId = checkDidRenderIdHook();
         } finally {
           setIsStrictModeForDevtools(false);
         }
       }
     }
 
+    if (getIsHydrating() && hasId) {
+      pushMaterializedTreeId(workInProgress);
+    }
+
     reconcileChildren(null, workInProgress, value, renderLanes);
     if (__DEV__) {
       validateFunctionComponentInDev(workInProgress, Component);

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -174,7 +174,11 @@ import {
   prepareToReadContext,
   scheduleWorkOnParentPath,
 } from './ReactFiberNewContext.old';
-import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.old';
+import {
+  renderWithHooks,
+  checkDidRenderIdHook,
+  bailoutHooks,
+} from './ReactFiberHooks.old';
 import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.old';
 import {
   getMaskedContext,
@@ -240,6 +244,7 @@ import {
   getForksAtLevel,
   isForkedChild,
   pushTreeId,
+  pushMaterializedTreeId,
 } from './ReactFiberTreeContext.old';
 
 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@@ -365,6 +370,7 @@ function updateForwardRef(
 
   // The rest is a fork of updateFunctionComponent
   let nextChildren;
+  let hasId;
   prepareToReadContext(workInProgress, renderLanes);
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -380,6 +386,7 @@ function updateForwardRef(
       ref,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
       workInProgress.mode & StrictLegacyMode
@@ -394,6 +401,7 @@ function updateForwardRef(
           ref,
           renderLanes,
         );
+        hasId = checkDidRenderIdHook();
       } finally {
         setIsStrictModeForDevtools(false);
       }
@@ -408,6 +416,7 @@ function updateForwardRef(
       ref,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -418,6 +427,10 @@ function updateForwardRef(
     return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
   }
 
+  if (getIsHydrating() && hasId) {
+    pushMaterializedTreeId(workInProgress);
+  }
+
   // React DevTools reads this flag.
   workInProgress.flags |= PerformedWork;
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -970,6 +983,7 @@ function updateFunctionComponent(
   }
 
   let nextChildren;
+  let hasId;
   prepareToReadContext(workInProgress, renderLanes);
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -985,6 +999,7 @@ function updateFunctionComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     if (
       debugRenderPhaseSideEffectsForStrictMode &&
       workInProgress.mode & StrictLegacyMode
@@ -999,6 +1014,7 @@ function updateFunctionComponent(
           context,
           renderLanes,
         );
+        hasId = checkDidRenderIdHook();
       } finally {
         setIsStrictModeForDevtools(false);
       }
@@ -1013,6 +1029,7 @@ function updateFunctionComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -1023,6 +1040,10 @@ function updateFunctionComponent(
     return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
   }
 
+  if (getIsHydrating() && hasId) {
+    pushMaterializedTreeId(workInProgress);
+  }
+
   // React DevTools reads this flag.
   workInProgress.flags |= PerformedWork;
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(
 
   prepareToReadContext(workInProgress, renderLanes);
   let value;
+  let hasId;
 
   if (enableSchedulingProfiler) {
     markComponentRenderStarted(workInProgress);
@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
     setIsRendering(false);
   } else {
     value = renderWithHooks(
@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
       context,
       renderLanes,
     );
+    hasId = checkDidRenderIdHook();
   }
   if (enableSchedulingProfiler) {
     markComponentRenderStopped();
@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(
             context,
             renderLanes,
           );
+          hasId = checkDidRenderIdHook();
         } finally {
           setIsStrictModeForDevtools(false);
         }
       }
     }
 
+    if (getIsHydrating() && hasId) {
+      pushMaterializedTreeId(workInProgress);
+    }
+
     reconcileChildren(null, workInProgress, value, renderLanes);
     if (__DEV__) {
       validateFunctionComponentInDev(workInProgress, Component);