Nicer Formatting of SSR Validation

[sebmarkbage]

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]

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]

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]

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

[quantizor]

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]

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:

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]

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

[stepnivlk]

@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]

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()} &gt; 0</div>

[gaearon]

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]

@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]

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]

@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]

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]

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]

Another possible approach (browser-only): #7300

[sachalifs]

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]

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

[gaearon]

The last attempt at this was #10737 but we didn't review it in time and it grew stale. #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]

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

[sompylasar]

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: #12063

[joseantonjr]

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

[sompylasar]

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

[rikkit]

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]

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]

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]

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]

We're adding proper diffs in #28512.