Nicer Formatting of SSR Validation

sebmarkbage commented 7 years ago

The new validation in #10026 only issues a warn for the first difference found in a HTML hydration scenario. Ideally it should instead queue up all the differences and then at the end (commit) issue a single warning with a nicely formatted diff.

1) Instead of warning add these warn calls to a global buffer (array, map, set, whatever).

2) Inside prepareForCommit, issue all the currently batched up warnings as a single message.

3) Format that message in terms of a JSX diff in a nicely formatted way. With only the relevant nodes (parent and child with changes). Irrelevant child content can be replaced with ellipsis. E.g.

...
<div className="unchanged">
- <div className="foo" />
+ <div className="bar">…</div>
+ <span />
</div>
...
<div className="another_unchanged">
- <span />
</div>
...

This strategy won't yield perfect results because if we're asynchronously hydrating, and it gets interrupted by another tree, we'll flush a warning before the actual hydrating particular tree is flushed. So we might show a partial diff in that case. This is probably. It's just a warning.

akshaynaik404 commented 7 years ago

Hi @sebmarkbage, Hi wanted to work on this issue, but I couldn't understand the info you gave above, also I am a beginner in React.

Is this issue suitable for me to is there some docs that I should read to get up to speed with this codebase ...

sebmarkbage commented 7 years ago

A general guide is here: https://facebook.github.io/react/contributing/how-to-contribute.html

For this particular issue you'll probably want to most test it in the SSR fixture. It's a bit messy because you have to build React itself in the root before testing in fixture.

https://github.com/facebook/react/tree/master/fixtures/ssr

This issue is certainly not easy to get right but also doesn't that much existing knowledge of the code base. Just how React works with SSR.

stepnivlk commented 7 years ago

Hey, @sebmarkbage I'm trying to work on it. I'll probably have some questions, but overall it looks manageable.

quantizor commented 7 years ago

At a minimum, giving the displayName of the component that broke rehydration would be very helpful. I've been seeing a lot of checksum mismatches originating from a library downstream from React (styled-components) and the diff isn't always so helpful because it may just be a generic div and perhaps a CSS class.

stepnivlk commented 7 years ago

Sry guys I've been busy at work this week. I'm trying to follow @sebmarkbage guide. It's still WIP, but now I have much more time for it. What I have in one picture: diff

Two questions (sorry I'm a total newbie to the React codebase): 1) You talk here about JSX, but in diffHydratedProperties I have access to server rendered dom nodes, it's a bit confusing, can you @sebmarkbage please elaborate a bit about those classNames? I'm not sure if I miss something here.

2) My way of doing it is to:

catch all the diff occurrences, store them as an array of objects containing cloned server dom node and different elements.
Iterate over the array, check or clone parent nodes and add/update server and client elements to parent nodes.
create one string from each parent.outerHTML and pass it to a warning. (I still miss some use cases like one client element/no server element)

Is it fine, stupid, or something else? (Again sorry for a bit retarded question I'm really unsure as I'm not familiar with the codebase yet and want to deliver something useful.)

Cheers.

gaearon commented 7 years ago

No question is silly. Can you please send a PR and we'll review it? @sebmarkbage is on a vacation now.

stepnivlk commented 7 years ago

@gaearon Thanks for the reply! OK, I will polish it a bit and open a PR, I'll be glad to get some feedback.

sebmarkbage commented 7 years ago

You talk here about JSX, but in diffHydratedProperties I have access to server rendered dom nodes...

Yea this is a bit confusing. Let me try to clarify.

The existing diff mechanism will give you propName, serverValue and clientValue passed here.

You'll notice that this is not called "attributeName" and the values passed are of type mixed, not only string. That's because these are props not attributes. React's props differ from attributes in that they can model numbers, booleans, style objects as well as strings. They also have different names than the attributes. E.g. a difference in the class attribute will show the propName as "className" here.

A way to visualize this is server-rendering this: <div className={Date.now()} />

Another one is <input type="checkbox" checked={Math.random() > 0.5} />.

Additionally when you format this text I'd expect the output to be JSX compatible, not HTML compatible. For example, self-closing tags like img have to be encoded as <img />, not <img>. IMO this is better to visualize because it is what the developer is actually coding. The transfer format is mostly opaque unless you happen to try to inspect it.

In fact, we'll likely want to experiment with other transfer formats than HTML and this visualization will transfer nicely to such environments. E.g. imagine React Native server rendering.

Similarly text content's encoding can follow what you'd write in JSX, not what the encoding is in the HTML transfer format. Example: <div>{Date.now()} > 0</div>

gaearon commented 7 years ago

We should also probably pass the relevant Fiber's element source to the warnings. This way we can add information about where in the tree we are, similar to other warnings with component stacks.

sebmarkbage commented 7 years ago

@gaearon There isn't just one Fiber that's responsible for the warning. Often it's many which is the point of this batched output. We could possibly include the component stack of the root most parent that failed and join them to create a whole tree view. Seems like it could get very noisy though. Isn't that better left to dev tools if you need a further tree view?

gaearon commented 7 years ago

