Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear extra nodes if there's a hydration mismatch within a suspense boundary #22592

Merged
merged 2 commits into from
Oct 19, 2021
Merged

Clear extra nodes if there's a hydration mismatch within a suspense boundary #22592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5062b1
Clear extra nodes if there's a mismatch within a suspense boundary
sebmarkbage Oct 19, 2021
1229123
Tighten up the types of getFirstHydratableChild
sebmarkbage Oct 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,64 @@ describe('ReactDOMServerPartialHydration', () => {
expect(deleted.length).toBe(1);
});

it('hydrates an empty suspense boundary', async () => {
function App() {
return (
<div>
<Suspense fallback="Loading..." />
<div>Sibling</div>
</div>
);
}

const finalHTML = ReactDOMServer.renderToString(<App />);

const container = document.createElement('div');
container.innerHTML = finalHTML;

ReactDOM.hydrateRoot(container, <App />);
Scheduler.unstable_flushAll();
jest.runAllTimers();

expect(container.innerHTML).toContain('<div>Sibling</div>');
});

it('recovers when server rendered additional nodes', async () => {
const ref = React.createRef();
function App({hasB}) {
return (
<div>
<Suspense fallback="Loading...">
<span ref={ref}>A</span>
{hasB ? <span>B</span> : null}
</Suspense>
<div>Sibling</div>
</div>
);
}

const finalHTML = ReactDOMServer.renderToString(<App hasB={true} />);

const container = document.createElement('div');
container.innerHTML = finalHTML;

const span = container.getElementsByTagName('span')[0];

expect(container.innerHTML).toContain('<span>A</span>');
expect(container.innerHTML).toContain('<span>B</span>');
expect(ref.current).toBe(null);

ReactDOM.hydrateRoot(container, <App hasB={false} />);
expect(() => {
Scheduler.unstable_flushAll();
}).toErrorDev('Did not expect server HTML to contain a <span> in <div>');
jest.runAllTimers();

expect(container.innerHTML).toContain('<span>A</span>');
expect(container.innerHTML).not.toContain('<span>B</span>');
expect(ref.current).toBe(span);
});

