Warn When The HTML Mismatches in DEV #10026

sebmarkbage · 2017-06-22T22:42:49Z

In the new hydration strategy we do our best to recover when the server response isn't valid but we don't check the attributes. However, we should still warn in DEV because that can end up pretty bad.

This starts adding warnings for those cases. I still have several steps left but this can ideally be landed at any midway point if someone wants to review it.

Warn for differences in text content.
Warn for known props that have different values.
Warn for extra attributes that are not known.
Warn for dangerouslySetInnerHTML differences.
Warn for extra nodes that will be deleted.
Warn for missing nodes that will have a new node inserted.

Left this for a follow up PR.

Warn for inline styles that have different values. Warn for inline style mismatches #10084
Add more test coverage for individual attribute mismatches, insert/delete, style mismatches, single child text content mismatches. Warn for inline style mismatches #10084
Consolidate multiple warnings into a single warn (ideally with a HTML diff like format) instead of just warning for one difference at a time. Nicer Formatting of SSR Validation #10085

sebmarkbage · 2017-06-23T20:44:46Z

scripts/fiber/tests-passing-except-dev.txt

-* should error reconnecting a div with children separated by different whitespace on the server
-* should error reconnecting a div with children separated by different whitespace
-* can distinguish an empty component from a dom node
+* renders a textarea with a value and an onChange with client render on top of good server markup


We currently apparently removing the text node from the textarea even though we really just manage the value property. Need to fix that by not trying to hydrate textarea content. Might be related to dangerouslySetInnerHTML.

sebmarkbage · 2017-06-23T20:45:55Z

scripts/fiber/tests-passing-except-dev.txt

+* renders a textarea value overriding defaultValue no matter the prop order with client render on top of good server markup
+* renders a controlled textarea with client render on top of good server markup
+* throws when rendering if getChildContext exists without childContextTypes with client render on top of bad server markup
+* throws when rendering if getChildContext returns a value not in childContextTypes with client render on top of bad server markup


These might need to be updated. This is issuing a warning because the rendered content of an error boundary is not the same content as the server rendered content.

sebmarkbage · 2017-06-23T22:40:57Z

@aickin This basically would've been a broken mess without your tests. So super helpful to find edge cases.

sebmarkbage · 2017-06-23T23:44:47Z

scripts/fiber/tests-passing-except-dev.txt

-* renders a self-closing tag as a child with client render on top of bad server markup
-* renders simple numbers with client render on top of bad server markup
-* renders simple strings with client render on top of bad server markup
-* renders string prop with true value with client render on top of bad server markup


I guess all of these should test these particular values can issue warnings, but in reality they all just test that the tag name didn't match. Which is why this fixes all of them.

nhunzaker · 2017-06-27T21:19:37Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+      serverText,
+      clientText,
+    );
+  };


Sort of beside the point, but I'm curious if this warning gets lifted out with uglify. Has there been any consideration to a warnOnce?

Even if hoisted, this assignment would get stripped, so it'd still get DCE.

We have had a few warnOnce solutions but the context of "once" varies a lot so hasn't been worth the abstraction cost. A few:

Once per React module instance.

Once per component class.

Once per component instance.

Once per subtree.

These can then combine with:

Once per each type of warning.

Once for any of multiple types of warnings.

Etc.

In this case, we should ideally gather all of these warnings up into one actionable "diff view" of all the differences. That's on my todo. To avoid spam, right now I just want to show the first diff among any type of diff.

Awesome, that's great to know. Very cool.

nhunzaker · 2017-06-27T21:23:00Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+          case 'checked':
+            break;
+          case 'selected':
+            break;


Could these be collapsed into a single break;, like:

case 'data-reactroot': case 'data-reactid': // etc break;

?

We have a lint rule against fall through.

We fall through elsewhere, e.g. https://github.com/facebook/react/blob/master/src/renderers/shared/fiber/ReactFiberBeginWork.js#L864-L869

I was also drawing from:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js#L156-L186

This triggers our non-reuse mode. This is covered by ReactMount already but the test doesn't pass yet without also landing facebook#10026.

nhunzaker

👍

This triggers our non-reuse mode. This is covered by ReactMount already but the test doesn't pass yet without also landing facebook#10026.

* Warn if unmounting a non-container * Warn if the 2nd+ child is a "root" element but not first This triggers our non-reuse mode. This is covered by ReactMount already but the test doesn't pass yet without also landing #10026.