My concern is it’s not always easy to find which <div> the warning is referring to. For example how would you find something like <div className="generated_abc123"> in your code?

quantizor commented 7 years ago

@gaearon see my comment above. Using displayName where possible would dramatically ease finding the fragment of component hierarchy responsible for the issue. E.g

<Foo>
  <div>

Instead of

<div>
  <div>

sebmarkbage commented 7 years ago

It has the parent, which is at least some context.

We only have the component stack from the client side but not the server side.

One issue with showing too much context is that it can be misleading when something diffs because it doesn't line up correctly. E.g. often many different components will have nested divs that we will consider being a match because of the type but they're actually part of a different component. If we show the diff as part of a component tree, it'll look like the bug was within that component but in reality it was actually just the wrong component being rendered at a higher level.

E.g. if App rendered <div><div className="app" /></div> in a branch instead of <Foo />, you'd get an error like this:

<App>
  <Foo>
    <div>
      <Bar>
-       <div className="app" />
+       <div className="bar" />
      </Bar>
    </div>
  </Foo>
</App>

wenchaojiang commented 7 years ago

Hi, I created a pull request #10737 for demo purpose, just want to get feedback to see whether I am on the right track. I have not fixed tests and linting, it is not ready for merging

basically, I managed to get warning message looks like

I created a HydrationWarningRenderer module which has the following method

setContainer() set the container dom element before hydrating. The container is required for organizing warnings into a tree structure like a dom tree.
registerWarning() instead of calling the old warningFor* methods during hydration, we call registerWarning method to save diffs in a central registry.
flushWarnings() this method is called in prepareForCommit when hydration is completed. There are two steps to print out warnings,
1. buildWarningTree, 2. warningTreeToText
buildWarningTree() starting from current container dom element, recursively build a tree, in which each node is a pair of {curentDomNode, diffs}

warningTreeToText() recursively convert diffs Info into warning string and print out the whole warning tree as formatted string.

I'd appreciate if anyone can let me know if this approach is worth of keeping working on.

gaearon commented 7 years ago

Another possible approach (browser-only): https://github.com/facebook/react/issues/7300

sachalifs commented 7 years ago

It would be nice to see this feature out.

In React v15 the message was more explicit (I could tell which elements were different), but now it just says:

Warning: Expected server HTML to contain a matching <a> in <div>

Also, now I'm having rendering issues due to this warning.

I'm new to React codebase but I will try to give you a hand with this issue too.

gaearon commented 7 years ago

Sorry, we were focused on getting a release out, and didn't look into existing PRs yet.

gaearon commented 6 years ago

The last attempt at this was https://github.com/facebook/react/pull/10737 but we didn't review it in time and it grew stale. https://github.com/facebook/react/issues/7300 is a much simpler solution and maybe we should do something like this instead.

If somebody wants to pick this up, let me know. It's not the easiest task so you should be relatively comfortable with working on your own, and discussing the proposed implementation in this issue before jumping to the code. In particular, you'll need to figure out which UX is the most ergonomic for this problem.

sompylasar commented 6 years ago

@gaearon Hi, I'd like to take a stab at this. "Not the easiest task" sounds great; at least I'll poke around and learn the React internals hands-on.

Do you have any particular preference or thoughts on the desired UX and implementation?

Here's mine:

Dump a text diff via `console` like https://github.com/facebook/react/pull/10737

Pros:

Text is easy to copy and share (post to a chat or ticket), transfer over the network (automatic reporting by hooking into console in a test / CI environment).
Text is easier to compare in tests.
Text console log is available and reliable in every browser and platform.
Text console log is searchable and filterable.

Cons:

Harder to distill the relevant subset of information about the mismatch.
Additional context needs to be looked up from the text manually.

Dump an element reference via `console` like https://github.com/facebook/react/issues/7300

Pros:

Browser Developer Tools highlight the mismatching element.
The user can explore the surrounding DOM to whatever extent necessary.
The user can navigate to React Developer Tools to explore the surrounding VDOM.

Cons:

Element references in console log are only usable in Browser Developer Tools; mind the tools (tests, CI, error reporting) which hook into the console methods to record, detect, and report anomalies.
Browser Developer Tools can display element references differently, may not be available, and not necessarily provide reliable navigation to the surrounding DOM.
React Developer Tools may not be available in every browser and platform.

Both text diff and element reference

Pros:

All of the above.

Cons:

More code and tests to develop and to support.
More information flushed onto the user at once, may be harder to scan.
Heavier development bundle.

sompylasar commented 6 years ago

