Skip to content

DOM diff and component stack in SSR hydrate mismatch warnings (#10085) #12063

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

Closed
sompylasar wants to merge 14 commits into from
Closed

DOM diff and component stack in SSR hydrate mismatch warnings (#10085) #12063

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a11486a
Add DOM hydrate mismatch test cases to fixtures/ssr
sompylasar Jan 20, 2018
6fc330d
Add all warning cases to fixtures/ssr
sompylasar Feb 18, 2018
283a56c
Add tests for all hydrate warning cases like in fixtures/ssr
sompylasar Feb 18, 2018
7ea192a
Proof-of-concept hydrate warning diff
sompylasar Feb 18, 2018
3aec458
Add diff to hydrate warnings
sompylasar Feb 26, 2018
9050bc5
Refactor hydrate warnings with feedback from KidkArolis, mxstbr
sompylasar Jun 8, 2018
bec7032
Add placeholder element to fixtures/ssr to keep it focused in DevTools
sompylasar Jun 9, 2018
8b9be96
Replace unicode ellipsis with ASCII ellipsis (three dots)
sompylasar Jun 19, 2018
949f2de
Add tests to emphasize special character output problem
sompylasar Jun 19, 2018
2240ab9
Add failing test: fiber.index is not the SSR host node index for the …
sompylasar Jun 23, 2018
31d7c85
Refactor ReactMount SSR warning tests
sompylasar Jun 27, 2018
b8c5b9d
Rename SSR warning variables to be more descriptive what they stand for
sompylasar Jun 27, 2018
221bc1f
Compute hydrationWarningHostInstanceIndex by traversing the fiber tree
sompylasar Jul 8, 2018
1a15f08
Change hydration warning index algorithm to use `.return` pointers, n…
sompylasar Aug 3, 2018
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
Prev Previous commit
Next Next commit
Compute hydrationWarningHostInstanceIndex by traversing the fiber tree
  • Loading branch information
@sompylasar
sompylasar committed Sep 8, 2018
commit 221bc1f42a2eebc43e376fa883bec9e90a1f23d3
135 changes: 131 additions & 4 deletions fixtures/ssr/src/components/SSRMismatchTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,132 @@ const testCases = [
</div>
),
},
{
key:
'ssr-hydrationWarningHostInstanceIndex-didNotFindHydratableInstance-replacement',
render: isServer => {
class TestPaddingBeforeInnerComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
</React.Fragment>
);
}
}
class TestPaddingBeforeComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
</React.Fragment>
);
}
}
class TestPaddingAfterComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-after="1" />
<div data-ssr-mismatch-padding-after="2" />
<div data-ssr-mismatch-padding-after="3" />
<div data-ssr-mismatch-padding-after="4" />
<div data-ssr-mismatch-padding-after="5" />
</React.Fragment>
);
}
}
class TestNestedComponent extends React.Component {
render() {
if (this.props.isServer) {
return (
<div>
<TestPaddingBeforeComponent />
<h1>SSRMismatchTest default text</h1>
<span />
<TestPaddingAfterComponent />
</div>
);
}
return (
<div>
<TestPaddingBeforeComponent />
<h2>SSRMismatchTest default text</h2>
<span />
<TestPaddingAfterComponent />
</div>
);
}
}
class TestComponent extends React.Component {
render() {
return <TestNestedComponent isServer={this.props.isServer} />;
}
}

return <TestComponent isServer={isServer} />;
},
},

{
key:
'ssr-hydrationWarningHostInstanceIndex-didNotFindHydratableInstance-insertion',
render(isServer) {
class TestPaddingBeforeInnerInnerComponent extends React.Component {
render() {
return <div data-ssr-mismatch-padding-before="6" />;
}
}
class TestPaddingBeforeInnerComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
<TestPaddingBeforeInnerInnerComponent />
</React.Fragment>
);
}
}
class TestPaddingBeforeComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="7" />
<div data-ssr-mismatch-padding-before="8" />
<div data-ssr-mismatch-padding-before="9" />
</React.Fragment>
);
}
}

return isServer ? (
<div>
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeComponent />
<div data-ssr-mismatch-padding-before="10" />
<div data-ssr-mismatch-padding-before="11" />
<div data-ssr-mismatch-padding-before="12" />
</div>
) : (
<div>
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeComponent />
<div data-ssr-mismatch-padding-before="10" />
<div data-ssr-mismatch-padding-before="11" />
<div data-ssr-mismatch-padding-before="12" />
SSRMismatchTest client text
</div>
);
},
},
];