Warns if there are unknown extra attributes in the hydrated node. It also tries to compare the existing property or attribute against the expected value. It does this by reading the property and comparing it to the prop. Except it's not that simple because multiple prop values can yield the same output. For this we pass an extra expected value that is a hint as to which one was used. This is a bit weird but I'm not sure the alternatives were much better.

This warns if there is ever an insertion or deletion due to hydration failing to find a match. Currently we can't warn for insertions required into the root because that's how we do all non-hydrating renders atm. Left a todo. This strategy is a bit unfortunate that it leads to so much plumbing code. And we have to add three extra methods to the HostConfig that are only used in DEV and not for anything else. I don't really have a better idea.

Textareas are special cases. The initial mount inserts children as the default value, but we should leave that untouched. This is the same as the special case where we set text content of children so I'll use that mechanism.

In Stack this is presented as HTML which needs to have normalized escaping rules. In Fiber it is currently not presented as HTML but a raw string so we don't escape it.

In Fiber, the second warning isn't issued because it's considered an update not a new initial render and we don't fire the warning for those.

In Fiber we don't expect empty strings to be different from rendering null. In fact, in a follow up I plan on formalizing this by never creating text Fibers for empty strings.

We can't just compare the raw innerHTML value because it will have been normalized. Instead, we'll create another element, set its innerHTML and read it back. Since there can be custom elements registered with this document, we want to avoid any side-effects they might cause. So I do this in a fresh new document. I'm not sure how this would affect controlled components and other stuff that could have changed after runtime. I think for those cases maybe we just need a general way of opting out of the diff.

acdlite · 2017-06-29T17:32:25Z

src/renderers/dom/fiber/ReactDOMFiberComponent.js

+    var names = [];
+    attributeNames.forEach(function(name) {
+      names.push(name);
+    });


Interesting, I wouldn't have guessed that forEach was the fastest way to do this. TIL. https://jsperf.com/set-iterator-vs-foreach/4

Just ran the benchmarks from that link locally in node 8.1.3 and the iterator wins there by a tiny bit. I feel like microbenchmarks cannot be trusted in js. Perhaps @bmeurer can chime in?

Unfortunately both are pretty slow in V8 at the moment. The fact that Set.prototype.forEach is faster in some cases isn't intended, and I wouldn't rely on that. We plan to improve the performance of the collection iterators in the future BTW.

My suggestion here: Don't base decisions on current perf results. Use whatever makes sense for your code and try to improve readability instead.

This isn't a perf hot path at all. It only exist in development mode and only fires if something already went wrong.

acdlite

Cool

* Warn for text content * Warn for different attributes/properties values Warns if there are unknown extra attributes in the hydrated node. It also tries to compare the existing property or attribute against the expected value. It does this by reading the property and comparing it to the prop. Except it's not that simple because multiple prop values can yield the same output. For this we pass an extra expected value that is a hint as to which one was used. This is a bit weird but I'm not sure the alternatives were much better. * Warn when there is an insertion or deletion during hydration This warns if there is ever an insertion or deletion due to hydration failing to find a match. Currently we can't warn for insertions required into the root because that's how we do all non-hydrating renders atm. Left a todo. This strategy is a bit unfortunate that it leads to so much plumbing code. And we have to add three extra methods to the HostConfig that are only used in DEV and not for anything else. I don't really have a better idea. * Don't try to delete children of a textarea Textareas are special cases. The initial mount inserts children as the default value, but we should leave that untouched. This is the same as the special case where we set text content of children so I'll use that mechanism. * Change expected format for text differences In Stack this is presented as HTML which needs to have normalized escaping rules. In Fiber it is currently not presented as HTML but a raw string so we don't escape it. * Unmount component in between tests In Fiber, the second warning isn't issued because it's considered an update not a new initial render and we don't fire the warning for those. * Change expectation of white space text behavior in Fiber In Fiber we don't expect empty strings to be different from rendering null. In fact, in a follow up I plan on formalizing this by never creating text Fibers for empty strings. * Warn for different dangerouslySetInnerHTML We can't just compare the raw innerHTML value because it will have been normalized. Instead, we'll create another element, set its innerHTML and read it back. Since there can be custom elements registered with this document, we want to avoid any side-effects they might cause. So I do this in a fresh new document. I'm not sure how this would affect controlled components and other stuff that could have changed after runtime. I think for those cases maybe we just need a general way of opting out of the diff. Warn for different dangerouslySetInnerHTML We can't just compare the raw innerHTML value because it will have been normalized. Instead, we'll create another element, set its innerHTML and read it back. Since there can be custom elements registered with this document, we want to avoid any side-effects they might cause. So I do this in a fresh new document. I'm not sure how this would affect controlled components and other stuff that could have changed after runtime. I think for those cases maybe we just need a general way of opting out of the diff.

