Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Contributor

@trueadm trueadm Sep 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do wonder if its still the best strategy to keep adding additional properties to DOM nodes vs a WeakMap with an object that contains { instance, container, props }. We could probably unify this with the already existing WeakMap we have on DOM nodes for checking for registered events:

https://github.com/facebook/react/blob/master/packages/react-dom/src/events/ReactBrowserEventEmitter.js#L89

This isn't really something for this PR, just an observation.

Update: After looking at this jsperf it might be better if we should stick with a hidden key, but rather than having 3, have a single one and remove the WeakMap and use an object with 4 properties.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one for props is an unfortunate one. That was never intentional. I just realized too late I didn't have a convenient place to find the current Fiber. I don't remember exactly why I couldn't just update the fiber pointer to the correct alternate every time events change and then read from memoizedProps. There's probably something we can do to get rid of that one.

Regarding the new container one. It's very rare that something will both be a container and an instance so there will almost never be more than one expando. So for that case it's probably not worth adding an indirection for the very common case just to avoid two slots in the rare case.

}

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