it('calls the onDeleted hydration callback if the parent gets deleted', async () => {
let suspend = false;
const promise = new Promise(() => {});
Expand Down
3 changes: 3 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,9 @@ function getNextHydratable(node) {
) {
break;
}
if (nodeData === SUSPENSE_END_DATA) {
return null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how I noticed this issue in the first place.

getFirstHydratableChildWithinSuspenseInstance calls nextSibling to get the first child but if there are no children in the Suspense boundary it'll pop out of the boundary. Because in <!--$--><!--/$--> nextSibling will give you the SUSPENSE_END_DATA comment. Which is then ignored by getNextHydratable. We can assume that in all calls of getNextHydratable, reaching one of these is like reaching the end of siblings.

}
}
}
}
Expand Down
11 changes: 5 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,16 +1007,16 @@ function completeWork(

if (enableSuspenseServerRenderer) {
if (nextState !== null && nextState.dehydrated !== null) {
// We might be inside a hydration state the first time we're picking up this
// Suspense boundary, and also after we've reentered it for further hydration.
const wasHydrated = popHydrationState(workInProgress);
if (current === null) {
const wasHydrated = popHydrationState(workInProgress);

if (!wasHydrated) {
throw new Error(
'A dehydrated suspense component was completed without a hydrated node. ' +
'This is probably a bug in React.',
);
}

prepareToHydrateHostSuspenseInstance(workInProgress);
bubbleProperties(workInProgress);
if (enableProfilerTimer) {
Expand All @@ -1034,9 +1034,8 @@ function completeWork(
}
return null;
} else {
// We should never have been in a hydration state if we didn't have a current.
// However, in some of those paths, we might have reentered a hydration state
// and then we might be inside a hydration state. In that case, we'll need to exit out of it.
// We might have reentered this boundary to hydrate it. If so, we need to reset the hydration
// state since we're now exiting out of it. popHydrationState doesn't do that for us.
resetHydrationState();
if ((workInProgress.flags & DidCapture) === NoFlags) {
// This boundary did not suspend so it's now hydrated and unsuspended.
Expand Down
11 changes: 5 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,16 +1007,16 @@ function completeWork(

if (enableSuspenseServerRenderer) {
if (nextState !== null && nextState.dehydrated !== null) {
// We might be inside a hydration state the first time we're picking up this
// Suspense boundary, and also after we've reentered it for further hydration.
const wasHydrated = popHydrationState(workInProgress);
if (current === null) {
const wasHydrated = popHydrationState(workInProgress);

if (!wasHydrated) {
throw new Error(
'A dehydrated suspense component was completed without a hydrated node. ' +
'This is probably a bug in React.',
);
}

prepareToHydrateHostSuspenseInstance(workInProgress);
bubbleProperties(workInProgress);
if (enableProfilerTimer) {
Expand All @@ -1034,9 +1034,8 @@ function completeWork(
}
return null;
} else {
// We should never have been in a hydration state if we didn't have a current.
// However, in some of those paths, we might have reentered a hydration state
// and then we might be inside a hydration state. In that case, we'll need to exit out of it.
// We might have reentered this boundary to hydrate it. If so, we need to reset the hydration
// state since we're now exiting out of it. popHydrationState doesn't do that for us.
resetHydrationState();
if ((workInProgress.flags & DidCapture) === NoFlags) {
// This boundary did not suspend so it's now hydrated and unsuspended.
Expand Down
17 changes: 11 additions & 6 deletions packages/react-reconciler/src/ReactFiberHydrationContext.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ function tryHydrate(fiber, nextInstance) {
const instance = canHydrateInstance(nextInstance, type, props);
if (instance !== null) {
fiber.stateNode = (instance: Instance);
hydrationParentFiber = fiber;
nextHydratableInstance = getFirstHydratableChild(instance);
return true;
}
return false;
Expand All @@ -270,6 +272,9 @@ function tryHydrate(fiber, nextInstance) {
const textInstance = canHydrateTextInstance(nextInstance, text);
if (textInstance !== null) {
fiber.stateNode = (textInstance: TextInstance);
hydrationParentFiber = fiber;
// Text Instances don't have children so there's nothing to hydrate.
nextHydratableInstance = null;
return true;
}
return false;
Expand All @@ -294,6 +299,10 @@ function tryHydrate(fiber, nextInstance) {
);
dehydratedFragment.return = fiber;
fiber.child = dehydratedFragment;
hydrationParentFiber = fiber;
Copy link
Collaborator Author

@sebmarkbage sebmarkbage Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could avoid updating this and instead leave it. I.e. not step into it and then not call pop in the complete phase instead. That might be better than "entering" it twice in two different ways.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also instead of trying to hydrate automatically entering these, we could make an explicit call in each begin phase ot enter.

// While a Suspense Instance does have children, we won't step into
// it during the first pass. Instead, we'll reenter it later.
nextHydratableInstance = null;
return true;
}
}
Expand Down Expand Up @@ -322,6 +331,7 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void {
// We use this as a heuristic. It's based on intuition and not data so it
// might be flawed or unnecessary.
nextInstance = getNextHydratableSibling(firstAttemptedInstance);
const prevHydrationParentFiber: Fiber = (hydrationParentFiber: any);
if (!nextInstance || !tryHydrate(fiber, nextInstance)) {
// Nothing to hydrate. Make it an insertion.
insertNonHydratedInstance((hydrationParentFiber: any), fiber);
Expand All @@ -333,13 +343,8 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void {
// superfluous and we'll delete it. Since we can't eagerly delete it
// we'll have to schedule a deletion. To do that, this node needs a dummy
// fiber associated with it.
deleteHydratableInstance(
(hydrationParentFiber: any),
firstAttemptedInstance,
);
deleteHydratableInstance(prevHydrationParentFiber, firstAttemptedInstance);
}
hydrationParentFiber = fiber;
nextHydratableInstance = getFirstHydratableChild((nextInstance: any));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens to return null on a comment so when we step into a suspense boundary, we find nothing. Which is actually good, because otherwise we'd delete its content with this change.

}

function prepareToHydrateHostInstance(
Expand Down
17 changes: 11 additions & 6 deletions packages/react-reconciler/src/ReactFiberHydrationContext.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ function tryHydrate(fiber, nextInstance) {
const instance = canHydrateInstance(nextInstance, type, props);
if (instance !== null) {
fiber.stateNode = (instance: Instance);
hydrationParentFiber = fiber;
nextHydratableInstance = getFirstHydratableChild(instance);
return true;
}
return false;
Expand All @@ -270,6 +272,9 @@ function tryHydrate(fiber, nextInstance) {
const textInstance = canHydrateTextInstance(nextInstance, text);
if (textInstance !== null) {
fiber.stateNode = (textInstance: TextInstance);
hydrationParentFiber = fiber;
// Text Instances don't have children so there's nothing to hydrate.
nextHydratableInstance = null;
return true;
}
return false;
Expand All @@ -294,6 +299,10 @@ function tryHydrate(fiber, nextInstance) {
);
dehydratedFragment.return = fiber;
fiber.child = dehydratedFragment;
hydrationParentFiber = fiber;
// While a Suspense Instance does have children, we won't step into
// it during the first pass. Instead, we'll reenter it later.
nextHydratableInstance = null;
return true;
}
}
Expand Down Expand Up @@ -322,6 +331,7 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void {
// We use this as a heuristic. It's based on intuition and not data so it
// might be flawed or unnecessary.
nextInstance = getNextHydratableSibling(firstAttemptedInstance);
const prevHydrationParentFiber: Fiber = (hydrationParentFiber: any);
if (!nextInstance || !tryHydrate(fiber, nextInstance)) {
// Nothing to hydrate. Make it an insertion.
insertNonHydratedInstance((hydrationParentFiber: any), fiber);
Expand All @@ -333,13 +343,8 @@ function tryToClaimNextHydratableInstance(fiber: Fiber): void {
// superfluous and we'll delete it. Since we can't eagerly delete it
// we'll have to schedule a deletion. To do that, this node needs a dummy
// fiber associated with it.
deleteHydratableInstance(
(hydrationParentFiber: any),
firstAttemptedInstance,
);
deleteHydratableInstance(prevHydrationParentFiber, firstAttemptedInstance);
}
hydrationParentFiber = fiber;
nextHydratableInstance = getFirstHydratableChild((nextInstance: any));
}

function prepareToHydrateHostInstance(
Expand Down