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

Jump to bottom

Don't delete trailing mismatches during hydration at the root #21021

Merged
merged 6 commits into from
Mar 17, 2021
Merged

Don't delete trailing mismatches during hydration at the root #21021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0451e99
Don't delete any trailing nodes in the container during hydration error
sebmarkbage Mar 16, 2021
64b4cf1
Warn when an error during hydration causes us to clear the container
sebmarkbage Mar 16, 2021
2ea0f18
Encode unfortunate case in test
sebmarkbage Mar 17, 2021
7234ff4
Wrap the root for tests that are applicable to nested cases
sebmarkbage Mar 17, 2021
c035eec
Now we can pipe Fizz into a container
sebmarkbage Mar 17, 2021
618232e
Grammatical fix
sebmarkbage Mar 17, 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
29 changes: 13 additions & 16 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ let Suspense;
let textCache;
let document;
let writable;
let container;
let buffer = '';
let hasErrored = false;
let fatalError = undefined;
Expand All @@ -38,10 +39,14 @@ describe('ReactDOMFizzServer', () => {
textCache = new Map();

// Test Environment
const jsdom = new JSDOM('<!DOCTYPE html><html><head></head><body>', {
runScripts: 'dangerously',
});
const jsdom = new JSDOM(
'<!DOCTYPE html><html><head></head><body><div id="container">',
{
runScripts: 'dangerously',
},
);
document = jsdom.window.document;
container = document.getElementById('container');

buffer = '';
hasErrored = false;
Expand Down Expand Up @@ -80,9 +85,9 @@ describe('ReactDOMFizzServer', () => {
const script = document.createElement('script');
script.textContent = node.textContent;
fakeBody.removeChild(node);
document.body.appendChild(script);
container.appendChild(script);
} else {
document.body.appendChild(node);
container.appendChild(node);
}
}
}
Expand Down Expand Up @@ -200,11 +205,11 @@ describe('ReactDOMFizzServer', () => {
writable,
);
});
expect(getVisibleChildren(document.body)).toEqual(<div>Loading...</div>);
expect(getVisibleChildren(container)).toEqual(<div>Loading...</div>);
await act(async () => {
resolveText('Hello World');
});
expect(getVisibleChildren(document.body)).toEqual(<div>Hello World</div>);
expect(getVisibleChildren(container)).toEqual(<div>Hello World</div>);
});

// @gate experimental
Expand All @@ -224,20 +229,12 @@ describe('ReactDOMFizzServer', () => {
}

await act(async () => {
ReactDOMFizzServer.pipeToNodeWritable(
// We currently have to wrap the server node in a container because
// otherwise the Fizz nodes get deleted during hydration.
<div id="container">
<App />
</div>,
writable,
);
ReactDOMFizzServer.pipeToNodeWritable(<App />, writable);
});

// We're still showing a fallback.

// Attempt to hydrate the content.
const container = document.body.firstChild;
const root = ReactDOM.unstable_createRoot(container, {hydrate: true});
root.render(<App />);
Scheduler.unstable_flushAll();
Expand Down
10 changes: 9 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerIntegrationFragment-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,15 @@ describe('ReactDOMServerIntegration', () => {
});

itRenders('an empty fragment', async render => {
expect(await render(<React.Fragment />)).toBe(null);
expect(
(
await render(
<div>
<React.Fragment />
</div>,
)
).firstChild,
).toBe(null);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
28 changes: 18 additions & 10 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1654,9 +1654,13 @@ describe('ReactDOMServerHooks', () => {
// This is the wrong HTML string
container.innerHTML = '<span></span>';
ReactDOM.unstable_createRoot(container, {hydrate: true}).render(<App />);
expect(() => Scheduler.unstable_flushAll()).toErrorDev([
'Warning: Expected server HTML to contain a matching <div> in <div>.',
]);
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
'Warning: Expected server HTML to contain a matching <div> in <div>.',
],
{withoutStack: 1},
);
});

// @gate experimental
Expand Down Expand Up @@ -1740,9 +1744,13 @@ describe('ReactDOMServerHooks', () => {
// This is the wrong HTML string
container.innerHTML = '<span></span>';
ReactDOM.unstable_createRoot(container, {hydrate: true}).render(<App />);
expect(() => Scheduler.unstable_flushAll()).toErrorDev([
'Warning: Expected server HTML to contain a matching <div> in <div>.',
]);
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
'Warning: Expected server HTML to contain a matching <div> in <div>.',
],
{withoutStack: 1},
);
});

