Hydration of previously rendered server markup #9580

sebmarkbage · 2017-05-02T04:41:17Z

This strategy keeps track of any available DOM nodes ("host instances") that we can reuse. They're stored in ReactFiberHydrationContext as we're reconciling and then picked off if they match. Any remaining non-matching nodes are deleted, others are inserted.

This is using the normal reconciliation model so it should be possible to run in async mode so that reviving doesn't block the main thread. Currently, there are some properties that are set eagerly and events gets wired up early so that will break in async mode. I'll leave that as a todo.

This detects whether to revive or clear the container based on whether the child has a data-reactid attribute. We might want to change this API to be explicit.

iamdustan · 2017-05-02T14:09:23Z

src/renderers/dom/fiber/ReactDOMFiber.js

+    // TODO: Possibly defer this until the commit phase where all the events
+    // get attached.
+    updateFiberProps(instance, props);
+    setInitialProperties(instance, type, props, rootContainerInstance);


Will this be able to solve reviving input values (#7745) (whether now or after 16) or is solving that in ReactDOM core a non-goal?

That's a non-goal. We might start warning about server-rendering controlled components.

We could potentially always fire a change event after reviving with the new value.

However, maybe it's better to model it as uncontrolled first and then switch to controlled as you're reviving with the knowledge that you'll have to pick up the current value and do something useful with it.

The first solution is certainly more convenient, but is that a good user experience? It seems to me that most use cases for this would suffer in some way unless you designed both experiences.

Although I guess doing both wouldn't hurt.

I don’t want to conflate this issue too deeply with this tangent knowing it’s a non-goal so let me know if it makes sense to have a tracking issue for thoughts/best practices (whether that turns into implementation or documentation in the form section)

We could potentially always fire a change event after reviving with the new value.
It seems to me that most use cases for this would suffer in some way unless you designed both experiences.

I’m curious what use cases would suffer or hurt the user experience with this? I’ve experienced this scenario from browser prefilling forms from autocomplete setting and resetting the value during a refresh.

In both scenarios having it modeled as an onmount setState would’ve been sufficient to hydrate the host environment values back to React component state.

sebmarkbage · 2017-05-15T22:22:37Z

scripts/fiber/tests-failing.txt

-* renders a controlled checkbox with client render on top of good server markup
-* renders a controlled select with client render on top of good server markup
-* should not blow away user-entered text on successful reconnect to an uncontrolled input
-* should not blow away user-entered text on successful reconnect to a controlled input
 * should not blow away user-entered text on successful reconnect to an uncontrolled checkbox
 * 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


These tests fail because currently I'm going through the setInitialProperties path which tries to set all the properties. This doesn't affect text inputs because there we use setAttribute('value', ...) but ideally we should always set properties so that this gets properly reset to what we think it is. But we should be explicit about we think the value should be. Either the current value or the defaultValue. I'd like to follow up on that in the next separate PR.

sebmarkbage · 2017-05-16T02:22:28Z

I think this is ready to land.

I'll fix the mentioned issues about controlled checkboxes/select later.

After the server-side lands I might change the heuristic that determines what to clear in head/body as well as start skipping comments from being considered for hydration. Since I can then change the output.

Additionally, there are a few async related issues that I'll leave for later:

How to revive things inside of "hidden" down-prioritized tree? Needs to create a lazy revival where hydration state gets entered when continuing the subtree. Currently it will clear out the children and not hydrate. Maybe that's for the best. Should these render at all in the server markup?
How to delay events from firing on partially revived trees? Currently events can start firing too soon. I have a few ideas but this might also be related to the <img onLoad={...} /> problem.
setInitialProperties are currently called during reconciliation. In an async hydration that means that some patches can start becoming visible before others. Ideally we shouldn't even need to call it since everything should already be in the state we expected.

sebmarkbage · 2017-05-18T00:07:49Z

Rebased on top #9710

acdlite · 2017-05-19T09:58:45Z

src/renderers/shared/fiber/ReactFiber.js

@@ -433,6 +433,12 @@ function createFiberFromElementType(

 exports.createFiberFromElementType = createFiberFromElementType;

+exports.createFiberFromHostInstanceForDeletion = function(): Fiber {
+  const fiber = createFiber(HostComponent, null, NoContext);
+  fiber.type = 'DELETED';


Any reason we don't pass the node to this function? Need to assign it to stateNode anyway.

Otherwise I don't understand the value of this function versus calling createFiber directly.

createFiber is not exposed at all. Makes it easier to add or change the default inputs here.

stateNode is not passed when other HostComponent Fibers are created so I figured we'd be consistent there.

The warnings need to check a valid container but that should happen in unstable_renderSubtreeIntoContainer too so might as well move it in.

Hydrates a server-rendered DOM tree by traversing it and connecting up the nodes if they match. Attributes are not diffed. They're assumed to be accurate on matching nodes.

Because the current server renderer includes the meta data, it remains in a revived tree.

During initial mount, we should not track Placement

TODO make these warnings instead.

When we don't match the first hydration node, we'll do an insertion. Currently we keep the next hydratable sibling so that we know where to pick up once we're done with the insertion. Unfortunately this makes the nodes inside the insertion think that there's a node to hydrate. I used to check for the direct parent but that doesn't work on nested host components. We need to instead keep track of that we're in an hydration context but we're not currently hydrating. Once we pop passed the inserted node can we resume hydrating.

isMounted checks whether a component is inside a Placement. During hydration we ideally don't do any Placements if hydration matches the tree. To work around this I use the Placement flag on the root, which isn't used for anything else. But only temporarily while we're hydrating. Then reset it before committing.

gaearon · 2017-08-01T14:14:51Z

src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js

-        'ReactDOMServer.renderToString() for server rendering.',
-    );
+    if (ReactDOMFeatureFlags.useFiber) {
+      ReactDOM.render(<Component text="Hello world" />, testDocument);


Is this intentional? Do we need to rename the test? It says "should throw".

I'm not sure those cross-browser quirks are still in modern browsers. I didn't find any issues (that aren't also issues for SSR).

I'm not sure how restrictive we want to be?

bvaughn · 2020-07-01T15:52:30Z

src/renderers/dom/shared/__tests__/ReactRenderDocument-test.js


    ReactDOM.render(<Component />, testDocument);

    expect(testDocument.body.innerHTML).toBe('Hello world');

    // Reactive update
-    expect(function() {
+    if (ReactDOMFeatureFlags.useFiber) {
+      // This works but is probably a bad idea.


Don't suppose you remember what you meant by this comment, @sebmarkbage?

See UNMOUNT_INVARIANT_MESSAGE at the top of the file. Browsers don't like when you unmount the html host node, or at least some didn't when we added that invariant to Stack.

hey Sophie 👋

Thanks! I misread Seb's comment as "this works but isn't a good way to test" rather than "this works but we don't encourage it in production"

I follow now

sebmarkbage requested review from sophiebits, aickin and acdlite May 2, 2017 04:41

facebook-github-bot added the CLA Signed label May 2, 2017

sebmarkbage force-pushed the ssrreviving branch 2 times, most recently from ab81299 to 693e65c Compare May 2, 2017 05:24

iamdustan reviewed May 2, 2017

View reviewed changes

sebmarkbage force-pushed the ssrreviving branch from 693e65c to 8cb9cdb Compare May 2, 2017 15:52

sebmarkbage force-pushed the ssrreviving branch 2 times, most recently from f73058d to 5d13239 Compare May 13, 2017 01:11

sebmarkbage commented May 15, 2017

View reviewed changes

sebmarkbage changed the title ~~[WIP] Hydration of previously rendered server markup~~ Hydration of previously rendered server markup May 16, 2017

sebmarkbage force-pushed the ssrreviving branch from 5d13239 to bfec75a Compare May 16, 2017 02:16

acdlite mentioned this pull request May 16, 2017

Refactor progressed work model #9695

Closed

21 tasks

sebmarkbage mentioned this pull request May 17, 2017

Add package builds for new server renderer and enable tests #9710

Merged

sebmarkbage force-pushed the ssrreviving branch from bfec75a to 6522b9e Compare May 18, 2017 00:07

sebmarkbage force-pushed the ssrreviving branch from 6522b9e to 6cbc601 Compare May 18, 2017 00:22

acdlite approved these changes May 19, 2017

View reviewed changes

acdlite mentioned this pull request May 23, 2017

[Umbrella] Async rendering #8830

Closed

17 tasks

sebmarkbage added 9 commits May 25, 2017 11:58

Don't double validate the DOM container

ebd27f5

The warnings need to check a valid container but that should happen in unstable_renderSubtreeIntoContainer too so might as well move it in.

Hydrating DOM

d942047

Hydrates a server-rendered DOM tree by traversing it and connecting up the nodes if they match. Attributes are not diffed. They're assumed to be accurate on matching nodes.

Remove meta data filtering from test

c534c77

Because the current server renderer includes the meta data, it remains in a revived tree.

Annotate because Flow

8917ec8

Don't track insertion effects if we're going to hydrate

b73af63

During initial mount, we should not track Placement

Fix up test cases to ignore errors that we no longer throw

8c0534b

TODO make these warnings instead.

Comments

ac2a8d3

Record tests

83e5b57

sebmarkbage force-pushed the ssrreviving branch from 6cbc601 to 83e5b57 Compare May 25, 2017 19:01

sebmarkbage merged commit 9c7a312 into facebook:master May 25, 2017

gaearon mentioned this pull request Jun 7, 2017

Fix a bug with memoizedState affecting React DevTools #9886

Merged

gaearon reviewed Aug 1, 2017

View reviewed changes

bvaughn reviewed Jul 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hydration of previously rendered server markup #9580

Hydration of previously rendered server markup #9580

sebmarkbage commented May 2, 2017

iamdustan May 2, 2017

sebmarkbage May 2, 2017

sebmarkbage May 2, 2017

iamdustan May 3, 2017

sebmarkbage May 15, 2017

sebmarkbage commented May 16, 2017

sebmarkbage commented May 18, 2017

acdlite May 19, 2017

sebmarkbage May 25, 2017

gaearon Aug 1, 2017

sebmarkbage Aug 1, 2017

bvaughn Jul 1, 2020

sophiebits Jul 1, 2020

bvaughn Jul 1, 2020

Hydration of previously rendered server markup #9580

Hydration of previously rendered server markup #9580

Conversation

sebmarkbage commented May 2, 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

sebmarkbage commented May 16, 2017

sebmarkbage commented May 18, 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

Choose a reason for hiding this comment