sebmarkbage requested review from aickin and gaearon June 22, 2017 22:42

facebook-github-bot added the CLA Signed label Jun 22, 2017

sebmarkbage force-pushed the fixssrwarnings branch 3 times, most recently from df20df1 to 3ada2c9 Compare June 23, 2017 20:43

sebmarkbage commented Jun 23, 2017

View reviewed changes

sebmarkbage force-pushed the fixssrwarnings branch 2 times, most recently from ec6e11d to fe9fcca Compare June 27, 2017 00:46

sebmarkbage requested a review from acdlite June 27, 2017 00:46

sebmarkbage force-pushed the fixssrwarnings branch from 1c1ec1c to 6e97863 Compare June 27, 2017 17:57

sebmarkbage requested a review from sophiebits June 27, 2017 18:00

sebmarkbage force-pushed the fixssrwarnings branch from 6e97863 to a62d031 Compare June 27, 2017 18:21

nhunzaker reviewed Jun 27, 2017

View reviewed changes

nhunzaker approved these changes Jun 27, 2017

View reviewed changes

sebmarkbage added 4 commits June 27, 2017 17:22

Warn for text content

35cafe0

Don't try to delete children of a textarea

124b6eb

Textareas are special cases. The initial mount inserts children as the default value, but we should leave that untouched. This is the same as the special case where we set text content of children so I'll use that mechanism.

sebmarkbage force-pushed the fixssrwarnings branch from 3729ca5 to f9499b9 Compare June 28, 2017 00:38

Change expected format for text differences

b5dd140

In Stack this is presented as HTML which needs to have normalized escaping rules. In Fiber it is currently not presented as HTML but a raw string so we don't escape it.

sebmarkbage force-pushed the fixssrwarnings branch from f9499b9 to b5dd140 Compare June 28, 2017 00:38

sebmarkbage added 3 commits June 27, 2017 17:56

Unmount component in between tests

fd2c59a

In Fiber, the second warning isn't issued because it's considered an update not a new initial render and we don't fire the warning for those.

Change expectation of white space text behavior in Fiber

9b2f6ce

In Fiber we don't expect empty strings to be different from rendering null. In fact, in a follow up I plan on formalizing this by never creating text Fibers for empty strings.

acdlite reviewed Jun 29, 2017

View reviewed changes

acdlite approved these changes Jun 29, 2017

View reviewed changes

sebmarkbage merged commit 8d61138 into facebook:master Jun 30, 2017

sebmarkbage mentioned this pull request Jul 1, 2017

Nicer Formatting of SSR Validation #10085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn When The HTML Mismatches in DEV #10026

Warn When The HTML Mismatches in DEV #10026

sebmarkbage commented Jun 22, 2017 •

edited

Loading

sebmarkbage Jun 23, 2017

sebmarkbage Jun 23, 2017

sebmarkbage commented Jun 23, 2017

sebmarkbage Jun 23, 2017

nhunzaker Jun 27, 2017

sebmarkbage Jun 27, 2017

nhunzaker Jun 27, 2017

nhunzaker Jun 27, 2017

sebmarkbage Jun 27, 2017

acdlite Jun 29, 2017 •

edited

Loading

nhunzaker Jun 29, 2017

nhunzaker left a comment

acdlite Jun 29, 2017

marvinhagemeister Jul 2, 2017

bmeurer Jul 3, 2017

sebmarkbage Jul 3, 2017

acdlite left a comment

Warn When The HTML Mismatches in DEV #10026

Warn When The HTML Mismatches in DEV #10026

Conversation

sebmarkbage commented Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebmarkbage commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

sebmarkbage commented Jun 22, 2017 •

edited

Loading

acdlite Jun 29, 2017 •

edited

Loading