@samitouri / QOS-React-2 / commits / 00ced1e2b7

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

Andrew Clark committed Nov 2, 2021 at 20:59 UTC 00ced1e2b7610543a519329a76ad0bfd12cd1c32

8 files changed +172 -36

packages/react-dom/src/__tests__/ReactDOMUseId-test.js

+57

@@ -16,6 +16,7 @@ let ReactDOMFizzServer;

16 let Stream;

17 let Suspense;

18 let useId;

19 +let useState;

20 let document;

21 let writable;

22 let container;

@@ -35,6 +36,7 @@ describe('useId', () => {

36 Stream = require('stream');

37 Suspense = React.Suspense;

38 useId = React.useId;

39 + useState = React.useState;

41 // Test Environment

42 const jsdom = new JSDOM(

@@ -198,6 +200,35 @@ describe('useId', () => {

200 `);

201 });

202

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 +

232 test('empty (null) children', async () => {

233 // We don't treat empty children different from non-empty ones, which means

234 // they get allocated a slot when generating ids. There's no inherent reason

@@ -313,6 +344,32 @@ describe('useId', () => {

344 `);

345 });

346

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 +

373 test('basic incremental hydration', async () => {

374 function App() {

375 return (

packages/react-reconciler/src/ReactFiberBeginWork.new.js

+30 -1

@@ -174,7 +174,11 @@ import {

174 prepareToReadContext,

175 scheduleWorkOnParentPath,

176 } from './ReactFiberNewContext.new';

177 -import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.new';

177 +import {

178 + renderWithHooks,

179 + checkDidRenderIdHook,

180 + bailoutHooks,

181 +} from './ReactFiberHooks.new';

182 import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.new';

183 import {

184 getMaskedContext,

@@ -240,6 +244,7 @@ import {

244 getForksAtLevel,

245 isForkedChild,

246 pushTreeId,

247 + pushMaterializedTreeId,

248 } from './ReactFiberTreeContext.new';

249

250 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

@@ -365,6 +370,7 @@ function updateForwardRef(

370

371 // The rest is a fork of updateFunctionComponent

372 let nextChildren;

373 + let hasId;

374 prepareToReadContext(workInProgress, renderLanes);

375 if (enableSchedulingProfiler) {

376 markComponentRenderStarted(workInProgress);

@@ -380,6 +386,7 @@ function updateForwardRef(

386 ref,

387 renderLanes,

388 );

389 + hasId = checkDidRenderIdHook();

390 if (

391 debugRenderPhaseSideEffectsForStrictMode &&

392 workInProgress.mode & StrictLegacyMode

@@ -394,6 +401,7 @@ function updateForwardRef(

401 ref,

402 renderLanes,

403 );

404 + hasId = checkDidRenderIdHook();

405 } finally {

406 setIsStrictModeForDevtools(false);

407 }

@@ -408,6 +416,7 @@ function updateForwardRef(

416 ref,

417 renderLanes,

418 );

419 + hasId = checkDidRenderIdHook();

420 }

421 if (enableSchedulingProfiler) {

422 markComponentRenderStopped();

@@ -418,6 +427,10 @@ function updateForwardRef(

427 return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);

428 }

429

430 + if (getIsHydrating() && hasId) {

431 + pushMaterializedTreeId(workInProgress);

432 + }

433 +

434 // React DevTools reads this flag.

435 workInProgress.flags |= PerformedWork;

436 reconcileChildren(current, workInProgress, nextChildren, renderLanes);

@@ -970,6 +983,7 @@ function updateFunctionComponent(

983 }

984

985 let nextChildren;

986 + let hasId;

987 prepareToReadContext(workInProgress, renderLanes);

988 if (enableSchedulingProfiler) {

989 markComponentRenderStarted(workInProgress);

@@ -985,6 +999,7 @@ function updateFunctionComponent(

999 context,

1000 renderLanes,

1001 );

1002 + hasId = checkDidRenderIdHook();

1003 if (

1004 debugRenderPhaseSideEffectsForStrictMode &&

1005 workInProgress.mode & StrictLegacyMode

@@ -999,6 +1014,7 @@ function updateFunctionComponent(

1014 context,

1015 renderLanes,

1016 );

1017 + hasId = checkDidRenderIdHook();

1018 } finally {

1019 setIsStrictModeForDevtools(false);

1020 }

@@ -1013,6 +1029,7 @@ function updateFunctionComponent(

1029 context,

1030 renderLanes,

1031 );

1032 + hasId = checkDidRenderIdHook();

1033 }

1034 if (enableSchedulingProfiler) {

1035 markComponentRenderStopped();

@@ -1023,6 +1040,10 @@ function updateFunctionComponent(

1040 return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);

1041 }

1042

1043 + if (getIsHydrating() && hasId) {

1044 + pushMaterializedTreeId(workInProgress);

1045 + }

1046 +

1047 // React DevTools reads this flag.

1048 workInProgress.flags |= PerformedWork;

1049 reconcileChildren(current, workInProgress, nextChildren, renderLanes);

@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(

1614

1615 prepareToReadContext(workInProgress, renderLanes);

1616 let value;

1617 + let hasId;

1618

1619 if (enableSchedulingProfiler) {

1620 markComponentRenderStarted(workInProgress);

@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(

1651 context,

1652 renderLanes,

1653 );

1654 + hasId = checkDidRenderIdHook();

1655 setIsRendering(false);

1656 } else {

1657 value = renderWithHooks(

@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(

1662 context,

1663 renderLanes,

1664 );

1665 + hasId = checkDidRenderIdHook();

1666 }

1667 if (enableSchedulingProfiler) {

1668 markComponentRenderStopped();

@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(

1782 context,

1783 renderLanes,

1784 );

1785 + hasId = checkDidRenderIdHook();

1786 } finally {

1787 setIsStrictModeForDevtools(false);

1788 }

1789 }

1790 }

1791

1792 + if (getIsHydrating() && hasId) {

1793 + pushMaterializedTreeId(workInProgress);

1794 + }

1795 +

1796 reconcileChildren(null, workInProgress, value, renderLanes);

1797 if (__DEV__) {

1798 validateFunctionComponentInDev(workInProgress, Component);

packages/react-reconciler/src/ReactFiberBeginWork.old.js

+30 -1

@@ -174,7 +174,11 @@ import {

174 prepareToReadContext,

175 scheduleWorkOnParentPath,

176 } from './ReactFiberNewContext.old';

177 -import {renderWithHooks, bailoutHooks} from './ReactFiberHooks.old';

177 +import {

178 + renderWithHooks,

179 + checkDidRenderIdHook,

180 + bailoutHooks,

181 +} from './ReactFiberHooks.old';

182 import {stopProfilerTimerIfRunning} from './ReactProfilerTimer.old';

183 import {

184 getMaskedContext,

@@ -240,6 +244,7 @@ import {

244 getForksAtLevel,

245 isForkedChild,

246 pushTreeId,

247 + pushMaterializedTreeId,

248 } from './ReactFiberTreeContext.old';

249

250 const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;

@@ -365,6 +370,7 @@ function updateForwardRef(

370

371 // The rest is a fork of updateFunctionComponent

372 let nextChildren;

373 + let hasId;

374 prepareToReadContext(workInProgress, renderLanes);

375 if (enableSchedulingProfiler) {

376 markComponentRenderStarted(workInProgress);

@@ -380,6 +386,7 @@ function updateForwardRef(

386 ref,

387 renderLanes,

388 );

389 + hasId = checkDidRenderIdHook();

390 if (

391 debugRenderPhaseSideEffectsForStrictMode &&

392 workInProgress.mode & StrictLegacyMode

@@ -394,6 +401,7 @@ function updateForwardRef(

401 ref,

402 renderLanes,

403 );

404 + hasId = checkDidRenderIdHook();

405 } finally {

406 setIsStrictModeForDevtools(false);

407 }

@@ -408,6 +416,7 @@ function updateForwardRef(

416 ref,

417 renderLanes,

418 );

419 + hasId = checkDidRenderIdHook();

420 }

421 if (enableSchedulingProfiler) {

422 markComponentRenderStopped();

@@ -418,6 +427,10 @@ function updateForwardRef(

427 return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);

428 }

429

430 + if (getIsHydrating() && hasId) {

431 + pushMaterializedTreeId(workInProgress);

432 + }

433 +

434 // React DevTools reads this flag.

435 workInProgress.flags |= PerformedWork;

436 reconcileChildren(current, workInProgress, nextChildren, renderLanes);

@@ -970,6 +983,7 @@ function updateFunctionComponent(

983 }

984

985 let nextChildren;

986 + let hasId;

987 prepareToReadContext(workInProgress, renderLanes);

988 if (enableSchedulingProfiler) {

989 markComponentRenderStarted(workInProgress);

@@ -985,6 +999,7 @@ function updateFunctionComponent(

999 context,

1000 renderLanes,

1001 );

1002 + hasId = checkDidRenderIdHook();

1003 if (

1004 debugRenderPhaseSideEffectsForStrictMode &&

1005 workInProgress.mode & StrictLegacyMode

@@ -999,6 +1014,7 @@ function updateFunctionComponent(

1014 context,

1015 renderLanes,

1016 );

1017 + hasId = checkDidRenderIdHook();

1018 } finally {

1019 setIsStrictModeForDevtools(false);

1020 }

@@ -1013,6 +1029,7 @@ function updateFunctionComponent(

1029 context,

1030 renderLanes,

1031 );

1032 + hasId = checkDidRenderIdHook();

1033 }

1034 if (enableSchedulingProfiler) {

1035 markComponentRenderStopped();

@@ -1023,6 +1040,10 @@ function updateFunctionComponent(

1040 return bailoutOnAlreadyFinishedWork(current, workInProgress, renderLanes);

1041 }

1042

1043 + if (getIsHydrating() && hasId) {

1044 + pushMaterializedTreeId(workInProgress);

1045 + }

1046 +

1047 // React DevTools reads this flag.

1048 workInProgress.flags |= PerformedWork;

1049 reconcileChildren(current, workInProgress, nextChildren, renderLanes);

@@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(

1614

1615 prepareToReadContext(workInProgress, renderLanes);

1616 let value;

1617 + let hasId;

1618

1619 if (enableSchedulingProfiler) {

1620 markComponentRenderStarted(workInProgress);

@@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(

1651 context,

1652 renderLanes,

1653 );

1654 + hasId = checkDidRenderIdHook();

1655 setIsRendering(false);

1656 } else {

1657 value = renderWithHooks(

@@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(

1662 context,

1663 renderLanes,

1664 );

1665 + hasId = checkDidRenderIdHook();

1666 }

1667 if (enableSchedulingProfiler) {

1668 markComponentRenderStopped();

@@ -1758,12 +1782,17 @@ function mountIndeterminateComponent(

1782 context,

1783 renderLanes,

1784 );

1785 + hasId = checkDidRenderIdHook();

1786 } finally {

1787 setIsStrictModeForDevtools(false);

1788 }

1789 }

1790 }

1791

1792 + if (getIsHydrating() && hasId) {

1793 + pushMaterializedTreeId(workInProgress);

1794 + }

1795 +

1796 reconcileChildren(null, workInProgress, value, renderLanes);

1797 if (__DEV__) {

1798 validateFunctionComponentInDev(workInProgress, Component);

packages/react-reconciler/src/ReactFiberHooks.new.js

+13 -17

@@ -110,7 +110,7 @@ import {

110 } from './ReactUpdateQueue.new';

111 import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new';

112 import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';

113 -import {getTreeId, pushTreeFork, pushTreeId} from './ReactFiberTreeContext.new';

113 +import {getTreeId} from './ReactFiberTreeContext.new';

114

115 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;

116

@@ -432,6 +432,7 @@ export function renderWithHooks<Props, SecondArg>(

432 let numberOfReRenders: number = 0;

433 do {

434 didScheduleRenderPhaseUpdateDuringThisPass = false;

435 + localIdCounter = 0;

436

437 if (numberOfReRenders >= RE_RENDER_LIMIT) {

438 throw new Error(

@@ -513,6 +514,8 @@ export function renderWithHooks<Props, SecondArg>(

514 }

515

516 didScheduleRenderPhaseUpdate = false;

517 + // This is reset by checkDidRenderIdHook

518 + // localIdCounter = 0;

519

520 if (didRenderTooFewHooks) {

521 throw new Error(

@@ -541,25 +544,18 @@ export function renderWithHooks<Props, SecondArg>(

544 }

545 }

546 }

544 -

545 - if (localIdCounter !== 0) {

546 - localIdCounter = 0;

547 - if (getIsHydrating()) {

548 - // This component materialized an id. This will affect any ids that appear

549 - // in its children.

550 - const returnFiber = workInProgress.return;

551 - if (returnFiber !== null) {

552 - const numberOfForks = 1;

553 - const slotIndex = 0;

554 - pushTreeFork(workInProgress, numberOfForks);

555 - pushTreeId(workInProgress, numberOfForks, slotIndex);

556 - }

557 - }

558 - }

559 -

547 return children;

548 }

549

550 +export function checkDidRenderIdHook() {

551 + // This should be called immediately after every renderWithHooks call.

552 + // Conceptually, it's part of the return value of renderWithHooks; it's only a

553 + // separate function to avoid using an array tuple.

554 + const didRenderIdHook = localIdCounter !== 0;

555 + localIdCounter = 0;

556 + return didRenderIdHook;

557 +}

558 +

559 export function bailoutHooks(

560 current: Fiber,

561 workInProgress: Fiber,

packages/react-reconciler/src/ReactFiberHooks.old.js

+13 -17

@@ -110,7 +110,7 @@ import {

110 } from './ReactUpdateQueue.old';

111 import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.old';

112 import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';

113 -import {getTreeId, pushTreeFork, pushTreeId} from './ReactFiberTreeContext.old';

113 +import {getTreeId} from './ReactFiberTreeContext.old';

114

115 const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;

116

@@ -432,6 +432,7 @@ export function renderWithHooks<Props, SecondArg>(

432 let numberOfReRenders: number = 0;

433 do {

434 didScheduleRenderPhaseUpdateDuringThisPass = false;

435 + localIdCounter = 0;

436

437 if (numberOfReRenders >= RE_RENDER_LIMIT) {

438 throw new Error(

@@ -513,6 +514,8 @@ export function renderWithHooks<Props, SecondArg>(

514 }

515

516 didScheduleRenderPhaseUpdate = false;

517 + // This is reset by checkDidRenderIdHook

518 + // localIdCounter = 0;

519

520 if (didRenderTooFewHooks) {

521 throw new Error(

@@ -541,25 +544,18 @@ export function renderWithHooks<Props, SecondArg>(

544 }

545 }

546 }

544 -

545 - if (localIdCounter !== 0) {

546 - localIdCounter = 0;

547 - if (getIsHydrating()) {

548 - // This component materialized an id. This will affect any ids that appear

549 - // in its children.

550 - const returnFiber = workInProgress.return;

551 - if (returnFiber !== null) {

552 - const numberOfForks = 1;

553 - const slotIndex = 0;

554 - pushTreeFork(workInProgress, numberOfForks);

555 - pushTreeId(workInProgress, numberOfForks, slotIndex);

556 - }

557 - }

558 - }

559 -

547 return children;

548 }

549

550 +export function checkDidRenderIdHook() {

551 + // This should be called immediately after every renderWithHooks call.

552 + // Conceptually, it's part of the return value of renderWithHooks; it's only a

553 + // separate function to avoid using an array tuple.

554 + const didRenderIdHook = localIdCounter !== 0;

555 + localIdCounter = 0;

556 + return didRenderIdHook;

557 +}

558 +

559 export function bailoutHooks(

560 current: Fiber,

561 workInProgress: Fiber,

packages/react-reconciler/src/ReactFiberTreeContext.new.js

+14

@@ -201,6 +201,20 @@ export function pushTreeId(

201 }

202 }

203

204 +export function pushMaterializedTreeId(workInProgress: Fiber) {

205 + warnIfNotHydrating();

206 +

207 + // This component materialized an id. This will affect any ids that appear

208 + // in its children.

209 + const returnFiber = workInProgress.return;

210 + if (returnFiber !== null) {

211 + const numberOfForks = 1;

212 + const slotIndex = 0;

213 + pushTreeFork(workInProgress, numberOfForks);

214 + pushTreeId(workInProgress, numberOfForks, slotIndex);

215 + }

216 +}

217 +

218 function getBitLength(number: number): number {

219 return 32 - clz32(number);

220 }

packages/react-reconciler/src/ReactFiberTreeContext.old.js

+14