Skip to content

Commit 00ced1e

Browse files
acdliteacdlite
authored
Fix useId in strict mode (#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
1 parent 9fb3442 commit 00ced1e

8 files changed

+172
-36
lines changed

Diff for: ‎packages/react-dom/src/__tests__/ReactDOMUseId-test.js

+57
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ let ReactDOMFizzServer;
1616
let Stream;
1717
let Suspense;
1818
let useId;
19+
let useState;
1920
let document;
2021
let writable;
2122
let container;
@@ -35,6 +36,7 @@ describe('useId', () => {
3536
Stream = require('stream');
3637
Suspense = React.Suspense;
3738
useId = React.useId;
39+
useState = React.useState;
3840

3941
// Test Environment
4042
const jsdom = new JSDOM(
@@ -198,6 +200,35 @@ describe('useId', () => {
198200
`);
199201
});
200202

203+
test('StrictMode double rendering', async () => {
204+
const {StrictMode} = React;
205+
206+
function App() {
207+
return (
208+
<StrictMode>
209+
<DivWithId />
210+
</StrictMode>
211+
);
212+
}
213+
214+
await serverAct(async () => {
215+
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
216+
pipe(writable);
217+
});
218+
await clientAct(async () => {
219+
ReactDOM.hydrateRoot(container, <App />);
220+
});
221+
expect(container).toMatchInlineSnapshot(`
222+
<div
223+
id="container"
224+
>
225+
<div
226+
id="0"
227+
/>
228+
</div>
229+
`);
230+
});
231+
201232
test('empty (null) children', async () => {
202233
// We don't treat empty children different from non-empty ones, which means
203234
// they get allocated a slot when generating ids. There's no inherent reason
@@ -313,6 +344,32 @@ describe('useId', () => {
313344
`);
314345
});
315346

347+
test('local render phase updates', async () => {
348+
function App({swap}) {
349+
const [count, setCount] = useState(0);
350+
if (count < 3) {
351+
setCount(count + 1);
352+
}
353+
return useId();
354+
}
355+
356+
await serverAct(async () => {
357+
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(<App />);
358+
pipe(writable);
359+
});
360+
await clientAct(async () => {
361+
ReactDOM.hydrateRoot(container, <App />);
362+
});
363+
expect(container).toMatchInlineSnapshot(`
364+
<div
365+
id="container"
366+
>
367+
R:0
368+
<!-- -->
369+
</div>
370+
`);
371+
});
372+
316373
test('basic incremental hydration', async () => {
317374
function App() {
318375
return (

Diff for: ‎packages/react-reconciler/src/ReactFiberBeginWork.new.js

+30-1
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,11 @@ import {
174174
prepareToReadContext,
175175
scheduleWorkOnParentPath,
176176
} from './ReactFiberNewContext.new';
177-
import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.new';
177+
import {
178+
renderWithHooks,
179+
checkDidRenderIdHook,
180+
bailoutHooks,
181+
} from './ReactFiberHooks.new';
178182
import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.new';
179183
import {
180184
getMaskedContext,
@@ -240,6 +244,7 @@ import {
240244
getForksAtLevel,
241245
isForkedChild,
242246
pushTreeId,
247+
pushMaterializedTreeId,
243248
} from './ReactFiberTreeContext.new';
244249

245250
const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@@ -365,6 +370,7 @@ function updateForwardRef(
365370

366371
// The rest is a fork of updateFunctionComponent
367372
let nextChildren;
373+
let hasId;
368374
prepareToReadContext(workInProgress, renderLanes);
369375
if (enableSchedulingProfiler) {
370376
markComponentRenderStarted(workInProgress);
@@ -380,6 +386,7 @@ function updateForwardRef(
380386
ref,
381387
renderLanes,
382388
);
389+
hasId = checkDidRenderIdHook();
383390
if (
384391
debugRenderPhaseSideEffectsForStrictMode &&
385392
workInProgress.mode & StrictLegacyMode
@@ -394,6 +401,7 @@ function updateForwardRef(
394401
ref,
395402
renderLanes,
396403
);
404+
hasId = checkDidRenderIdHook();
397405
} finally {
398406
setIsStrictModeForDevtools(false);
399407
}
@@ -408,6 +416,7 @@ function updateForwardRef(
408416
ref,
409417
renderLanes,
410418
);
419+
hasId = checkDidRenderIdHook();
411420
}
412421
if (enableSchedulingProfiler) {
413422
markComponentRenderStopped();
@@ -418,6 +427,10 @@ function updateForwardRef(
418427
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
419428
}
420429

430+
if (getIsHydrating() && hasId) {
431+
pushMaterializedTreeId(workInProgress);
432+
}
433+
421434
// React DevTools reads this flag.
422435
workInProgress.flags |= PerformedWork;
423436
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -970,6 +983,7 @@ function updateFunctionComponent(
970983
}
971984

972985
let nextChildren;
986+
let hasId;
973987
prepareToReadContext(workInProgress, renderLanes);
974988
if (enableSchedulingProfiler) {
975989
markComponentRenderStarted(workInProgress);
@@ -985,6 +999,7 @@ function updateFunctionComponent(
985999
context,
9861000
renderLanes,
9871001
);
1002+
hasId = checkDidRenderIdHook();
9881003
if (
9891004
debugRenderPhaseSideEffectsForStrictMode &&
9901005
workInProgress.mode & StrictLegacyMode
@@ -999,6 +1014,7 @@ function updateFunctionComponent(
9991014
context,
10001015
renderLanes,
10011016
);
1017+
hasId = checkDidRenderIdHook();
10021018
} finally {
10031019
setIsStrictModeForDevtools(false);
10041020
}
@@ -1013,6 +1029,7 @@ function updateFunctionComponent(
10131029
context,
10141030
renderLanes,
10151031
);
1032+
hasId = checkDidRenderIdHook();
10161033
}
10171034
if (enableSchedulingProfiler) {
10181035
markComponentRenderStopped();
@@ -1023,6 +1040,10 @@ function updateFunctionComponent(
10231040
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
10241041
}
10251042

1043+
if (getIsHydrating() && hasId) {
1044+
pushMaterializedTreeId(workInProgress);
1045+
}
1046+
10261047
// React DevTools reads this flag.
10271048
workInProgress.flags |= PerformedWork;
10281049
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(
15931614

15941615
prepareToReadContext(workInProgress, renderLanes);
15951616
let value;
1617+
let hasId;
15961618

15971619
if (enableSchedulingProfiler) {
15981620
markComponentRenderStarted(workInProgress);
@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
16291651
context,
16301652
renderLanes,
16311653
);
1654+
hasId = checkDidRenderIdHook();
16321655
setIsRendering(false);
16331656
} else {
16341657
value = renderWithHooks(
@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
16391662
context,
16401663
renderLanes,
16411664
);
1665+
hasId = checkDidRenderIdHook();
16421666
}
16431667
if (enableSchedulingProfiler) {
16441668
markComponentRenderStopped();
@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(
17581782
context,
17591783
renderLanes,
17601784
);
1785+
hasId = checkDidRenderIdHook();
17611786
} finally {
17621787
setIsStrictModeForDevtools(false);
17631788
}
17641789
}
17651790
}
17661791

1792+
if (getIsHydrating() && hasId) {
1793+
pushMaterializedTreeId(workInProgress);
1794+
}
1795+
17671796
reconcileChildren(null, workInProgress, value, renderLanes);
17681797
if (__DEV__) {
17691798
validateFunctionComponentInDev(workInProgress, Component);

Diff for: ‎packages/react-reconciler/src/ReactFiberBeginWork.old.js

+30-1
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,11 @@ import {
174174
prepareToReadContext,
175175
scheduleWorkOnParentPath,
176176
} from './ReactFiberNewContext.old';
177-
import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.old';
177+
import {
178+
renderWithHooks,
179+
checkDidRenderIdHook,
180+
bailoutHooks,
181+
} from './ReactFiberHooks.old';
178182
import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.old';
179183
import {
180184
getMaskedContext,
@@ -240,6 +244,7 @@ import {
240244
getForksAtLevel,
241245
isForkedChild,
242246
pushTreeId,
247+
pushMaterializedTreeId,
243248
} from './ReactFiberTreeContext.old';
244249

245250
const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
@@ -365,6 +370,7 @@ function updateForwardRef(
365370

366371
// The rest is a fork of updateFunctionComponent
367372
let nextChildren;
373+
let hasId;
368374
prepareToReadContext(workInProgress, renderLanes);
369375
if (enableSchedulingProfiler) {
370376
markComponentRenderStarted(workInProgress);
@@ -380,6 +386,7 @@ function updateForwardRef(
380386
ref,
381387
renderLanes,
382388
);
389+
hasId = checkDidRenderIdHook();
383390
if (
384391
debugRenderPhaseSideEffectsForStrictMode &&
385392
workInProgress.mode & StrictLegacyMode
@@ -394,6 +401,7 @@ function updateForwardRef(
394401
ref,
395402
renderLanes,
396403
);
404+
hasId = checkDidRenderIdHook();
397405
} finally {
398406
setIsStrictModeForDevtools(false);
399407
}
@@ -408,6 +416,7 @@ function updateForwardRef(
408416
ref,
409417
renderLanes,
410418
);
419+
hasId = checkDidRenderIdHook();
411420
}
412421
if (enableSchedulingProfiler) {
413422
markComponentRenderStopped();
@@ -418,6 +427,10 @@ function updateForwardRef(
418427
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
419428
}
420429

430+
if (getIsHydrating() && hasId) {
431+
pushMaterializedTreeId(workInProgress);
432+
}
433+
421434
// React DevTools reads this flag.
422435
workInProgress.flags |= PerformedWork;
423436
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -970,6 +983,7 @@ function updateFunctionComponent(
970983
}
971984

972985
let nextChildren;
986+
let hasId;
973987
prepareToReadContext(workInProgress, renderLanes);
974988
if (enableSchedulingProfiler) {
975989
markComponentRenderStarted(workInProgress);
@@ -985,6 +999,7 @@ function updateFunctionComponent(
985999
context,
9861000
renderLanes,
9871001
);
1002+
hasId = checkDidRenderIdHook();
9881003
if (
9891004
debugRenderPhaseSideEffectsForStrictMode &&
9901005
workInProgress.mode & StrictLegacyMode
@@ -999,6 +1014,7 @@ function updateFunctionComponent(
9991014
context,
10001015
renderLanes,
10011016
);
1017+
hasId = checkDidRenderIdHook();
10021018
} finally {
10031019
setIsStrictModeForDevtools(false);
10041020
}
@@ -1013,6 +1029,7 @@ function updateFunctionComponent(
10131029
context,
10141030
renderLanes,
10151031
);
1032+
hasId = checkDidRenderIdHook();
10161033
}
10171034
if (enableSchedulingProfiler) {
10181035
markComponentRenderStopped();
@@ -1023,6 +1040,10 @@ function updateFunctionComponent(
10231040
return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);
10241041
}
10251042

1043+
if (getIsHydrating() && hasId) {
1044+
pushMaterializedTreeId(workInProgress);
1045+
}
1046+
10261047
// React DevTools reads this flag.
10271048
workInProgress.flags |= PerformedWork;
10281049
reconcileChildren(current, workInProgress, nextChildren, renderLanes);
@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(
15931614

15941615
prepareToReadContext(workInProgress, renderLanes);
15951616
let value;
1617+
let hasId;
15961618

15971619
if (enableSchedulingProfiler) {
15981620
markComponentRenderStarted(workInProgress);
@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
16291651
context,
16301652
renderLanes,
16311653
);
1654+
hasId = checkDidRenderIdHook();
16321655
setIsRendering(false);
16331656
} else {
16341657
value = renderWithHooks(
@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
16391662
context,
16401663
renderLanes,
16411664
);
1665+
hasId = checkDidRenderIdHook();
16421666
}
16431667
if (enableSchedulingProfiler) {
16441668
markComponentRenderStopped();
@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(
17581782
context,
17591783
renderLanes,
17601784
);
1785+
hasId = checkDidRenderIdHook();
17611786
} finally {
17621787
setIsStrictModeForDevtools(false);
17631788
}
17641789
}
17651790
}
17661791

1792+
if (getIsHydrating() && hasId) {
1793+
pushMaterializedTreeId(workInProgress);
1794+
}
1795+
17671796
reconcileChildren(null, workInProgress, value, renderLanes);
17681797
if (__DEV__) {
17691798
validateFunctionComponentInDev(workInProgress, Component);

0 commit comments

Comments
 (0)