Skip to content

Commit

Permalink
Strict Mode: Reuse memoized result from first pass (#25583)
Browse files Browse the repository at this point in the history
In Strict Mode, during development, user functions are double invoked to
help detect side effects. Currently, the way we implement this is to
completely discard the first pass and start over. Theoretically this
should be fine because components are idempotent. However, it's a bit
tricky to get right because our implementation (i.e. `renderWithHooks`)
is not completely idempotent with respect to internal data structures,
like the work-in-progress fiber. In the past we've had to be really
careful to avoid subtle bugs — for example, during the initial mount,
`setState` functions are bound to the particular hook instances that
were created during that render. If we compute new hook instances, we
must also compute new children, and they must correspond to each other.

This commit addresses a similar issue that came up related to `use`:
when something suspends, `use` reuses the promise that was passed during
the first attempt. This is itself a form of memoization. We need to be
able to memoize the reactive inputs to the `use` call using a hook (i.e.
`useMemo`), which means, the reactive inputs to `use` must come from the
same component invocation as the output.

The solution I've chosen is, rather than double invoke the entire
`renderWithHook` function, we should double invoke each individual user
function. It's a bit confusing but here's how it works:

We will invoke the entire component function twice. However, during the
second invocation of the component, the hook state from the first
invocation will be reused. That means things like `useMemo` functions
won't run again, because the deps will match and the memoized result
will be reused.

We want memoized functions to run twice, too, so account for this, user
functions are double invoked during the *first* invocation of the
component function, and are *not* double invoked during the second
incovation:

- First execution of component function: user functions are double
invoked
- Second execution of component function (in Strict Mode, during
development): user functions are not double invoked.

It's hard to explain verbally but much clearer when you run the test
cases I've added.
  • Loading branch information
acdlite authored Oct 28, 2022
1 parent d2a0176 commit 5450dd4
Show file tree
Hide file tree
Showing 5 changed files with 455 additions and 198 deletions.
58 changes: 0 additions & 58 deletions packages/react-reconciler/src/ReactFiberBeginWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,25 +418,6 @@ function updateForwardRef(
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
setIsRendering(false);
} else {
nextChildren = renderWithHooks(
Expand Down Expand Up @@ -1125,25 +1106,6 @@ function updateFunctionComponent(
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
setIsRendering(false);
} else {
nextChildren = renderWithHooks(
Expand Down Expand Up @@ -1969,26 +1931,6 @@ function mountIndeterminateComponent(
getComponentNameFromType(Component) || 'Unknown',
);
}

if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
value = renderWithHooks(
null,
workInProgress,
Component,
props,
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
}

if (getIsHydrating() && hasId) {
Expand Down
58 changes: 0 additions & 58 deletions packages/react-reconciler/src/ReactFiberBeginWork.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -418,25 +418,6 @@ function updateForwardRef(
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
nextChildren = renderWithHooks(
current,
workInProgress,
render,
nextProps,
ref,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
setIsRendering(false);
} else {
nextChildren = renderWithHooks(
Expand Down Expand Up @@ -1125,25 +1106,6 @@ function updateFunctionComponent(
renderLanes,
);
hasId = checkDidRenderIdHook();
if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
nextChildren = renderWithHooks(
current,
workInProgress,
Component,
nextProps,
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
setIsRendering(false);
} else {
nextChildren = renderWithHooks(
Expand Down Expand Up @@ -1969,26 +1931,6 @@ function mountIndeterminateComponent(
getComponentNameFromType(Component) || 'Unknown',
);
}

if (
debugRenderPhaseSideEffectsForStrictMode &&
workInProgress.mode & StrictLegacyMode
) {
setIsStrictModeForDevtools(true);
try {
value = renderWithHooks(
null,
workInProgress,
Component,
props,
context,
renderLanes,
);
hasId = checkDidRenderIdHook();
} finally {
setIsStrictModeForDevtools(false);
}
}
}

if (getIsHydrating() && hasId) {
Expand Down
Loading

0 comments on commit 5450dd4

Please sign in to comment.