Skip to content

Commit

Permalink
[Partial Hydration] Don't invoke listeners on parent of dehydrated ev…
Browse files Browse the repository at this point in the history
…ent target (#16591)

* Don't invoke listeners on parent of dehydrated event target

* Move Suspense boundary check to getClosestInstanceFromNode

Now getClosestInstanceFromNode can return either a host component,
host text component or suspense component when the suspense
component is dehydrated.

We then use that to ignore events on a suspense component.

* Attach the HostRoot fiber to the DOM container

This lets us detect if an event happens on this root's subtree before it
has rendered something.

* Add todo

The approach of checking isFiberMounted answers if we might be in an
in-progress hydration but it doesn't answer which root or boundary
might be in-progress so we don't know what to wait for.

This needs some refactoring.

* Refactor isFiberMountedImpl to getNearestMountedFiber

We'll need the nearest boundary for event replaying so this prepares for
that.

This surfaced an issue that we attach Hydrating tag on the root but normally
this (and Placement) is attached on the child. This surfaced an issue
that this can lead to both Placement and Hydrating effects which is not
supported so we need to ensure that we only ever use one or the other.

* Add todo for bug I spotted

* Cache tags

* Check the ContainerInstanceKey before the InstanceKey

The container is inside the instance, so we must find it before the
instance, since otherwise we'll miss it.
  • Loading branch information
sebmarkbage authored Sep 5, 2019
1 parent 9ce8711 commit 8d7c733
Show file tree
Hide file tree
Showing 14 changed files with 444 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1895,4 +1895,166 @@ describe('ReactDOMServerPartialHydration', () => {

document.body.removeChild(container);
});

it('does not invoke the parent of dehydrated boundary event', async () => {
let suspend = false;
let resolve;
let promise = new Promise(resolvePromise => (resolve = resolvePromise));

let clicksOnParent = 0;
let clicksOnChild = 0;

function Child({text}) {
if (suspend) {
throw promise;
} else {
return (
<span
onClick={e => {
// The stopPropagation is showing an example why invoking
// the event on only a parent might not be correct.
e.stopPropagation();
clicksOnChild++;
}}>
Hello
</span>
);
}
}

function App() {
return (
<div onClick={() => clicksOnParent++}>
<Suspense fallback="Loading...">
<Child />
</Suspense>
</div>
);
}

suspend = false;
let finalHTML = ReactDOMServer.renderToString(<App />);
let container = document.createElement('div');
container.innerHTML = finalHTML;

// We need this to be in the document since we'll dispatch events on it.
document.body.appendChild(container);

let span = container.getElementsByTagName('span')[0];

// On the client we don't have all data yet but we want to start
// hydrating anyway.
suspend = true;
let root = ReactDOM.unstable_createRoot(container, {hydrate: true});
root.render(<App />);
Scheduler.unstable_flushAll();
jest.runAllTimers();

// We're now partially hydrated.
span.click();
expect(clicksOnChild).toBe(0);
expect(clicksOnParent).toBe(0);

// Resolving the promise so that rendering can complete.
suspend = false;
resolve();
await promise;

Scheduler.unstable_flushAll();
jest.runAllTimers();

// TODO: With selective hydration the event should've been replayed
// but for now we'll have to issue it again.
act(() => {
span.click();
});

expect(clicksOnChild).toBe(1);
// This will be zero due to the stopPropagation.
expect(clicksOnParent).toBe(0);

document.body.removeChild(container);
});

it('does not invoke an event on a parent tree when a subtree is dehydrated', async () => {
let suspend = false;
let resolve;
let promise = new Promise(resolvePromise => (resolve = resolvePromise));

let clicks = 0;
let childSlotRef = React.createRef();

function Parent() {
return <div onClick={() => clicks++} ref={childSlotRef} />;
}

function Child({text}) {
if (suspend) {
throw promise;
} else {
return <a>Click me</a>;
}
}

function App() {
// The root is a Suspense boundary.
return (
<Suspense fallback="Loading...">
<Child />
</Suspense>
);
}

suspend = false;
let finalHTML = ReactDOMServer.renderToString(<App />);

let parentContainer = document.createElement('div');
let childContainer = document.createElement('div');

// We need this to be in the document since we'll dispatch events on it.
document.body.appendChild(parentContainer);

// We're going to use a different root as a parent.
// This lets us detect whether an event goes through React's event system.
let parentRoot = ReactDOM.unstable_createRoot(parentContainer);
parentRoot.render(<Parent />);
Scheduler.unstable_flushAll();

childSlotRef.current.appendChild(childContainer);

childContainer.innerHTML = finalHTML;

let a = childContainer.getElementsByTagName('a')[0];

suspend = true;

// Hydrate asynchronously.
let root = ReactDOM.unstable_createRoot(childContainer, {hydrate: true});
root.render(<App />);
jest.runAllTimers();
Scheduler.unstable_flushAll();

// The Suspense boundary is not yet hydrated.
a.click();
expect(clicks).toBe(0);

// Resolving the promise so that rendering can complete.
suspend = false;
resolve();
await promise;

Scheduler.unstable_flushAll();
jest.runAllTimers();

// We're now full hydrated.
// TODO: With selective hydration the event should've been replayed
// but for now we'll have to issue it again.
act(() => {
a.click();
});

expect(clicks).toBe(1);

document.body.removeChild(parentContainer);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -586,4 +586,62 @@ describe('ReactDOMServerHydration', () => {

document.body.removeChild(container);
});

it('does not invoke an event on a parent tree when a subtree is hydrating', () => {
let clicks = 0;
let childSlotRef = React.createRef();

function Parent() {
return <div onClick={() => clicks++} ref={childSlotRef} />;
}

function App() {
return (
<div>
<a>Click me</a>
</div>
);
}

let finalHTML = ReactDOMServer.renderToString(<App />);

let parentContainer = document.createElement('div');
let childContainer = document.createElement('div');

// We need this to be in the document since we'll dispatch events on it.
document.body.appendChild(parentContainer);

// We're going to use a different root as a parent.
// This lets us detect whether an event goes through React's event system.
let parentRoot = ReactDOM.unstable_createRoot(parentContainer);
parentRoot.render(<Parent />);
Scheduler.unstable_flushAll();

childSlotRef.current.appendChild(childContainer);

childContainer.innerHTML = finalHTML;

let a = childContainer.getElementsByTagName('a')[0];

// Hydrate asynchronously.
let root = ReactDOM.unstable_createRoot(childContainer, {hydrate: true});
root.render(<App />);
// Nothing has rendered so far.

a.click();
expect(clicks).toBe(0);

Scheduler.unstable_flushAll();

// We're now full hydrated.
// TODO: With selective hydration the event should've been replayed
// but for now we'll have to issue it again.
act(() => {
a.click();
});

expect(clicks).toBe(1);

document.body.removeChild(parentContainer);
});
});
3 changes: 3 additions & 0 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import {
getNodeFromInstance,
getFiberCurrentPropsFromNode,
getClosestInstanceFromNode,
markContainerAsRoot,
} from './ReactDOMComponentTree';
import {restoreControlledState} from './ReactDOMComponent';
import {dispatchEvent} from '../events/ReactDOMEventListener';
Expand Down Expand Up @@ -375,6 +376,7 @@ function ReactSyncRoot(
(options != null && options.hydrationOptions) || null;
const root = createContainer(container, tag, hydrate, hydrationCallbacks);
this._internalRoot = root;
markContainerAsRoot(root.current, container);
}

function ReactRoot(container: DOMContainer, options: void | RootOptions) {
Expand All @@ -388,6 +390,7 @@ function ReactRoot(container: DOMContainer, options: void | RootOptions) {
hydrationCallbacks,
);
this._internalRoot = root;
markContainerAsRoot(root.current, container);
}

ReactRoot.prototype.render = ReactSyncRoot.prototype.render = function(
Expand Down
98 changes: 74 additions & 24 deletions packages/react-dom/src/client/ReactDOMComponentTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,43 +8,93 @@
import {HostComponent, HostText} from 'shared/ReactWorkTags';
import invariant from 'shared/invariant';

import {getParentSuspenseInstance} from './ReactDOMHostConfig';

const randomKey = Math.random()
.toString(36)
.slice(2);
const internalInstanceKey = '__reactInternalInstance$' + randomKey;
const internalEventHandlersKey = '__reactEventHandlers$' + randomKey;
const internalContainerInstanceKey = '__reactContainere$' + randomKey;

export function precacheFiberNode(hostInst, node) {
node[internalInstanceKey] = hostInst;
}

/**
* Given a DOM node, return the closest ReactDOMComponent or
* ReactDOMTextComponent instance ancestor.
*/
export function getClosestInstanceFromNode(node) {
let inst = node[internalInstanceKey];
if (inst) {
return inst;
}
export function markContainerAsRoot(hostRoot, node) {
node[internalContainerInstanceKey] = hostRoot;
}

do {
node = node.parentNode;
if (node) {
inst = node[internalInstanceKey];
} else {
// Top of the tree. This node must not be part of a React tree (or is
// unmounted, potentially).
return null;
// Given a DOM node, return the closest HostComponent or HostText fiber ancestor.
// If the target node is part of a hydrated or not yet rendered subtree, then
// this may also return a SuspenseComponent or HostRoot to indicate that.
// Conceptually the HostRoot fiber is a child of the Container node. So if you
// pass the Container node as the targetNode, you wiill not actually get the
// HostRoot back. To get to the HostRoot, you need to pass a child of it.
// The same thing applies to Suspense boundaries.
export function getClosestInstanceFromNode(targetNode) {
let targetInst = targetNode[internalInstanceKey];
if (targetInst) {
// Don't return HostRoot or SuspenseComponent here.
return targetInst;
}
// If the direct event target isn't a React owned DOM node, we need to look
// to see if one of its parents is a React owned DOM node.
let parentNode = targetNode.parentNode;
while (parentNode) {
// We'll check if this is a container root that could include
// React nodes in the future. We need to check this first because
// if we're a child of a dehydrated container, we need to first
// find that inner container before moving on to finding the parent
// instance. Note that we don't check this field on the targetNode
// itself because the fibers are conceptually between the container
// node and the first child. It isn't surrounding the container node.
targetInst = parentNode[internalContainerInstanceKey];
if (targetInst) {
// If so, we return the HostRoot Fiber.
return targetInst;
}
} while (!inst);
targetInst = parentNode[internalInstanceKey];
if (targetInst) {
// Since this wasn't the direct target of the event, we might have
// stepped past dehydrated DOM nodes to get here. However they could
// also have been non-React nodes. We need to answer which one.

let tag = inst.tag;
switch (tag) {
case HostComponent:
case HostText:
// In Fiber, this will always be the deepest root.
return inst;
// If we the instance doesn't have any children, then there can't be
// a nested suspense boundary within it. So we can use this as a fast
// bailout. Most of the time, when people add non-React children to
// the tree, it is using a ref to a child-less DOM node.
// We only need to check one of the fibers because if it has ever
// gone from having children to deleting them or vice versa it would
// have deleted the dehydrated boundary nested inside already.
if (targetInst.child !== null) {
// Next we need to figure out if the node that skipped past is
// nested within a dehydrated boundary and if so, which one.
let suspenseInstance = getParentSuspenseInstance(targetNode);
if (suspenseInstance !== null) {
// We found a suspense instance. That means that we haven't
// hydrated it yet. Even though we leave the comments in the
// DOM after hydrating, and there are boundaries in the DOM
// that could already be hydrated, we wouldn't have found them
// through this pass since if the target is hydrated it would
// have had an internalInstanceKey on it.
// Let's get the fiber associated with the SuspenseComponent
// as the deepest instance.
let targetSuspenseInst = suspenseInstance[internalInstanceKey];
if (targetSuspenseInst) {
return targetSuspenseInst;
}
// If we don't find a Fiber on the comment, it might be because
// we haven't gotten to hydrate it yet. That should mean that
// the parent component also hasn't hydrated yet but we can
// just return that since it will bail out on the isMounted
// check.
}
}
return targetInst;
}
targetNode = parentNode;
parentNode = targetNode.parentNode;
}
return null;
}
Expand Down
Loading

0 comments on commit 8d7c733

Please sign in to comment.