Skip to content

Commit

Permalink
Fix: useId in strict mode
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@acdlite
acdlite committed Nov 2, 2021
1 parent 9fb3442 commit de5f63f
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 36 deletions.
29 changes: 29 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMUseId-test.js
View file
Expand Up @@ -198,6 +198,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
Expand Down
31 changes: 30 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.new.js
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -240,6 +244,7 @@ import {
getForksAtLevel,
isForkedChild,
pushTreeId,
pushMaterializedTreeId,
} from './ReactFiberTreeContext.new';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
Expand Down Expand Up @@ -365,6 +370,7 @@ function updateForwardRef(

// The rest is a fork of updateFunctionComponent
let nextChildren;
let hasId;
prepareToReadContext(workInProgress, renderLanes);
if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand All @@ -380,6 +386,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
Expand All @@ -394,6 +401,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
Expand All @@ -408,6 +416,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand All @@ -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);
Expand Down Expand Up @@ -970,6 +983,7 @@ function updateFunctionComponent(
}

let nextChildren;
let hasId;
prepareToReadContext(workInProgress, renderLanes);
if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand All @@ -985,6 +999,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
Expand All @@ -999,6 +1014,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
Expand All @@ -1013,6 +1029,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand All @@ -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);
Expand Down Expand Up @@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(

prepareToReadContext(workInProgress, renderLanes);
let value;
let hasId;

if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand Down Expand Up @@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
setIsRendering(false);
} else {
value = renderWithHooks(
Expand All @@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand Down Expand Up @@ -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);
Expand Down
31 changes: 30 additions & 1 deletion packages/react-reconciler/src/ReactFiberBeginWork.old.js
View file
Expand Up @@ -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,
Expand Down Expand Up @@ -240,6 +244,7 @@ import {
getForksAtLevel,
isForkedChild,
pushTreeId,
pushMaterializedTreeId,
} from './ReactFiberTreeContext.old';

const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
Expand Down Expand Up @@ -365,6 +370,7 @@ function updateForwardRef(

// The rest is a fork of updateFunctionComponent
let nextChildren;
let hasId;
prepareToReadContext(workInProgress, renderLanes);
if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand All @@ -380,6 +386,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
Expand All @@ -394,6 +401,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
Expand All @@ -408,6 +416,7 @@ function updateForwardRef(
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand All @@ -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);
Expand Down Expand Up @@ -970,6 +983,7 @@ function updateFunctionComponent(
}

let nextChildren;
let hasId;
prepareToReadContext(workInProgress, renderLanes);
if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand All @@ -985,6 +999,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
Expand All @@ -999,6 +1014,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
Expand All @@ -1013,6 +1029,7 @@ function updateFunctionComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand All @@ -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);
Expand Down Expand Up @@ -1593,6 +1614,7 @@ function mountIndeterminateComponent(

prepareToReadContext(workInProgress, renderLanes);
let value;
let hasId;

if (enableSchedulingProfiler) {
markComponentRenderStarted(workInProgress);
Expand Down Expand Up @@ -1629,6 +1651,7 @@ function mountIndeterminateComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
setIsRendering(false);
} else {
value = renderWithHooks(
Expand All @@ -1639,6 +1662,7 @@ function mountIndeterminateComponent(
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
}
if (enableSchedulingProfiler) {
markComponentRenderStopped();
Expand Down Expand Up @@ -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);
Expand Down
30 changes: 13 additions & 17 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Expand Up @@ -110,7 +110,7 @@ import {
} from './ReactUpdateQueue.new';
import {pushInterleavedQueue} from './ReactFiberInterleavedUpdates.new';
import {warnOnSubscriptionInsideStartTransition} from 'shared/ReactFeatureFlags';
import {getTreeId, pushTreeFork, pushTreeId} from './ReactFiberTreeContext.new';
import {getTreeId} from './ReactFiberTreeContext.new';

const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;

Expand Down Expand Up @@ -432,6 +432,7 @@ export function renderWithHooks<Props, SecondArg>(
let numberOfReRenders: number = 0;
do {
didScheduleRenderPhaseUpdateDuringThisPass = false;
localIdCounter = 0;

if (numberOfReRenders >= RE_RENDER_LIMIT) {
throw new Error(
Expand Down Expand Up @@ -513,6 +514,8 @@ export function renderWithHooks<Props, SecondArg>(
}

didScheduleRenderPhaseUpdate = false;
// This is reset by checkDidRenderIdHook
// localIdCounter = 0;

if (didRenderTooFewHooks) {
throw new Error(
Expand Down Expand Up @@ -541,25 +544,18 @@ export function renderWithHooks<Props, SecondArg>(
}
}
}

if (localIdCounter !== 0) {
localIdCounter = 0;
if (getIsHydrating()) {
// This component materialized an id. This will affect any ids that appear
// in its children.
const returnFiber = workInProgress.return;
if (returnFiber !== null) {
const numberOfForks = 1;
const slotIndex = 0;
pushTreeFork(workInProgress, numberOfForks);
pushTreeId(workInProgress, numberOfForks, slotIndex);
}
}
}

return children;
}

export function checkDidRenderIdHook() {
// This should be called immediately after every renderWithHooks call.
// Conceptually, it's part of the return value of renderWithHooks; it's only a
// separate function to avoid using an array tuple.
const didRenderIdHook = localIdCounter !== 0;
localIdCounter = 0;
return didRenderIdHook;
}

export function bailoutHooks(
current: Fiber,
workInProgress: Fiber,
Expand Down

0 comments on commit de5f63f

Please sign in to comment.