DOM diff and component stack in SSR hydrate mismatch warnings (#10085) #12063
Conversation
|
Can this be merged please?
^ completely useless |
|
@sompylasar This is amazing - thank you for the PR! I am wondering for the |
|
@oyeanuj Thanks, yes, I'll see what I can come up with. Currently, I realized I missed a few cases: |
|
Hey @sompylasar, thanks for the cite - since the other PR #11215 is approved I would rather it stayed separate. I don’t know the approval / merge timeline. @evan-scott-zocdoc, since #12115 resolves your issue and should be complete, you may want to leave a comment there too. |
|
FYI, I'm working on a complete set of warning cases, starting at the fixtures app, then translating the findings into the test suite. Preview screenshot of fixtures/ssr
|
sompylasar
force-pushed
the
10085-nicer-ssr-mismatch-warning
branch
from
1ab33f4 to
8aca686
Compare
|
Rebased on top of latest |
|
Here's what I got here so far:
|
|
@oyeanuj Hey, have a look, diffs!
Unfortunately, to make this possible, I had to extend didNotFindHydratableInstance(
parentType: T,
parentProps: P,
parentInstance: I,
type: T,
props: P,
+ index: number,
): void,and modify didNotFindHydratableInstance(
parentType,
parentProps,
parentInstance,
type,
props,
+ fiber.index,
);into didNotFindHydratableInstance(
parentType: string,
parentProps: Props,
parentInstance: Instance,
type: string,
props: Props,
+ index: number,
) {
if (__DEV__ && parentProps[SUPPRESS_HYDRATION_WARNING] !== true) {
- warnForInsertedHydratedElement(parentInstance, type, props);
+ warnForInsertedHydratedElement(parentInstance, type, props, index);
}
},I haven't cleaned up the diff building code and haven't updated the tests so please bare with me, it's yet a POC: sompylasar@6c425e7 |
Details of bundled changes.Comparing: b87aabd...1a15f08 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
schedule
Generated by 🚫 dangerJS |
Add diff to hydrate warnings (#10085)Renders DOM attributes in the tags mentioned in the warnings. Borrows the idea and partially implementation of Renders DOM diff showing visually the location where the hydration failed. Example warning with a diff: Requires changes to ReactFiberReconciler interface functions that handle hydration errors to distinguish insertion from replacement and show insertion as one added line in the diff; show replacement as one removed, one added line, at correct position among the parentInstance's DOM children:
Extends the proof-of-concept at commit 6c425e7
See the tests for more warning examples. |
|
The most recent CircleCI build failure reason is unrelated to this PR:
```
$ node ./scripts/tasks/danger
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063/commits
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos//issues/12063
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063/commits
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063/reviews
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Request failed [401]: https://api.github.com/repos/facebook/react/pulls/12063/requested_reviewers
Response: {
"message": "Bad credentials",
"documentation_url": "https://developer.github.com/v3"
}
Error: TypeError: Cannot read property 'repo' of undefined
at GitHub.APIMetadataForPR (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:274:27)
at GitHub. (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:164:39)
at step (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:40:23)
at Object.next (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:21:53)
at fulfilled (/home/circleci/project/node_modules/danger/distribution/platforms/GitHub.js:12:58)
at
at process._tickCallback (internal/process/next_tick.js:188:7)
Danger failed
```
|
sompylasar
changed the title
| @@ -946,18 +946,28 @@ const DOMRenderer = ReactFiberReconciler({ | |||
| parentContainer: Container, | |||
| type: string, | |||
| props: Props, | |||
| index: number, | |||
| isReplaced: boolean, | |||
There was a problem hiding this comment.
@acdlite Hi,
This PR is about showing a nice DOM diff and a component stack in the hydration mismatch warnings for developers.
To implement the diff I had to add two additional arguments 1) index: number and 2) isReplaced: boolean to the ReactFiberReconciler's didNotFindHydratable* methods to pass down to the warning-generating functions 1) the index of the child node that's being hydrated within the parent node's hydratable children and 2) the fact that it's being replaced or inserted.
The index is required to show the minus/plus diff prefix at the correct line when looping through the parent node's hydratable children while constructing the diff.
The isReplaced fact is required to not add the minus diff prefix when the preceding sibling node was not in fact removed during hydration.
Example warning when the <p> element did not match and was replaced with the <div> element (isReplaced: true):
Warning: Expected server HTML to contain a matching <div>{['children ', …]}</div>
in <div>nested<!-- --> <p>children <b>text</b></p></div>.
<div>
{'nested'}
{' '}
- <p>children <b>text</b></p>
+ <div>{['children ', …]}</div>
</div>
in div (at **)
in Component (at **)
Example warning when the <p> element did match thus not removed/replaced, the <div> element was added (isReplaced: false):
Warning: Expected server HTML to contain a matching <div>{['children ', …]}</div>
in <div>nested<!-- --> <p>children <b>text</b></p></div>.
<div>
{'nested'}
{' '}
<p>children <b>text</b></p>
+ <div>{['children ', …]}</div>
</div>
in div (at **)
in Component (at **)
Could you please approve this change or recommend another direction?
Thank you.
|
@sompylasar Thank you for all your work on this PR! Hopefully, we can get this reviewed soon and merged! |
|
Hey sorry for the lack of review so far. Been pretty busy preparing for 16.3. We'll try our best to look at this soon. |
|
@acdlite Sure, no worries, at least thank you for giving the notice here. Good luck with 16.3, looking forward to it. |
|
@acdlite @sompylasar can this PR be re-based and reviewed soon? |
|
Sorry, it's still hard to find the time. I think a better strategy is to get someone who's actively using React SSR (cc @rauchg) to help out with testing this and reviewing. |
|
fwiw, I ran this branch locally in an SSR project. Works well, prints more useful info than
Could be formatted slightly better (?), e.g. the first bit is very long in my case, perhaps split X and Y in |
|
Ah, I didn't understand this error message at first, since those long divs kind of all got jumbled, didn't notice the My actual issue was that the client was trying to render an empty app. This new diffing feature is very helpful have I realised where the diff begins. |
|
Or even something like: |
|
@KidkArolis Yes, valid suggestions, the readability can be improved. |
sompylasar
force-pushed
the
10085-nicer-ssr-mismatch-warning
branch
2 times, most recently
from
b34d07b to
9afbc48
Compare
|
👆Rebased to the current |
|
@sompylasar can you publish this to |
|
Before we go too far into that route I'd like to better understand why we need indexing at all. Since we're able to insert/delete at the right place, why don't we have sufficient information for creating the diff? |
As I figured out, the "right place" hydration state is maintained implicitly within the reconciler program via the "instruction counter" (i.e. at which point of the program is the JS engine now, how many host nodes within a parent host node the reconciliation code has passed via react/packages/react-dom/src/client/ReactDOMHostConfig.js Lines 427 to 440 in 28cd494 This host node hydration index is required because the real DOM which we traverse and display in the diff contains more nodes than were hydrated (i.e. matched with the virtual DOM rendered by the components), but the program does not know how many the reconciler has passed/skipped (from the beginning of the parent node) and how many remain unreconciled (from the end of the parent node). If |
Let's say we can change any APIs related to hydration in the host config, but we have a new constraint: the diffing and formatting logic should be in the reconciler, and any renderer with hydration should get warnings with diffs “for free”. How would you solve that problem? |
|
Would you like a separate branch/PR from scratch for that, or keep it all here? |
|
What's more convenient for you? Separate is fine as long as we carry tests over. |
sompylasar
force-pushed
the
10085-nicer-ssr-mismatch-warning
branch
2 times, most recently
from
c022a52 to
7d2c991
Compare
* Add into existing fixture, not as a new one, to avoid duplicating server-rendering and hot-reloading code. * Add the browser-side counterpart for the `url` that was already passed to `render` at the server but wasn't used. Pass it down to the `App` component.
Next, will add a test per every case.
Example warning:
```
Warning: Expected server HTML to contain a matching <em> in <div>.
<div className="SSRMismatchTest__wrapper">
…
<span className="SSRMismatchTest__2">2</span>
<span className="SSRMismatchTest__3">3</span>
<span className="SSRMismatchTest__4">4</span>
<span className="SSRMismatchTest__5">5</span>
<span className="SSRMismatchTest__6">6</span>
- <strong> SSRMismatchTest default text </strong>
+ <em />
<span className="SSRMismatchTest__7">7</span>
<span className="SSRMismatchTest__8">8</span>
<span className="SSRMismatchTest__9">9</span>
<span className="SSRMismatchTest__10">10</span>
<span className="SSRMismatchTest__11">11</span>
…
</div>
in em (at SSRMismatchTest.js:224)
in div (at SSRMismatchTest.js:217)
in div (at SSRMismatchTest.js:283)
in SSRMismatchTest (at App.js:14)
in div (at App.js:11)
in body (at Chrome.js:17)
in html (at Chrome.js:9)
in Chrome (at App.js:10)
in App (at index.js:8)
```
https://user-images.githubusercontent.com/498274/36351251-d04e8fca-145b-11e8-995d-389e0ae99456.png
Renders DOM attributes in the tags mentioned in the warnings. Borrows the idea and partially implementation of `getNodeSignature` from @giles-v facebook#12115 Renders DOM diff showing visually the location where the hydration failed. Example warning with a diff: ``` Warning: Expected server HTML to contain a matching <div>{['children ', …]}</div> in <div>nested<!-- --> <p>children <b>text</b></p></div>. <div> {'nested'} {' '} <p>children <b>text</b></p> + <div>{['children ', …]}</div> </div> in div (at SSRMismatchTest.js:280) in div (at SSRMismatchTest.js:275) in div (at SSRMismatchTest.js:308) in SSRMismatchTest (at App.js:14) in div (at App.js:11) in body (at Chrome.js:17) in html (at Chrome.js:9) in Chrome (at App.js:10) in App (at index.js:8) ``` Requires changes to ReactFiberReconciler interface functions that handle hydration errors to distinguish insertion from replacement and show insertion as one added line in the diff; show replacement as one removed, one added line, at correct position among the parentInstance's DOM children: - add `index` (use `fiber.index`) to point at which child node the insertion or replacement occurs; - add `isReplaced` to distinguish insertion from replacement. Extends the proof-of-concept at commit a5e9a70 https://user-images.githubusercontent.com/498274/36652198-11bb46fe-1a62-11e8-9fa2-a612827d1463.gif
- Change headline to contain only the tag names, not attributes and children. - Add newlines between headline, diff, and stack. Besides the above feedback: - Add comment nodes display in the diff. - Add weird node handling in the diff.
When going through the fixtures one by one it's convenient to see the server-side rendered markup in the Chrome DevTools Elements along with the generated warning in Console. The placeholder element adds consistency to the markup: it does not disappear with page reloads, so Chrome DevTools is smart enough to keep it focused.
Stick to ASCII in the source code to avoid potential cross-browser compatibility and charset issues.
…diff
```
● ReactMount › should warn when hydrate replaces an element within server-rendered nested components
Error: Unexpected warning recorded: - Expected
+ Received
Warning: Expected server HTML to contain a matching <h2> in <div>.
<div data-reactroot="">
<div data-ssr-mismatch-padding-before="1"></div>
- <div data-ssr-mismatch-padding-before="2"></div>
+ - <div data-ssr-mismatch-padding-before="2"></div>
+ + <h2>SSRMismatchTest default text</h2>
<div data-ssr-mismatch-padding-before="3"></div>
<div data-ssr-mismatch-padding-before="4"></div>
<div data-ssr-mismatch-padding-before="5"></div>
- - <h1>SSRMismatchTest default text</h1>
- + <h2>SSRMismatchTest default text</h2>
+ <h1>SSRMismatchTest default text</h1>
<span></span>
<div data-ssr-mismatch-padding-after="1"></div>
<div data-ssr-mismatch-padding-after="2"></div>
<div data-ssr-mismatch-padding-after="3"></div>
<div data-ssr-mismatch-padding-after="4"></div>
<div data-ssr-mismatch-padding-after="5"></div>
</div>
in h2 (at **)
in div (at **)
in TestNestedComponent (at **)
in TestComponent (at **)
```
…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 **)
```
sompylasar
force-pushed
the
10085-nicer-ssr-mismatch-warning
branch
from
7d2c991 to
1a15f08
Compare
|
A separate PR submitted here: #13602 – it's still work-in-progress as of now. |
|
PR #13602 seems to have reached solid enough state to replace this one, so if the design decision of moving the warnings to the reconciler holds, should we close this and focus on the reconciler one? |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
Okay, stale[bot], I guess we can close this one, as nobody's been looking at it, and focus on #13602 instead. |
Renders DOM attributes in the tags mentioned in the warnings. Borrows the idea and partially implementation of
getNodeSignaturefrom @giles-v #12115Renders DOM diff and component stack showing visually the location where the hydration failed. Example warning with a diff (see the
fixtures/ssrand the tests for more warning examples):Requires changes to ReactFiberReconciler interface functions that handle hydration errors to distinguish insertion from replacement and show insertion as one added line in the diff; show replacement as one removed, one added line, at correct position among the parentInstance's DOM children:
index(usefiber.index) to point at which child node the insertion or replacement occurs;isReplacedto distinguish insertion from replacement.The latest screen recording from
fixtures/ssr(7.8MB – click to expand)Previous revisions
Extends the proof-of-concept at commit 6c425e7
Example warning:
Fixes #10085