// Triggers the DOM mismatch warnings if requested via query string.
Expand All @@ -297,13 +423,14 @@ export default class SSRMismatchTest extends Component {
);
if (testCaseFound) {
// In the browser where `window` is available, triggering a DOM mismatch if it's requested.
const isServer = typeof window === 'undefined';
let render;
if (typeof window !== 'undefined') {
render = testCaseFound.renderBrowser || testCaseFound.render;
} else {
if (isServer) {
render = testCaseFound.renderServer || testCaseFound.render;
} else {
render = testCaseFound.renderBrowser || testCaseFound.render;
}
content = render();
content = render(isServer);
}

return (
Expand Down
95 changes: 72 additions & 23 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 @@ -663,13 +663,24 @@ describe('ReactMount', () => {
});

it('should warn when hydrate replaces an element within server-rendered nested components (replacement diff)', () => {
class TestPaddingBeforeComponent extends React.Component {
// See fixtures/ssr: ssr-hydrationWarningHostInstanceIndex-didNotFindHydratableInstance-replacement

class TestPaddingBeforeInnerComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="1" />
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
</React.Fragment>
);
}
}
class TestPaddingBeforeComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
</React.Fragment>
Expand All @@ -691,7 +702,7 @@ describe('ReactMount', () => {
}
class TestNestedComponent extends React.Component {
render() {
if (this.props.server) {
if (this.props.isServer) {
return (
<div>
<TestPaddingBeforeComponent />
Expand All @@ -713,18 +724,18 @@ describe('ReactMount', () => {
}
class TestComponent extends React.Component {
render() {
return <TestNestedComponent server={this.props.server} />;
return <TestNestedComponent isServer={this.props.isServer} />;
}
}

const div = document.createElement('div');
const markup = ReactDOMServer.renderToString(
<TestComponent server={true} />,
<TestComponent isServer={true} />,
);
div.innerHTML = markup;

expect(() =>
ReactDOM.hydrate(<TestComponent server={false} />, div),
ReactDOM.hydrate(<TestComponent isServer={false} />, div),
).toWarnDev(
'Warning: Expected server HTML to contain a matching <h2> in <div>.\n\n' +
' <div data-reactroot="">\n' +
Expand Down Expand Up @@ -894,27 +905,60 @@ describe('ReactMount', () => {
);
});

it('should warn when hydrate inserts a text node between matching elements (insertion diff)', () => {
// See fixtures/ssr: ssr-warnForInsertedHydratedText-didNotFindHydratableTextInstance
it('should warn when hydrate inserts a text node after matching elements (insertion diff)', () => {
// See fixtures/ssr: ssr-hydrationWarningHostInstanceIndex-didNotFindHydratableInstance-insertion

class TestPaddingBeforeInnerInnerComponent extends React.Component {
render() {
return <div data-ssr-mismatch-padding-before="6" />;
}
}
class TestPaddingBeforeInnerComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
<TestPaddingBeforeInnerInnerComponent />
</React.Fragment>
);
}
}
class TestPaddingBeforeComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="7" />
<div data-ssr-mismatch-padding-before="8" />
<div data-ssr-mismatch-padding-before="9" />
</React.Fragment>
);
}
}

const div = document.createElement('div');
const markup = ReactDOMServer.renderToString(
<div>
<span data-ssr-mismatch-padding-before="1" />
<span />
<div data-ssr-mismatch-padding-after="1" />
<div data-ssr-mismatch-padding-after="2" />
<div data-ssr-mismatch-padding-after="3" />
<div data-ssr-mismatch-padding-after="4" />
<div data-ssr-mismatch-padding-after="5" />
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeComponent />
<div data-ssr-mismatch-padding-before="10" />
<div data-ssr-mismatch-padding-before="11" />
<div data-ssr-mismatch-padding-before="12" />
</div>,
);
div.innerHTML = markup;

expect(() =>
ReactDOM.hydrate(
<div>
<span data-ssr-mismatch-padding-before="1" />
<div data-ssr-mismatch-padding-before="1" />
<TestPaddingBeforeComponent />
<div data-ssr-mismatch-padding-before="10" />
<div data-ssr-mismatch-padding-before="11" />
<div data-ssr-mismatch-padding-before="12" />
SSRMismatchTest client text
</div>,
div,
Expand All @@ -923,14 +967,19 @@ describe('ReactMount', () => {
'Warning: Expected server HTML to contain a matching text node' +
" for {'SSRMismatchTest client text'} in <div>.\n\n" +
' <div data-reactroot="">\n' +
' <span data-ssr-mismatch-padding-before="1"></span>\n' +
' <div data-ssr-mismatch-padding-before="1"></div>\n' +
' <div data-ssr-mismatch-padding-before="2"></div>\n' +
' <div data-ssr-mismatch-padding-before="3"></div>\n' +
' <div data-ssr-mismatch-padding-before="4"></div>\n' +
' <div data-ssr-mismatch-padding-before="5"></div>\n' +
' <div data-ssr-mismatch-padding-before="6"></div>\n' +
' <div data-ssr-mismatch-padding-before="7"></div>\n' +
' <div data-ssr-mismatch-padding-before="8"></div>\n' +
' <div data-ssr-mismatch-padding-before="9"></div>\n' +
' <div data-ssr-mismatch-padding-before="10"></div>\n' +
' <div data-ssr-mismatch-padding-before="11"></div>\n' +
' <div data-ssr-mismatch-padding-before="12"></div>\n' +
"+ {'SSRMismatchTest client text'}\n" +
' <span></span>\n' +
' <div data-ssr-mismatch-padding-after="1"></div>\n' +
' <div data-ssr-mismatch-padding-after="2"></div>\n' +
' <div data-ssr-mismatch-padding-after="3"></div>\n' +
' <div data-ssr-mismatch-padding-after="4"></div>\n' +
' <div data-ssr-mismatch-padding-after="5"></div>\n' +
' </div>\n\n' +
' in div (at **)',
);
Expand Down
26 changes: 24 additions & 2 deletions packages/react-reconciler/src/ReactFiberHydrationContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,30 @@ function insertNonHydratedInstance(
) {
fiber.effectTag |= Placement;
if (__DEV__) {
// TODO: Find out what `hydrationWarningHostInstanceIndex` should be, `fiber.index` is wrong.
const hydrationWarningHostInstanceIndex = fiber.index;
let hydrationWarningHostInstanceIndex = 0;
{
// Count rendered host nodes by traversing `returnFiber` subtree until `fiber` is found.
let node = returnFiber.child;
const nextNodeStack = [];
while (node && node !== fiber) {
if (node.tag === HostComponent || node.tag === HostText) {
++hydrationWarningHostInstanceIndex;
Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

If a node is a host component, why do we need to go inside of it? I imagine we'd want to skip over it to search for the next one. Otherwise we'd count its children (which we shouldn't). Am I wrong?

Copy link
Contributor Author

@sompylasar sompylasar Aug 1, 2018

Choose a reason for hiding this comment

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

Yes I just had this thought, too, while drawing the diagram. Should it not have children by definition though?

Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor Author

@sompylasar sompylasar Aug 1, 2018

Choose a reason for hiding this comment

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

Okay, I think I got it: if a HostComponent is e.g. a div, it may have children. Then we should only count children-less HostComponents and HostTexts.

Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

Why shouldn’t we couldn’t them?

I think we should count them regardless of whether they have children or not. We just shouldn’t descend into them because their children have no effect on the index.

That said see my comment below.

Copy link
Contributor Author

@sompylasar sompylasar Aug 1, 2018

Choose a reason for hiding this comment

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

Yes, you're right. I assumed we need to count leaf host nodes, but in fact we need to count only the parent's immediate children host nodes. Thanks for the repro test case.

}
// Depth-first traversal.
if (node.child) {
if (node.sibling) {
// Remember where to continue on this tree level, then go deeper.
nextNodeStack.push(node.sibling);
}
node = node.child;
} else if (node.sibling) {
node = node.sibling;
} else {
node = nextNodeStack.pop();
}
}
}
Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

Can you please describe this algorithm in words? What is it looking for in particular?

Specifically I wonder if you can avoid the need for nextNodeStack.

Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

OK, I think I see what you're doing.

You're starting the search at the return fiber, and then looking for this fiber, counting how many host children there appear to be between them.

I think you can still avoid nextNodeStack by using .return pointers instead where necessary. However, you'll need to remember to write to them every time you descend down. See for example propagateContextChange.

Copy link
Contributor Author

@sompylasar sompylasar Aug 1, 2018

Choose a reason for hiding this comment

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

sketch-1533143022802 01

Yes, I saw the traversal variant with writing into the return pointers but I thought it would be unsafe to modify the structure owned and maintained by another piece of code. If return pointers are only used during one traversal and their previous state is not relied upon, I can rewrite to use them instead of the stack.

Said that, this traversal is not performance-critical as it happens only in development mode and only when hydration failed.

Copy link
Collaborator

@gaearon gaearon Aug 1, 2018

Choose a reason for hiding this comment

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

If return pointers are only used during one traversal and their previous state is not relied upon, I can rewrite to use them instead of the stack.

Yeah, we rely on this pretty much everywhere.

Said that, this traversal is not performance-critical as it happens only in development mode and only when hydration failed.

Agree but would prefer consistency since that's how we traverse elsewhere.


switch (returnFiber.tag) {
case HostRoot: {
const parentContainer = returnFiber.stateNode.containerInfo;
Expand Down