From 1a62e567e4e865e5153d1961ba35ab163dbff412 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2019 13:07:02 -0700 Subject: [PATCH 1/8] Don't invoke listeners on parent of dehydrated event target --- ...DOMServerPartialHydration-test.internal.js | 80 +++++++++++++++++++ .../src/client/ReactDOMHostConfig.js | 33 ++++++++ .../src/events/ReactDOMEventListener.js | 59 ++++++++++++-- 3 files changed, 165 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index d31bf6cfc9a..7f76fa403a7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -1895,4 +1895,84 @@ 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 ( + { + // The stopPropagation is showing an example why invoking + // the event on only a parent might not be correct. + e.stopPropagation(); + clicksOnChild++; + }}> + Hello + + ); + } + } + + function App() { + return ( +
clicksOnParent++}> + + + +
+ ); + } + + suspend = false; + let finalHTML = ReactDOMServer.renderToString(); + 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(); + 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); + }); }); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 6b95e63ac9d..3c52988a872 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -704,6 +704,39 @@ export function getNextHydratableInstanceAfterSuspenseInstance( return null; } +// Returns the SuspenseInstance if this node is a direct child of a +// SuspenseInstance. I.e. if its previous sibling is a Comment with +// SUSPENSE_x_START_DATA. Otherwise, null. +export function getParentSuspenseInstance( + targetInstance: Instance, +): null | SuspenseInstance { + let node = targetInstance.previousSibling; + // Skip past all nodes within this suspense boundary. + // There might be nested nodes so we need to keep track of how + // deep we are and only break out when we're back on top. + let depth = 0; + while (node) { + if (node.nodeType === COMMENT_NODE) { + let data = ((node: any).data: string); + if ( + data === SUSPENSE_START_DATA || + data === SUSPENSE_FALLBACK_START_DATA || + data === SUSPENSE_PENDING_START_DATA + ) { + if (depth === 0) { + return ((node: any): SuspenseInstance); + } else { + depth--; + } + } else if (data === SUSPENSE_END_DATA) { + depth++; + } + } + node = node.previousSibling; + } + return null; +} + export function didNotMatchHydratedContainerTextInstance( parentContainer: Container, textInstance: TextInstance, diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 0c8a3f99490..e5778ee54d2 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -39,10 +39,14 @@ import { addEventCaptureListenerWithPassiveFlag, } from './EventListener'; import getEventTarget from './getEventTarget'; -import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree'; +import { + getClosestInstanceFromNode, + getInstanceFromNode, +} from '../client/ReactDOMComponentTree'; import SimpleEventPlugin from './SimpleEventPlugin'; import {getRawEventName} from './DOMTopLevelEventTypes'; import {passiveBrowserEventsSupported} from './checkPassiveEvents'; +import {getParentSuspenseInstance} from '../client/ReactDOMHostConfig'; import { enableFlareAPI, @@ -309,13 +313,54 @@ export function dispatchEvent( return; } const nativeEventTarget = getEventTarget(nativeEvent); - let targetInst = getClosestInstanceFromNode(nativeEventTarget); - if ( - targetInst !== null && - typeof targetInst.tag === 'number' && - !isFiberMounted(targetInst) - ) { + // This is effectively an inlined version of getClosestInstanceFromNode with + // some special semantics for dehydrated Suspense boundaries. + let targetInst = getInstanceFromNode(nativeEventTarget); + if (targetInst === null) { + // 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 targetNode = nativeEventTarget; + let parentNode = targetNode.parentNode; + while (parentNode) { + targetInst = getInstanceFromNode(parentNode); + if (targetInst !== null) { + // 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. + + // 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. + // We're going to proceed as if there was no target yet. + // TODO: This is a good opportunity to schedule a replay of + // the event instead once this boundary has been hydrated. + targetInst = null; + } + } + break; + } + targetNode = parentNode; + parentNode = targetNode.parent; + } + } + + if (targetInst !== null && !isFiberMounted(targetInst)) { // If we get an event (ex: img onload) before committing that // component's mount, ignore it for now (that is, treat it as if it was an // event on a non-React tree). We might also consider queueing events and From 1f77c91e33e610e51032ad025876fe32c3f1a63e Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2019 18:32:30 -0700 Subject: [PATCH 2/8] 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. --- .../src/client/ReactDOMComponentTree.js | 69 ++++++++++++----- .../src/client/ReactDOMHostConfig.js | 7 ++ .../src/events/ReactDOMEventListener.js | 75 +++++-------------- .../src/ReactFiberCompleteWork.js | 2 + .../src/ReactFiberHydrationContext.js | 22 ++++++ .../src/forks/ReactFiberHostConfig.custom.js | 1 + packages/shared/HostConfigWithNoHydration.js | 1 + scripts/error-codes/codes.json | 3 +- 8 files changed, 102 insertions(+), 78 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index 8d3105ae443..c90c43b5a66 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -8,6 +8,8 @@ import {HostComponent, HostText} from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; +import {getParentSuspenseInstance} from './ReactDOMHostConfig'; + const randomKey = Math.random() .toString(36) .slice(2); @@ -22,29 +24,56 @@ export function precacheFiberNode(hostInst, node) { * 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 getClosestInstanceFromNode(targetNode) { + let targetInst = targetNode[internalInstanceKey]; + if (targetInst) { + 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) { + 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. - 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; + // 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; } - } while (!inst); - - let tag = inst.tag; - switch (tag) { - case HostComponent: - case HostText: - // In Fiber, this will always be the deepest root. - return inst; + targetNode = parentNode; + parentNode = targetNode.parentNode; } return null; } diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 3c52988a872..1f7604eb605 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -673,6 +673,13 @@ export function hydrateTextInstance( return diffHydratedText(textInstance, text); } +export function hydrateSuspenseInstance( + suspenseInstance: SuspenseInstance, + internalInstanceHandle: Object, +) { + precacheFiberNode(internalInstanceHandle, suspenseInstance); +} + export function getNextHydratableInstanceAfterSuspenseInstance( suspenseInstance: SuspenseInstance, ): null | HydratableInstance { diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index e5778ee54d2..2afb35aa0c5 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -23,7 +23,7 @@ import { import {runExtractedPluginEventsInBatch} from 'legacy-events/EventPluginHub'; import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem'; import {isFiberMounted} from 'react-reconciler/reflection'; -import {HostRoot} from 'shared/ReactWorkTags'; +import {HostRoot, SuspenseComponent} from 'shared/ReactWorkTags'; import { type EventSystemFlags, PLUGIN_EVENT_SYSTEM, @@ -39,14 +39,10 @@ import { addEventCaptureListenerWithPassiveFlag, } from './EventListener'; import getEventTarget from './getEventTarget'; -import { - getClosestInstanceFromNode, - getInstanceFromNode, -} from '../client/ReactDOMComponentTree'; +import {getClosestInstanceFromNode} from '../client/ReactDOMComponentTree'; import SimpleEventPlugin from './SimpleEventPlugin'; import {getRawEventName} from './DOMTopLevelEventTypes'; import {passiveBrowserEventsSupported} from './checkPassiveEvents'; -import {getParentSuspenseInstance} from '../client/ReactDOMHostConfig'; import { enableFlareAPI, @@ -313,61 +309,26 @@ export function dispatchEvent( return; } const nativeEventTarget = getEventTarget(nativeEvent); - - // This is effectively an inlined version of getClosestInstanceFromNode with - // some special semantics for dehydrated Suspense boundaries. - let targetInst = getInstanceFromNode(nativeEventTarget); - if (targetInst === null) { - // 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 targetNode = nativeEventTarget; - let parentNode = targetNode.parentNode; - while (parentNode) { - targetInst = getInstanceFromNode(parentNode); - if (targetInst !== null) { - // 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. - - // 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. - // We're going to proceed as if there was no target yet. - // TODO: This is a good opportunity to schedule a replay of - // the event instead once this boundary has been hydrated. - targetInst = null; - } - } - break; + let targetInst = getClosestInstanceFromNode(nativeEventTarget); + + if (targetInst !== null) { + if (isFiberMounted(targetInst)) { + if (targetInst.tag === SuspenseComponent) { + // TODO: This is a good opportunity to schedule a replay of + // the event instead once this boundary has been hydrated. + // For now we're going to just ignore this event as if it's + // not mounted. + targetInst = null; } - targetNode = parentNode; - parentNode = targetNode.parent; + } else { + // If we get an event (ex: img onload) before committing that + // component's mount, ignore it for now (that is, treat it as if it was an + // event on a non-React tree). We might also consider queueing events and + // dispatching them after the mount. + targetInst = null; } } - if (targetInst !== null && !isFiberMounted(targetInst)) { - // If we get an event (ex: img onload) before committing that - // component's mount, ignore it for now (that is, treat it as if it was an - // event on a non-React tree). We might also consider queueing events and - // dispatching them after the mount. - targetInst = null; - } - if (enableFlareAPI) { if (eventSystemFlags === PLUGIN_EVENT_SYSTEM) { dispatchEventForPluginEventSystem( diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js index 8f9addc35a1..7c6e0f90fcf 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.js @@ -107,6 +107,7 @@ import {popProvider} from './ReactFiberNewContext'; import { prepareToHydrateHostInstance, prepareToHydrateHostTextInstance, + prepareToHydrateHostSuspenseInstance, popHydrationState, resetHydrationState, } from './ReactFiberHydrationContext'; @@ -819,6 +820,7 @@ function completeWork( 'A dehydrated suspense component was completed without a hydrated node. ' + 'This is probably a bug in React.', ); + prepareToHydrateHostSuspenseInstance(workInProgress); if (enableSchedulerTracing) { markSpawnedWork(Never); } diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.js b/packages/react-reconciler/src/ReactFiberHydrationContext.js index 30a3c20dd6e..849a44bf075 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.js @@ -41,6 +41,7 @@ import { getFirstHydratableChild, hydrateInstance, hydrateTextInstance, + hydrateSuspenseInstance, getNextHydratableInstanceAfterSuspenseInstance, didNotMatchHydratedContainerTextInstance, didNotMatchHydratedTextInstance, @@ -370,6 +371,26 @@ function prepareToHydrateHostTextInstance(fiber: Fiber): boolean { return shouldUpdate; } +function prepareToHydrateHostSuspenseInstance(fiber: Fiber): void { + if (!supportsHydration) { + invariant( + false, + 'Expected prepareToHydrateHostSuspenseInstance() to never be called. ' + + 'This error is likely caused by a bug in React. Please file an issue.', + ); + } + + let suspenseState: null | SuspenseState = fiber.memoizedState; + let suspenseInstance: null | SuspenseInstance = + suspenseState !== null ? suspenseState.dehydrated : null; + invariant( + suspenseInstance, + 'Expected to have a hydrated suspense instance. ' + + 'This error is likely caused by a bug in React. Please file an issue.', + ); + hydrateSuspenseInstance(suspenseInstance, fiber); +} + function skipPastDehydratedSuspenseInstance( fiber: Fiber, ): null | HydratableInstance { @@ -471,5 +492,6 @@ export { tryToClaimNextHydratableInstance, prepareToHydrateHostInstance, prepareToHydrateHostTextInstance, + prepareToHydrateHostSuspenseInstance, popHydrationState, }; diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index 2f234a1cfe9..61c71588a42 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -126,6 +126,7 @@ export const getNextHydratableSibling = $$$hostConfig.getNextHydratableSibling; export const getFirstHydratableChild = $$$hostConfig.getFirstHydratableChild; export const hydrateInstance = $$$hostConfig.hydrateInstance; export const hydrateTextInstance = $$$hostConfig.hydrateTextInstance; +export const hydrateSuspenseInstance = $$$hostConfig.hydrateSuspenseInstance; export const getNextHydratableInstanceAfterSuspenseInstance = $$$hostConfig.getNextHydratableInstanceAfterSuspenseInstance; export const clearSuspenseBoundary = $$$hostConfig.clearSuspenseBoundary; diff --git a/packages/shared/HostConfigWithNoHydration.js b/packages/shared/HostConfigWithNoHydration.js index 1be5f0b8a98..7fa5a025b6a 100644 --- a/packages/shared/HostConfigWithNoHydration.js +++ b/packages/shared/HostConfigWithNoHydration.js @@ -34,6 +34,7 @@ export const getNextHydratableSibling = shim; export const getFirstHydratableChild = shim; export const hydrateInstance = shim; export const hydrateTextInstance = shim; +export const hydrateSuspenseInstance = shim; export const getNextHydratableInstanceAfterSuspenseInstance = shim; export const clearSuspenseBoundary = shim; export const clearSuspenseBoundaryFromContainer = shim; diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 5946ec50069..5cac8a18d89 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -341,5 +341,6 @@ "340": "Threw in newly mounted dehydrated component. This is likely a bug in React. Please file an issue.", "341": "We just came from a parent so we must have had a parent. This is a bug in React.", "342": "A React component suspended while rendering, but no fallback UI was specified.\n\nAdd a component higher in the tree to provide a loading indicator or placeholder to display.", - "343": "ReactDOMServer does not yet support scope components." + "343": "ReactDOMServer does not yet support scope components.", + "344": "Expected prepareToHydrateHostSuspenseInstance() to never be called. This error is likely caused by a bug in React. Please file an issue." } From 5a28397a511852dc65c33181201f56cc84394dd4 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 27 Aug 2019 21:34:14 -0700 Subject: [PATCH 3/8] 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. --- ...DOMServerPartialHydration-test.internal.js | 82 +++++++++++++++++++ .../ReactServerRenderingHydration-test.js | 58 +++++++++++++ packages/react-dom/src/client/ReactDOM.js | 3 + .../src/client/ReactDOMComponentTree.js | 12 +++ .../src/events/EnterLeaveEventPlugin.js | 4 + .../src/events/ReactDOMEventListener.js | 21 ++++- 6 files changed, 178 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 7f76fa403a7..f4e465ea4d3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -1975,4 +1975,86 @@ describe('ReactDOMServerPartialHydration', () => { 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
clicks++} ref={childSlotRef} />; + } + + function Child({text}) { + if (suspend) { + throw promise; + } else { + return Click me; + } + } + + function App() { + // The root is a Suspense boundary. + return ( + + + + ); + } + + suspend = false; + let finalHTML = ReactDOMServer.renderToString(); + + 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(); + 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(); + 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); + }); }); diff --git a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js index a533923d5b7..819855bbfce 100644 --- a/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js +++ b/packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js @@ -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
clicks++} ref={childSlotRef} />; + } + + function App() { + return ( +
+ Click me +
+ ); + } + + let finalHTML = ReactDOMServer.renderToString(); + + 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(); + 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(); + // 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); + }); }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index e11756cb6bf..80ee16b7bf0 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -70,6 +70,7 @@ import { getNodeFromInstance, getFiberCurrentPropsFromNode, getClosestInstanceFromNode, + markContainerAsRoot, } from './ReactDOMComponentTree'; import {restoreControlledState} from './ReactDOMComponent'; import {dispatchEvent} from '../events/ReactDOMEventListener'; @@ -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) { @@ -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( diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index c90c43b5a66..4a28aa664c8 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -15,11 +15,16 @@ const randomKey = Math.random() .slice(2); const internalInstanceKey = '__reactInternalInstance$' + randomKey; const internalEventHandlersKey = '__reactEventHandlers$' + randomKey; +const internalContainerInstanceKey = '__reactContainere$' + randomKey; export function precacheFiberNode(hostInst, node) { node[internalInstanceKey] = hostInst; } +export function markContainerAsRoot(hostRoot, node) { + node[internalContainerInstanceKey] = hostRoot; +} + /** * Given a DOM node, return the closest ReactDOMComponent or * ReactDOMTextComponent instance ancestor. @@ -72,6 +77,13 @@ export function getClosestInstanceFromNode(targetNode) { } return targetInst; } + // Next we'll check if this is a container root that could include + // React nodes in the future. + targetInst = parentNode[internalContainerInstanceKey]; + if (targetInst) { + // If so, we return the HostRoot Fiber. + return targetInst; + } targetNode = parentNode; parentNode = targetNode.parentNode; } diff --git a/packages/react-dom/src/events/EnterLeaveEventPlugin.js b/packages/react-dom/src/events/EnterLeaveEventPlugin.js index c16db1c1111..41330064881 100644 --- a/packages/react-dom/src/events/EnterLeaveEventPlugin.js +++ b/packages/react-dom/src/events/EnterLeaveEventPlugin.js @@ -19,6 +19,7 @@ import { getClosestInstanceFromNode, getNodeFromInstance, } from '../client/ReactDOMComponentTree'; +import {HostComponent, HostText} from 'shared/ReactWorkTags'; const eventTypes = { mouseEnter: { @@ -89,6 +90,9 @@ const EnterLeaveEventPlugin = { from = targetInst; const related = nativeEvent.relatedTarget || nativeEvent.toElement; to = related ? getClosestInstanceFromNode(related) : null; + if (to !== null && to.tag !== HostComponent && to.tag !== HostText) { + to = null; + } } else { // Moving to a node from outside the window. from = null; diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 2afb35aa0c5..e6478fecf81 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -23,7 +23,12 @@ import { import {runExtractedPluginEventsInBatch} from 'legacy-events/EventPluginHub'; import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem'; import {isFiberMounted} from 'react-reconciler/reflection'; -import {HostRoot, SuspenseComponent} from 'shared/ReactWorkTags'; +import { + HostRoot, + SuspenseComponent, + HostComponent, + HostText, +} from 'shared/ReactWorkTags'; import { type EventSystemFlags, PLUGIN_EVENT_SYSTEM, @@ -77,6 +82,9 @@ type BookKeepingInstance = { * other). If React trees are not nested, returns null. */ function findRootContainerNode(inst) { + if (inst.tag === HostRoot) { + return inst.stateNode.containerInfo; + } // TODO: It may be a good idea to cache this to prevent unnecessary DOM // traversal, but caching is difficult to do correctly without using a // mutation observer to listen for all DOM changes. @@ -141,7 +149,9 @@ function handleTopLevel(bookKeeping: BookKeepingInstance) { if (!root) { break; } - bookKeeping.ancestors.push(ancestor); + if (ancestor.tag === HostComponent || ancestor.tag === HostText) { + bookKeeping.ancestors.push(ancestor); + } ancestor = getClosestInstanceFromNode(root); } while (ancestor); @@ -319,6 +329,13 @@ export function dispatchEvent( // For now we're going to just ignore this event as if it's // not mounted. targetInst = null; + } else if (targetInst.tag === HostRoot) { + // We have not yet mounted/hydrated the first children. + // TODO: This is a good opportunity to schedule a replay of + // the event instead once this root has been hydrated. + // For now we're going to just ignore this event as if it's + // not mounted. + targetInst = null; } } else { // If we get an event (ex: img onload) before committing that From 14db1d7a11ab49add5a3ac2e635db833a88f3915 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 4 Sep 2019 17:34:03 -0700 Subject: [PATCH 4/8] 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. --- packages/react-dom/src/events/ReactDOMEventListener.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index e6478fecf81..7a00158e885 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -338,6 +338,11 @@ export function dispatchEvent( targetInst = null; } } else { + // TODO: If the nearest match was not mounted because it's part + // an in-progress hydration, this would be a good time schedule + // a replay of the event. However, we don't have easy access to + // the HostRoot or SuspenseComponent here. + // If we get an event (ex: img onload) before committing that // component's mount, ignore it for now (that is, treat it as if it was an // event on a non-React tree). We might also consider queueing events and From fbaca52bf4a173747ac0b2ce0652f34008cc3e64 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 4 Sep 2019 22:14:53 -0700 Subject: [PATCH 5/8] 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. --- .../src/events/ReactDOMEventListener.js | 29 +++++++++--------- .../src/ReactFiberBeginWork.js | 23 ++++++++------ .../src/ReactFiberHydrationContext.js | 4 +-- .../src/ReactFiberTreeReflection.js | 30 +++++++++---------- 4 files changed, 44 insertions(+), 42 deletions(-) diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index 7a00158e885..add4e2ee873 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -22,7 +22,7 @@ import { } from 'legacy-events/ReactGenericBatching'; import {runExtractedPluginEventsInBatch} from 'legacy-events/EventPluginHub'; import {dispatchEventForResponderEventSystem} from '../events/DOMEventResponderSystem'; -import {isFiberMounted} from 'react-reconciler/reflection'; +import {getNearestMountedFiber} from 'react-reconciler/reflection'; import { HostRoot, SuspenseComponent, @@ -322,32 +322,31 @@ export function dispatchEvent( let targetInst = getClosestInstanceFromNode(nativeEventTarget); if (targetInst !== null) { - if (isFiberMounted(targetInst)) { - if (targetInst.tag === SuspenseComponent) { + let nearestMounted = getNearestMountedFiber(targetInst); + if (nearestMounted === null) { + // This tree has been unmounted already. + targetInst = null; + } else { + if (nearestMounted.tag === SuspenseComponent) { // TODO: This is a good opportunity to schedule a replay of // the event instead once this boundary has been hydrated. // For now we're going to just ignore this event as if it's // not mounted. targetInst = null; - } else if (targetInst.tag === HostRoot) { + } else if (nearestMounted.tag === HostRoot) { // We have not yet mounted/hydrated the first children. // TODO: This is a good opportunity to schedule a replay of // the event instead once this root has been hydrated. // For now we're going to just ignore this event as if it's // not mounted. targetInst = null; + } else if (nearestMounted !== targetInst) { + // If we get an event (ex: img onload) before committing that + // component's mount, ignore it for now (that is, treat it as if it was an + // event on a non-React tree). We might also consider queueing events and + // dispatching them after the mount. + targetInst = null; } - } else { - // TODO: If the nearest match was not mounted because it's part - // an in-progress hydration, this would be a good time schedule - // a replay of the event. However, we don't have easy access to - // the HostRoot or SuspenseComponent here. - - // If we get an event (ex: img onload) before committing that - // component's mount, ignore it for now (that is, treat it as if it was an - // event on a non-React tree). We might also consider queueing events and - // dispatching them after the mount. - targetInst = null; } } diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index c75a4cebec3..2918f5c9580 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -947,20 +947,25 @@ function updateHostRoot(current, workInProgress, renderExpirationTime) { // be any children to hydrate which is effectively the same thing as // not hydrating. - // Mark the host root with a Hydrating effect to know that we're - // currently in a mounting state. That way isMounted, findDOMNode and - // event replaying works as expected. - workInProgress.effectTag |= Hydrating; - - // Ensure that children mount into this root without tracking - // side-effects. This ensures that we don't store Placement effects on - // nodes that will be hydrated. - workInProgress.child = mountChildFibers( + let child = mountChildFibers( workInProgress, null, nextChildren, renderExpirationTime, ); + workInProgress.child = child; + + let node = child; + while (node) { + // Mark each child as hydrating. This is a fast path to know whether this + // tree is part of a hydrating tree. This is used to determine if a child + // node has fully mounted yet, and for scheduling event replaying. + // Conceptually this is similar to Placement in that a new subtree is + // inserted into the React tree here. It just happens to not need DOM + // mutations because it already exists. + node.effectTag = (node.effectTag & ~Placement) | Hydrating; + node = node.sibling; + } } else { // Otherwise reset hydration state in case we aborted and resumed another // root. diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.js b/packages/react-reconciler/src/ReactFiberHydrationContext.js index 849a44bf075..8ae0f7b8516 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.js @@ -24,7 +24,7 @@ import { HostRoot, SuspenseComponent, } from 'shared/ReactWorkTags'; -import {Deletion, Placement} from 'shared/ReactSideEffectTags'; +import {Deletion, Placement, Hydrating} from 'shared/ReactSideEffectTags'; import invariant from 'shared/invariant'; import { @@ -140,7 +140,7 @@ function deleteHydratableInstance( } function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) { - fiber.effectTag |= Placement; + fiber.effectTag = (fiber.effectTag & ~Hydrating) | Placement; if (__DEV__) { switch (returnFiber.tag) { case HostRoot: { diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index 7b9e687de69..8d5c360a23c 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -28,14 +28,9 @@ import {enableFundamentalAPI} from 'shared/ReactFeatureFlags'; const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; -const MOUNTING = 1; -const MOUNTED = 2; -const UNMOUNTED = 3; - -type MountState = 1 | 2 | 3; - -function isFiberMountedImpl(fiber: Fiber): MountState { +export function getNearestMountedFiber(fiber: Fiber): null | Fiber { let node = fiber; + let nearestMounted = fiber; if (!fiber.alternate) { // If there is no alternate, this might be a new tree that isn't inserted // yet. If it is, then it will have a pending insertion effect on it. @@ -43,7 +38,10 @@ function isFiberMountedImpl(fiber: Fiber): MountState { do { node = nextNode; if ((node.effectTag & (Placement | Hydrating)) !== NoEffect) { - return MOUNTING; + // This is an insertion or in-progress hydration. The nearest possible + // mounted fiber is the parent but we need to continue to figure out + // if that one is still mounted. + nearestMounted = node.return; } nextNode = node.return; } while (nextNode); @@ -55,15 +53,15 @@ function isFiberMountedImpl(fiber: Fiber): MountState { if (node.tag === HostRoot) { // TODO: Check if this was a nested HostRoot when used with // renderContainerIntoSubtree. - return MOUNTED; + return nearestMounted; } // If we didn't hit the root, that means that we're in an disconnected tree // that has been unmounted. - return UNMOUNTED; + return null; } export function isFiberMounted(fiber: Fiber): boolean { - return isFiberMountedImpl(fiber) === MOUNTED; + return getNearestMountedFiber(fiber) === fiber; } export function isMounted(component: React$Component): boolean { @@ -89,12 +87,12 @@ export function isMounted(component: React$Component): boolean { if (!fiber) { return false; } - return isFiberMountedImpl(fiber) === MOUNTED; + return getNearestMountedFiber(fiber) === fiber; } function assertIsMounted(fiber) { invariant( - isFiberMountedImpl(fiber) === MOUNTED, + getNearestMountedFiber(fiber) === fiber, 'Unable to find node on an unmounted component.', ); } @@ -103,12 +101,12 @@ export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null { let alternate = fiber.alternate; if (!alternate) { // If there is no alternate, then we only need to check if it is mounted. - const state = isFiberMountedImpl(fiber); + const nearestMounted = getNearestMountedFiber(fiber); invariant( - state !== UNMOUNTED, + nearestMounted !== null, 'Unable to find node on an unmounted component.', ); - if (state === MOUNTING) { + if (nearestMounted !== fiber) { return null; } return fiber; From 38737b5c9b08bbcb2a07c00ee49c4531c53c0169 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 4 Sep 2019 22:23:19 -0700 Subject: [PATCH 6/8] Add todo for bug I spotted --- packages/react-reconciler/src/ReactFiberBeginWork.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 2918f5c9580..945191c4454 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -938,6 +938,9 @@ function updateHostRoot(current, workInProgress, renderExpirationTime) { } const root: FiberRoot = workInProgress.stateNode; if ( + // TODO: This is a bug because if we render null after having hydrating, + // we'll reenter hydration state at the next update which will then + // trigger hydration warnings. (current === null || current.child === null) && root.hydrate && enterHydrationState(workInProgress) From be7e4f98173875e5798d5a70dc453e1d844a9ae4 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 5 Sep 2019 08:40:44 -0700 Subject: [PATCH 7/8] Cache tags --- packages/react-dom/src/events/ReactDOMEventListener.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/events/ReactDOMEventListener.js b/packages/react-dom/src/events/ReactDOMEventListener.js index add4e2ee873..b2d7e33b5d3 100644 --- a/packages/react-dom/src/events/ReactDOMEventListener.js +++ b/packages/react-dom/src/events/ReactDOMEventListener.js @@ -149,7 +149,8 @@ function handleTopLevel(bookKeeping: BookKeepingInstance) { if (!root) { break; } - if (ancestor.tag === HostComponent || ancestor.tag === HostText) { + const tag = ancestor.tag; + if (tag === HostComponent || tag === HostText) { bookKeeping.ancestors.push(ancestor); } ancestor = getClosestInstanceFromNode(root); @@ -327,13 +328,14 @@ export function dispatchEvent( // This tree has been unmounted already. targetInst = null; } else { - if (nearestMounted.tag === SuspenseComponent) { + const tag = nearestMounted.tag; + if (tag === SuspenseComponent) { // TODO: This is a good opportunity to schedule a replay of // the event instead once this boundary has been hydrated. // For now we're going to just ignore this event as if it's // not mounted. targetInst = null; - } else if (nearestMounted.tag === HostRoot) { + } else if (tag === HostRoot) { // We have not yet mounted/hydrated the first children. // TODO: This is a good opportunity to schedule a replay of // the event instead once this root has been hydrated. From 1616fe32ae9b3e44e71d61bcae12d55d5a9ac096 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 5 Sep 2019 08:42:18 -0700 Subject: [PATCH 8/8] 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. --- .../src/client/ReactDOMComponentTree.js | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js index 4a28aa664c8..2fe287ec8b5 100644 --- a/packages/react-dom/src/client/ReactDOMComponentTree.js +++ b/packages/react-dom/src/client/ReactDOMComponentTree.js @@ -25,19 +25,35 @@ export function markContainerAsRoot(hostRoot, node) { node[internalContainerInstanceKey] = hostRoot; } -/** - * Given a DOM node, return the closest ReactDOMComponent or - * ReactDOMTextComponent instance ancestor. - */ +// 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; + } targetInst = parentNode[internalInstanceKey]; if (targetInst) { // Since this wasn't the direct target of the event, we might have @@ -77,13 +93,6 @@ export function getClosestInstanceFromNode(targetNode) { } return targetInst; } - // Next we'll check if this is a container root that could include - // React nodes in the future. - targetInst = parentNode[internalContainerInstanceKey]; - if (targetInst) { - // If so, we return the HostRoot Fiber. - return targetInst; - } targetNode = parentNode; parentNode = targetNode.parentNode; }