// @gate experimental
Expand All @@ -1764,7 +1772,7 @@ describe('ReactDOMServerHooks', () => {
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
'Warning: Did not expect server HTML to contain a <span> in <div>.',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: 1},
);
Expand All @@ -1789,7 +1797,7 @@ describe('ReactDOMServerHooks', () => {
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
'Warning: Did not expect server HTML to contain a <span> in <div>.',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: 1},
);
Expand All @@ -1813,7 +1821,7 @@ describe('ReactDOMServerHooks', () => {
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
'Warning: Did not expect server HTML to contain a <div> in <div>.',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: 1},
);
Expand All @@ -1834,7 +1842,7 @@ describe('ReactDOMServerHooks', () => {
expect(() => Scheduler.unstable_flushAll()).toErrorDev(
[
'Warning: The object passed back from useOpaqueIdentifier is meant to be passed through to attributes only. Do not read the value directly.',
'Warning: Did not expect server HTML to contain a <div> in <div>.',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <div>.',
],
{withoutStack: 1},
);
Expand Down
10 changes: 9 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMServerIntegrationModes-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,15 @@ describe('ReactDOMServerIntegration', () => {
});

itRenders('an empty strict mode', async render => {
expect(await render(<React.StrictMode />)).toBe(null);
expect(
(
await render(
<div>
<React.StrictMode />
</div>,
)
).firstChild,
).toBe(null);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
});
});
});
10 changes: 3 additions & 7 deletions packages/react-dom/src/__tests__/ReactMount-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,12 @@ describe('ReactMount', () => {
expect(instance1 === instance2).toBe(true);
});

