Don't hydrate any properties other than event listeners and text content by sebmarkbage · Pull Request #9858 · facebook/react

sebmarkbage · 2017-06-06T03:35:10Z

This strategy assumes that the rendered HTML is correct if the tree lines up. Therefore we don't diff any attributes of the rendered HTML.

However, as a precaution I ensure that textContent is updated. This ensures that if something goes wrong with keys lining up etc. at least there is some feedback that the event handlers might not line up. With what you expect. Attributes and styles etc might cause a slightly broken look but that's because the app is considered broken in this state. The textContent updates is just a precaution against the worst things that can happen (e.g. clicking delete on a row with an event listener that doesn't match the row content).

This might not be what you want e.g. for date formatting where it can different between server and client though. Since that can cause janky layout and repaint.

It is expected that content will line up. To ensure that I will in a follow up ensure that the warning is issued if it doesn't line up so that in development this can be addressed.

The text content updates are now moved to the commit phase so if the tree is asynchronously hydrated it doesn't start partially swapping out. I use the regular update side-effect with payload if the content doesn't match up.

Since we no longer guarantee that attributes is correct I changed the bad mark up SSR integration tests to only assert on the textContent instead.

sebmarkbage · 2017-06-06T03:35:33Z

-* should not blow away user-entered text on successful reconnect to a controlled checkbox
-* should not blow away user-selected value on successful reconnect to an uncontrolled select
-* should not blow away user-selected value on successful reconnect to an controlled select
+


trueadm

LGTM.

I also think it would be good to add a fixture to show hydration working. In my experience, I've had so many issues with hydration bugs that it would be good to track this in fixtures for future releases.

sebmarkbage · 2017-06-06T17:17:29Z

We do have a fixture but it doesn't have much stuff in it. I'd love to have more forms and stuff. I'll add an issue for it. #9866

aickin

Nice job! And super-w00t for 0 SSR tests failing!

Both of my suggested changes are optional.

Other than those two comments, LGTM, although I readily admit I still don't really understand all the Fiber internal code, so my opinion should probably be ignored there. For the part I do know (the SSR test code I wrote), nice job!

aickin · 2017-06-06T20:21:11Z

