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
Change hydration warning index algorithm to use .return pointers, n… 
…ot stack

The stack was required to track where to return after child siblings traversal.

The `fiber.return` pointers were made for that purpose,
it's safe to overwrite them because they are only used to traverse the fiber tree,
and their values outside the traversal functions are not relied upon.

One test fails for yet unknown reason, the text node is inserted too early:
```
  ● ReactMount › should warn when hydrate inserts a text node after matching elements (insertion diff)

    Error: Unexpected warning recorded: - Expected
    + Received

      Warning: Expected server HTML to contain a matching text node for {'SSRMismatchTest client text'} in <div>.

        <div data-reactroot="">
          <div data-ssr-mismatch-padding-before="1"><span></span></div>
          <div data-ssr-mismatch-padding-before="2"></div>
    + +   {'SSRMismatchTest client text'}
          <div data-ssr-mismatch-padding-before="3"></div>
          <div data-ssr-mismatch-padding-before="4"></div>
          <div data-ssr-mismatch-padding-before="5"></div>
          <div data-ssr-mismatch-padding-before="6"></div>
          <div data-ssr-mismatch-padding-before="7"></div>
          <div data-ssr-mismatch-padding-before="8"></div>
          <div data-ssr-mismatch-padding-before="9"></div>
          <div data-ssr-mismatch-padding-before="10"></div>
          <div data-ssr-mismatch-padding-before="11"></div>
          <div data-ssr-mismatch-padding-before="12"></div>
          <div data-ssr-mismatch-padding-before="13"></div>
    - +   {'SSRMismatchTest client text'}
        </div>

          in div (at **)
```
  • Loading branch information
@sompylasar
sompylasar committed Sep 8, 2018
commit 1a15f08a360176222d0f2a4abd816f509ee794bf
40 changes: 25 additions & 15 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 @@ -679,7 +679,9 @@ describe('ReactMount', () => {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="1" />
<div data-ssr-mismatch-padding-before="1">
<span />
</div>
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
Expand Down Expand Up @@ -739,7 +741,7 @@ describe('ReactMount', () => {
).toWarnDev(
'Warning: Expected server HTML to contain a matching <h2> in <div>.\n\n' +
' <div data-reactroot="">\n' +
' <div data-ssr-mismatch-padding-before="1"></div>\n' +
' <div data-ssr-mismatch-padding-before="1"><span></span></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' +
Expand Down Expand Up @@ -910,15 +912,15 @@ describe('ReactMount', () => {

class TestPaddingBeforeInnerInnerComponent extends React.Component {
render() {
return <div data-ssr-mismatch-padding-before="6" />;
return <div data-ssr-mismatch-padding-before="8" />;
}
}
class TestPaddingBeforeInnerComponent extends React.Component {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
<div data-ssr-mismatch-padding-before="6" />
<div data-ssr-mismatch-padding-before="7" />
<TestPaddingBeforeInnerInnerComponent />
</React.Fragment>
);
Expand All @@ -928,12 +930,11 @@ describe('ReactMount', () => {
render() {
return (
<React.Fragment>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
<div data-ssr-mismatch-padding-before="4" />
<div data-ssr-mismatch-padding-before="5" />
<TestPaddingBeforeInnerComponent />
<div data-ssr-mismatch-padding-before="7" />
<div data-ssr-mismatch-padding-before="8" />
<div data-ssr-mismatch-padding-before="9" />
<div data-ssr-mismatch-padding-before="10" />
</React.Fragment>
);
}
Expand All @@ -942,32 +943,40 @@ describe('ReactMount', () => {
const div = document.createElement('div');
const markup = ReactDOMServer.renderToString(
<div>
<div data-ssr-mismatch-padding-before="1" />
<div data-ssr-mismatch-padding-before="1">
<span />
</div>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
<TestPaddingBeforeComponent />
<div data-ssr-mismatch-padding-before="10" />
<div data-ssr-mismatch-padding-before="11" />
<div data-ssr-mismatch-padding-before="12" />
<div data-ssr-mismatch-padding-before="13" />
</div>,
);
div.innerHTML = markup;

expect(() =>
ReactDOM.hydrate(
<div>
<div data-ssr-mismatch-padding-before="1" />
<div data-ssr-mismatch-padding-before="1">
<span />
</div>
<div data-ssr-mismatch-padding-before="2" />
<div data-ssr-mismatch-padding-before="3" />
<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 data-ssr-mismatch-padding-before="13" />
{'SSRMismatchTest client text'}
</div>,
div,
),
).toWarnDev(
'Warning: Expected server HTML to contain a matching text node' +
" for {'SSRMismatchTest client text'} in <div>.\n\n" +
' <div data-reactroot="">\n' +
' <div data-ssr-mismatch-padding-before="1"></div>\n' +
' <div data-ssr-mismatch-padding-before="1"><span></span></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' +
Expand All @@ -979,6 +988,7 @@ describe('ReactMount', () => {
' <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' +
' <div data-ssr-mismatch-padding-before="13"></div>\n' +
"+ {'SSRMismatchTest client text'}\n" +
' </div>\n\n' +
' in div (at **)',
Expand Down
38 changes: 24 additions & 14 deletions packages/react-reconciler/src/ReactFiberHydrationContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@ import type {
HostContext,
} from './ReactFiberHostConfig';

import {HostComponent, HostText, HostRoot} from 'shared/ReactWorkTags';
import {
HostComponent,
HostText,
HostRoot,
HostPortal,
} from 'shared/ReactWorkTags';
import {Deletion, Placement} from 'shared/ReactSideEffectTags';
import invariant from 'shared/invariant';

Expand Down Expand Up @@ -108,24 +113,29 @@ function insertNonHydratedInstance(
if (__DEV__) {
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) {
// Find index of `fiber`, the place where hydration failed, among immediate children host nodes of `returnFiber`.
const startNode = returnFiber.child;
let node: Fiber | null = startNode;
search: 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.

} else if (node.tag === HostPortal) {
// Do not count HostPortal and do not descend into them as they do not affect the index within the parent.
} else if (node.child !== null) {
// Do not descend into HostComponent or HostText as they do not affect the index within the parent.
node.child.return = node;
node = node.child;
continue;
}
// Depth-first traversal.
if (node.child) {
if (node.sibling) {
// Remember where to continue on this tree level, then go deeper.
nextNodeStack.push(node.sibling);
while (node && node.sibling === null) {
if (node.return === null || node.return === startNode) {
break search;
}
node = node.child;
} else if (node.sibling) {
node = node.return;
}
if (node && node.sibling) {
node.sibling.return = node.return;
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.

Expand Down