@gaearon While working on this I discovered that hydrate does not warn on props mismatch when the server-side prop is missing and the browser-side prop is set to null explicitly. Is this expected behavior?

  it('should warn when a hydrated element has an extra prop', () => {
    const div = document.createElement('div');
    const markup = ReactDOMServer.renderToString(<div />);
    div.innerHTML = markup;

    expect(() =>
      ReactDOM.hydrate(<div data-ssr-prop-mismatch={null} />, div),
    ).toWarnDev(
      'Prop `data-ssr-prop-mismatch` did not match. ' +
        'Server: "undefined" ' +
        'Client: "null"\n' +
        '    in div (at **)',
    );
  });

 FAIL  packages/react-dom/src/__tests__/ReactMount-test.js
  ● ReactMount › should warn when a hydrated element has an extra prop

    Expected warning was not recorded:
      "Prop `data-ssr-prop-mismatch` did not match. Server: \"undefined\" Client: \"null\"
        in div (at **)"

      164 |     expect(() =>
      165 |       ReactDOM.hydrate(<div data-ssr-prop-mismatch={null} />, div),
    > 166 |     ).toWarnDev(
      167 |       'Prop `data-ssr-prop-mismatch` did not match. ' +
      168 |         'Server: "undefined" ' +
      169 |         'Client: "null"\n' +

      at Object.<anonymous> (packages/react-dom/src/__tests__/ReactMount-test.js:166:5)

sompylasar commented 6 years ago

Started by adding the existing component stack trace which already adds some value to the warnings if the components are named properly: https://github.com/facebook/react/pull/12063

What do y'all think?

giles-v commented 6 years ago

One closely related problem I have is the vagueness of simply listing the nodeName for large and complex views. In other words, the warning

Did not expect server HTML to contain a <div> in <div>.

... is too vague to debug.

Here is a concept branch which converts this to:

Did not expect server HTML to contain <div class="slide-info-wrapper"> in <div itemprop="articleBody" class="slideshow-container">.

I would love feedback on whether this is useful before I brush it up. Happy to open a separate, more tightly focused issue for it if preferred.

Thank you!

sompylasar commented 6 years ago

@giles-v Oh this looks nice! I'd love to merge this with my component stack PR https://github.com/facebook/react/pull/12063 somehow keeping both of us as contributors.

giles-v commented 6 years ago

@sompylasar thanks! glad it makes sense.

We could add it to your PR, but my initial inclination is that since they don't depend on each other it would be neater to keep PRs as atomic as possible. Let me clean it up anyway, and then I can open as a PR either to master or to your branch.

sompylasar commented 6 years ago

@giles-v I noticed you changed the warnings that I missed (there are two sets of similar functions at the top and bottom of the ReactDOMFiberComponent file). It would be great if you found when do these warnings appear. I only found when the top ones are called, and added tests for these. I'd like to add the component stack and the tests for the bottom ones, too.

giles-v commented 6 years ago

@sompylasar awesome - check. I'll probably try to highlight how these are called via test too; then we can merge our work.

sompylasar commented 6 years ago

Unrelated question, but how do y'all cope with GitHub issue references in commits in conjunction with git rebase?

This commit reference spam above is caused by rebases 😔

sompylasar commented 6 years ago

New updates in the PR, please have a look: https://github.com/facebook/react/pull/12063#issuecomment-368380888

react-ssr-warning-diff

WOLVIE97 commented 6 years ago

i just really want Android 8.1 Oreo already... i mean daaaaaaang.LOLOL yeahduh-duh mean yeahduh-sayn'...haha, its true. anyone can tell me how to get this 7.1 nugett to upgrade on my Samsung G Note8 i would be much abliged?

sompylasar commented 6 years ago

The latest screen recording from fixtures/ssr (7.8MB – click to expand)

![react-fixtures-ssr-10mb](https://user-images.githubusercontent.com/498274/41187303-53d64b02-6b5b-11e8-9730-729dd2817d12.gif)

sompylasar commented 6 years ago

I think the label on this issue should be changed from "good first issue" to "good first issue (taken)" as I've been working on it for quite some time and reached quite some progress in here: https://github.com/facebook/react/pull/12063

joseantonjr commented 6 years ago

I can work on this issue, if it's still open. Please let me know

sompylasar commented 6 years ago

@joseantonjr I'm sorry but it's taken and almost done twice: here https://github.com/facebook/react/pull/12063 (this approach was abandoned after code review from the React Core team) and here https://github.com/facebook/react/pull/13602 (pending code review from the React Core team).

rikkit commented 5 years ago

Excited for this to be merged... SSR setup is finicky enough without having to debug Did not expect server HTML to contain a <div> in <div>. !

chulanovskyi commented 5 years ago

After so much effort from @sompylasar, we still pocking around with the Warning: Did not expect server HTML to contain a <div> in <div>.. Sad :(

gaearon commented 4 years ago

We didn't end up doing precisely this. However, we did add component stacks to hydration warnings in React 16.13: https://reactjs.org/blog/2020/03/02/react-v16.13.0.html#component-stacks-in-hydration-warnings

That should get you most of the way there. They might still be occasionally misleading because the problem might be in one of the parent components rendering something unexpected. But they're much better than nothing. I think we can close this as other solutions are too complex.

threepointone commented 4 years ago

I don't think this should have been closed, because one of the goals was to show all warnings that happen on a pass, instead of just the first one that occurs.

If there's interest in this, I'll be happy to be the one who removes the didWarnInvalidHydration flag that enforces this once-only constraint, and fix the ~260 tests that fail on removing it.

gaearon commented 7 months ago

We're adding proper diffs in https://github.com/facebook/react/pull/28512.

facebook / react