it('should warn if mounting into left padded rendered markup', () => {
it('does not warn if mounting into left padded rendered markup', () => {
const container = document.createElement('container');
container.innerHTML = ReactDOMServer.renderToString(<div />) + ' ';

expect(() =>
ReactDOM.hydrate(<div />, container),
).toErrorDev(
'Did not expect server HTML to contain the text node " " in <container>.',
{withoutStack: true},
);
// This should probably ideally warn but we ignore extra markup at the root.
ReactDOM.hydrate(<div />, container);
});

it('should warn if mounting into right padded rendered markup', () => {
Expand Down
21 changes: 20 additions & 1 deletion packages/react-dom/src/__tests__/ReactRenderDocument-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,12 +368,31 @@ describe('rendering React components at document', () => {
expect(testDocument.body.innerHTML).toBe('Hello world');
});

it('renders over an existing text child without throwing', () => {
it('cannot render over an existing text child at the root', () => {
const container = document.createElement('div');
container.textContent = 'potato';
expect(() => ReactDOM.hydrate(<div>parsnip</div>, container)).toErrorDev(
'Expected server HTML to contain a matching <div> in <div>.',
);
// This creates an unfortunate double text case.
expect(container.textContent).toBe('potatoparsnip');
});
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Looks like this test verifies broken behavior.

I wonder if there's a more explicit way we could write it (e.g. // @gate false and then expect the desired output instead of the unfortunate current output)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I'm not sure what would be best way to encode the subtleties here. This is not a "bug" as in it's not an issue with the implementation details that we could fix. It's an intentional behavior that we want to keep whatever else was in there and changing that behavior could be a breaking change.


it('renders over an existing nested text child without throwing', () => {
const container = document.createElement('div');
const wrapper = document.createElement('div');
wrapper.textContent = 'potato';
container.appendChild(wrapper);
expect(() =>
ReactDOM.hydrate(
<div>
<div>parsnip</div>
</div>,
container,
),
).toErrorDev(
'Expected server HTML to contain a matching <div> in <div>.',
);
expect(container.textContent).toBe('parsnip');
});

Expand Down
35 changes: 28 additions & 7 deletions packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,26 +510,47 @@ describe('ReactDOMServerHydration', () => {

it('Suspense + hydration in legacy mode', () => {
const element = document.createElement('div');
element.innerHTML = '<div>Hello World</div>';
const div = element.firstChild;
element.innerHTML = '<div><div>Hello World</div></div>';
const div = element.firstChild.firstChild;
const ref = React.createRef();
expect(() =>
ReactDOM.hydrate(
<React.Suspense fallback={null}>
<div ref={ref}>Hello World</div>
</React.Suspense>,
<div>
<React.Suspense fallback={null}>
<div ref={ref}>Hello World</div>
</React.Suspense>
</div>,
element,
),
).toErrorDev(
'Warning: Did not expect server HTML to contain a <div> in <div>.',
{withoutStack: true},
);

// The content should've been client rendered and replaced the
// existing div.
expect(ref.current).not.toBe(div);
// The HTML should be the same though.
expect(element.innerHTML).toBe('<div>Hello World</div>');
expect(element.innerHTML).toBe('<div><div>Hello World</div></div>');
});

it('Suspense + hydration in legacy mode (at root)', () => {
const element = document.createElement('div');
element.innerHTML = '<div>Hello World</div>';
const div = element.firstChild;
const ref = React.createRef();
ReactDOM.hydrate(
<React.Suspense fallback={null}>
<div ref={ref}>Hello World</div>
</React.Suspense>,
element,
);

// The content should've been client rendered.
expect(ref.current).not.toBe(div);
// Unfortunately, since we don't delete the tail at the root, a duplicate will remain.
expect(element.innerHTML).toBe(
'<div>Hello World</div><div>Hello World</div>',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the difference in observable behavior between these two tests:

Suspense + hydration in legacy mode
Suspense + hydration in legacy mode (at root)

Seems like the significant difference here is that you have a <div> wrapper around the topmost Suspense boundary in your hydrated content. I think that causes us to add an extra node/segment and maybe the result of that is that we also don't clean up the previous <div>Hello World</div> content? Because of the changed fiber.tag !== HostRoot conditional?

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 test is a bit weird. There's no suspense boundary comment in the HTML so it doesn't find any matching suspense boundary. If that mismatch is at the root, we just assume that we have to insert the DOM node. We assume that we want to keep what was already there - that didn't match. So we end up with two.

If it's not at the root, we still mismatch and insert a new node but we also remove what was already there.

);
});

it('Suspense + hydration in legacy mode with no fallback', () => {
Expand Down
15 changes: 15 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 @@ -894,6 +894,12 @@ export function commitHydratedSuspenseInstance(
retryIfBlockedOn(suspenseInstance);
}

export function shouldDeleteUnhydratedTailInstances(
parentType: string,
): boolean {
return parentType !== 'head' || parentType !== 'body';
}

export function didNotMatchHydratedContainerTextInstance(
parentContainer: Container,
textInstance: TextInstance,
Expand Down Expand Up @@ -1008,6 +1014,15 @@ export function didNotFindHydratableSuspenseInstance(
}
}

export function errorHydratingContainer(parentContainer: Container): void {
if (__DEV__) {
console.error(
'An error occurred during hydration. The server HTML was replaced with client content in <%s>.',
parentContainer.nodeName.toLowerCase(),
);
}
}
bvaughn marked this conversation as resolved.
Show resolved Hide resolved

export function getInstanceFromNode(node: HTMLElement): null | Object {
return getClosestInstanceFromNode(node) || null;
}
Expand Down
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberHostConfigWithNoHydration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export const commitHydratedContainer = shim;
export const commitHydratedSuspenseInstance = shim;
export const clearSuspenseBoundary = shim;
export const clearSuspenseBoundaryFromContainer = shim;
export const shouldDeleteUnhydratedTailInstances = shim;
export const didNotMatchHydratedContainerTextInstance = shim;
export const didNotMatchHydratedTextInstance = shim;
export const didNotHydrateContainerInstance = shim;
Expand All @@ -50,3 +51,4 @@ export const didNotFindHydratableContainerSuspenseInstance = shim;
export const didNotFindHydratableInstance = shim;
export const didNotFindHydratableTextInstance = shim;
export const didNotFindHydratableSuspenseInstance = shim;
export const errorHydratingContainer = shim;
14 changes: 6 additions & 8 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 @@ -43,6 +43,7 @@ import {
hydrateTextInstance,
hydrateSuspenseInstance,
getNextHydratableInstanceAfterSuspenseInstance,
shouldDeleteUnhydratedTailInstances,
didNotMatchHydratedContainerTextInstance,
didNotMatchHydratedTextInstance,
didNotHydrateContainerInstance,
Expand Down Expand Up @@ -438,18 +439,15 @@ function popHydrationState(fiber: Fiber): boolean {
return false;
}

const type = fiber.type;

// If we have any remaining hydratable nodes, we need to delete them now.
// We only do this deeper than head and body since they tend to have random
Copy link
Contributor

Choose a reason for hiding this comment

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

nit This comment is pretty DOM renderer specific

// other nodes in them. We also ignore components with pure text content in
// side of them.
// TODO: Better heuristic.
// side of them. We also don't delete anything inside the root container.
if (
fiber.tag !== HostComponent ||
(type !== 'head' &&
type !== 'body' &&
!shouldSetTextContent(type, fiber.memoizedProps))
fiber.tag !== HostRoot &&
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 the effective change.

(fiber.tag !== HostComponent ||
(shouldDeleteUnhydratedTailInstances(fiber.type) &&
!shouldSetTextContent(fiber.type, fiber.memoizedProps)))
) {
let nextInstance = nextHydratableInstance;
while (nextInstance) {
Expand Down
14 changes: 6 additions & 8 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 @@ -43,6 +43,7 @@ import {
hydrateTextInstance,
hydrateSuspenseInstance,
getNextHydratableInstanceAfterSuspenseInstance,
shouldDeleteUnhydratedTailInstances,
didNotMatchHydratedContainerTextInstance,
didNotMatchHydratedTextInstance,
didNotHydrateContainerInstance,
Expand Down Expand Up @@ -438,18 +439,15 @@ function popHydrationState(fiber: Fiber): boolean {
return false;
}

const type = fiber.type;

// If we have any remaining hydratable nodes, we need to delete them now.
// We only do this deeper than head and body since they tend to have random
// other nodes in them. We also ignore components with pure text content in
// side of them.
// TODO: Better heuristic.
// side of them. We also don't delete anything inside the root container.
if (
fiber.tag !== HostComponent ||
(type !== 'head' &&
type !== 'body' &&
!shouldSetTextContent(type, fiber.memoizedProps))
fiber.tag !== HostRoot &&
(fiber.tag !== HostComponent ||
(shouldDeleteUnhydratedTailInstances(fiber.type) &&
!shouldSetTextContent(fiber.type, fiber.memoizedProps)))
) {
let nextInstance = nextHydratableInstance;
while (nextInstance) {
Expand Down
Loading