Skip to content

Commit

Permalink
Don't delete trailing mismatches during hydration at the root (facebo…
Browse files Browse the repository at this point in the history
…ok#21021)

* Don't delete any trailing nodes in the container during hydration error

* Warn when an error during hydration causes us to clear the container

* Encode unfortunate case in test

* Wrap the root for tests that are applicable to nested cases

* Now we can pipe Fizz into a container

* Grammatical fix
  • Loading branch information
sebmarkbage authored and zhengjitf committed Mar 23, 2021
1 parent e2a1825 commit 2facf5e
Show file tree
Hide file tree
Showing 15 changed files with 159 additions and 63 deletions.
29 changes: 13 additions & 16 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
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
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);
});
});
});
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
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);
});
});
});
10 changes: 3 additions & 7 deletions packages/react-dom/src/__tests__/ReactMount-test.js
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
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');
});

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
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>',
);
});

it('Suspense + hydration in legacy mode with no fallback', () => {
Expand Down
15 changes: 15 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
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(),
);
}
}

export function getInstanceFromNode(node: HTMLElement): null | Object {
return getClosestInstanceFromNode(node) || null;
}
Expand Down
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
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
14 changes: 6 additions & 8 deletions packages/react-reconciler/src/ReactFiberHydrationContext.old.js
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

0 comments on commit 2facf5e

Please sign in to comment.