+            updatePayload = [CHILDREN, nextProp];
+          }
+        } else if (typeof nextProp === 'number') {
+          if (domElement.textContent !== '' + nextProp) {


The use of textContent here is interesting. I'd argue that innerHTML may be more technically correct, as textContent will ignore child elements in the existing DOM tree.

For example, for the JSX <div>My Content</div>, textContent would be fine hydrating <div>My <span style="color: blue">Content</span><input type='text' value='some input value'></div>, whereas innerHTML would not.

You are deliberately not checking everything in the tree, though, and the documentation for textContent on MDN says that it's tends to perform better than innerHTML, so maybe this is intentional? If so, I think a comment is in order.

innerHTML would be difficult to compare to the string since it will contain escape characters etc. instead of the raw original text encoding representation.

This is interesting though. Since the goal here isn't to preserve the correct document structure and attributes etc, but actually only to preserve text content in terms of how they might line up to critical event handlers this still satisfies that requirement. Even if the text content doesn't fully line up.

I'll add a comment.

Cool.

One other thought I just had: what about when children is an array of text or an array of numbers (or an array of arrays of strings, etc.)? I think this code will fail to fix up the text in that case, right?

That will go through another pass in Fiber that deals with HostText. This particular pass is only an "optimization" that avoids extra Fibers etc.

hydrateTextInstance will return true if it diffs in those cases. That will schedule a commitTextUpdate to patch it up. (I'm going to move this into a helper in ReactDOMFiberComponent in a follow up diff that deals with warnings.)

And hence my comment about still not understanding the Fiber internals. Thanks for your patience; I'll be quiet now. 😁

aickin · 2017-06-06T20:36:15Z

+  // First we render the top of bad mark up.
  var domElement = document.createElement('div');
  domElement.innerHTML =
    '<div id="badIdWhichWillCauseMismatch" data-reactroot="" data-reactid="1"></div>';


I'd argue that it's probably a good idea to put in some junk text here to make sure that the patch up occurs. It's not impossible to assume that some tests try rendering <div someprop='some value'/> (either already or in the future), and that wouldn't trigger the patch up code at all.

There is already other tests in other places in the test suite that covers this so I think it's mostly fine. What we don't have is a test that ensures that the text node is preserved (which I missed in the original PR). However, I think that's more of an optimization than correctness.

I thought at one point about having the test in the hydrating "good markup" case actually walk the entire DOM tree before and after rendering to confirm that the nodes were not replaced by hydration. Would that be helpful as a PR?

I didn't want to be too prescriptive but if it is a completely good tree that's probably pretty inherent to the whole model. So, yes, that would be great.

It turns out I can't put junk text in there because <div dangerouslySetInnerHTML={...} /> won't actually hit the patch up path so it fails on bad markup. However, I think we're fine not patching up the tree in that case since comparing would be difficult and expensive.

aickin · 2017-06-06T20:43:22Z

(Oh, and I'm not really used to GitHub's review API, and I wasn't sure given my comments whether I should have clicked "Request Changes" or "Approve". Please correct me if I chose wrong.)

aweary · 2017-06-06T20:52:20Z

@aickin I think "Request Changes" is usually used for blocking change requests. You can use "Approve" when giving a LGTM with optional feedback.

acdlite · 2017-06-06T21:15:48Z

    childToDelete.effectTag = Deletion;

+    // TODO(acdlite): Do we need to add this to the progressed deletions list?
+    // I don't think so and it's generally too late to do now.


Could you add a comment here with the explanation you gave to me in person?

aickin · 2017-06-06T21:20:10Z

@aweary Thanks for the explanation; I updated my review to "Approve".

This strategy assumes that the rendered HTML is correct if the tree lines up. Therefore we don't diff any attributes of the rendered HTML. However, as a precaution I ensure that textContent *is* updated. This ensures that if something goes wrong with keys lining up etc. at least there is some feedback that the event handlers might not line up. With what you expect. This might not be what you want e.g. for date formatting where it can different between server and client. It is expected that content will line up. To ensure that I will in a follow up ensure that the warning is issued if it doesn't line up so that in development this can be addressed. The text content updates are now moved to the commit phase so if the tree is asynchronously hydrated it doesn't start partially swapping out. I use the regular update side-effect with payload if the content doesn't match up. Since we no longer guarantee that attributes is correct I changed the bad mark up SSR integration tests to only assert on the textContent instead.

Currently we're never matching text nodes so we need to properly branch.

sebmarkbage requested review from acdlite, aickin, sophiebits and trueadm June 6, 2017 03:35

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

sebmarkbage commented Jun 6, 2017

View reviewed changes

sebmarkbage force-pushed the fixhydration branch 2 times, most recently from 1cd5bb3 to a6cc5e0 Compare June 6, 2017 04:15

trueadm approved these changes Jun 6, 2017

View reviewed changes

aickin suggested changes Jun 6, 2017

View reviewed changes

acdlite approved these changes Jun 6, 2017

View reviewed changes

aickin approved these changes Jun 6, 2017

View reviewed changes

sebmarkbage force-pushed the fixhydration branch from 3914fa1 to 2a18dfb Compare June 6, 2017 21:43

Hydrate text node if possible

ae018bf

Currently we're never matching text nodes so we need to properly branch.

sebmarkbage force-pushed the fixhydration branch from 2a18dfb to ae018bf Compare June 6, 2017 22:09

sebmarkbage merged commit 262b9c7 into facebook:master Jun 6, 2017

Conversation

sebmarkbage commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueadm left a comment

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aickin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aickin Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aickin Jun 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aickin commented Jun 6, 2017

Uh oh!

aweary commented Jun 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aickin commented Jun 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sebmarkbage commented Jun 6, 2017 •

edited

Loading

sebmarkbage commented Jun 6, 2017 •

edited

Loading

aickin Jun 6, 2017 •

edited

Loading

sebmarkbage Jun 6, 2017 •

edited

Loading

aickin Jun 6, 2017 •

edited

Loading