Skip to content

Commit

Permalink
Bugfix: Selective hydration triggers false update loop error (faceboo…
Browse files Browse the repository at this point in the history
…k#27439)

This adds a regression test and fix for a case where a sync update
triggers selective hydration, which then leads to a "Maximum update
depth exceeded" error, even though there was only a single update. This
happens when a single sync update flows into many sibling dehydrated
Suspense boundaries.

This fix is, if a commit was the result of selective hydration, we
should not increment the nested update count, because those renders
conceptually are not updates.

Ideally, they wouldn't even be in a separate commit — we should be able
to hydrate a tree and apply an update on top of it within the same
render phase. We could do this once we implement resumable context
stacks.
  • Loading branch information
acdlite authored and AndyPengc12 committed Apr 15, 2024
1 parent 0cdeb29 commit e6b96fd
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1853,4 +1853,58 @@ describe('ReactDOMServerSelectiveHydration', () => {
assertLog(['App', 'A', 'App', 'AA', 'DefaultContext', 'Commit']);
});
});

it('regression: selective hydration does not contribute to "maximum update limit" count', async () => {
const outsideRef = React.createRef(null);
const insideRef = React.createRef(null);
function Child() {
return (
<Suspense fallback="Loading...">
<div ref={insideRef} />
</Suspense>
);
}

let setIsMounted = false;
function App() {
const [isMounted, setState] = React.useState(false);
setIsMounted = setState;

const children = [];
for (let i = 0; i < 100; i++) {
children.push(<Child key={i} isMounted={isMounted} />);
}

return <div ref={outsideRef}>{children}</div>;
}

const finalHTML = ReactDOMServer.renderToString(<App />);
const container = document.createElement('div');
container.innerHTML = finalHTML;

await act(async () => {
ReactDOMClient.hydrateRoot(container, <App />);

// Commit just the shell
await waitForPaint([]);

// Assert that the shell has hydrated, but not the children
expect(outsideRef.current).not.toBe(null);
expect(insideRef.current).toBe(null);

// Update the shell synchronously. The update will flow into the children,
// which haven't hydrated yet. This will trigger a cascade of commits
// caused by selective hydration. However, since there's really only one
// update, it should not be treated as an update loop.
// NOTE: It's unfortunate that every sibling boundary is separately
// committed in this case. We should be able to commit everything in a
// render phase, which we could do if we had resumable context stacks.
ReactDOM.flushSync(() => {
setIsMounted(true);
});
});

// Should have successfully hydrated with no errors.
expect(insideRef.current).not.toBe(null);
});
});
9 changes: 8 additions & 1 deletion packages/react-reconciler/src/ReactFiberLane.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ export const InputContinuousLane: Lane = /* */ 0b0000000000000000000
export const DefaultHydrationLane: Lane = /* */ 0b0000000000000000000000000010000;
export const DefaultLane: Lane = /* */ 0b0000000000000000000000000100000;

export const SyncUpdateLanes: Lane = /* */ 0b0000000000000000000000000101010;
export const SyncUpdateLanes: Lane = enableUnifiedSyncLane
? SyncLane | InputContinuousLane | DefaultLane
: SyncLane;

const TransitionHydrationLane: Lane = /* */ 0b0000000000000000000000001000000;
const TransitionLanes: Lanes = /* */ 0b0000000011111111111111110000000;
Expand Down Expand Up @@ -84,6 +86,11 @@ export const IdleLane: Lane = /* */ 0b0100000000000000000

export const OffscreenLane: Lane = /* */ 0b1000000000000000000000000000000;

// Any lane that might schedule an update. This is used to detect infinite
// update loops, so it doesn't include hydration lanes or retries.
export const UpdateLanes: Lanes =
SyncLane | InputContinuousLane | DefaultLane | TransitionLanes;

// This function is used for the experimental timeline (react-devtools-timeline)
// It should be kept in sync with the Lanes values above.
export function getLabelForLane(lane: Lane): string | void {
Expand Down
14 changes: 13 additions & 1 deletion packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,8 @@ import {
includesOnlyNonUrgentLanes,
includesSomeLane,
OffscreenLane,
SyncUpdateLanes,
UpdateLanes,
} from './ReactFiberLane';
import {
DiscreteEventPriority,
Expand Down Expand Up @@ -2932,7 +2934,17 @@ function commitRootImpl(

// Read this again, since a passive effect might have updated it
remainingLanes = root.pendingLanes;
if (includesSyncLane(remainingLanes)) {

// Check if this render scheduled a cascading synchronous update. This is a
// heurstic to detect infinite update loops. We are intentionally excluding
// hydration lanes in this check, because render triggered by selective
// hydration is conceptually not an update.
if (
// Was the finished render the result of an update (not hydration)?
includesSomeLane(lanes, UpdateLanes) &&
// Did it schedule a sync update?
includesSomeLane(remainingLanes, SyncUpdateLanes)
) {
if (enableProfilerTimer && enableProfilerNestedUpdatePhase) {
markNestedUpdateScheduled();
}
Expand Down

0 comments on commit e6b96fd

Please sign in to comment.