From 68cea765b01460241c853cb8a8749bfcdd2808fc Mon Sep 17 00:00:00 2001 From: Josh Story Date: Tue, 28 Jan 2025 09:29:13 -0800 Subject: [PATCH] [Fiber] Disable comments as containers in OSS 3 years ago we partially disabled comment nodes as valid containers. Some unflagged support was left in due to legacy APIs like `unmountComponentAtNode` and `unstable_renderSubtreeIntoContainer` but these were since removed in React 19. This update flags the remaining uses of comments as containers. --- .../src/client/ReactDOMContainer.js | 13 -- .../src/client/ReactFiberConfigDOM.js | 121 ++++++++---------- .../src/events/DOMPluginEventSystem.js | 4 +- packages/react-dom/src/client/ReactDOMRoot.js | 3 +- .../react-dom/src/client/ReactDOMRootFB.js | 21 ++- 5 files changed, 75 insertions(+), 87 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMContainer.js b/packages/react-dom-bindings/src/client/ReactDOMContainer.js index 0af0f13a790a8..df9bcfb443803 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMContainer.js +++ b/packages/react-dom-bindings/src/client/ReactDOMContainer.js @@ -27,16 +27,3 @@ export function isValidContainer(node: any): boolean { (node: any).nodeValue === ' react-mount-point-unstable ')) ); } - -// TODO: Remove this function which also includes comment nodes. -// We only use it in places that are currently more relaxed. -export function isValidContainerLegacy(node: any): boolean { - return !!( - node && - (node.nodeType === ELEMENT_NODE || - node.nodeType === DOCUMENT_NODE || - node.nodeType === DOCUMENT_FRAGMENT_NODE || - (node.nodeType === COMMENT_NODE && - (node: any).nodeValue === ' react-mount-point-unstable ')) - ); -} diff --git a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js index 5d58df645c94d..68ff3bf8be34e 100644 --- a/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js +++ b/packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js @@ -94,6 +94,7 @@ import { enableTrustedTypesIntegration, disableLegacyMode, enableMoveBefore, + disableCommentsAsDOMContainers, } from 'shared/ReactFeatureFlags'; import { HostComponent, @@ -258,7 +259,7 @@ export function getRootHostContext( } default: { const container: any = - nodeType === COMMENT_NODE + !disableCommentsAsDOMContainers && nodeType === COMMENT_NODE ? rootContainerInstance.parentNode : rootContainerInstance; type = container.tagName; @@ -802,29 +803,25 @@ export function appendChildToContainer( container: Container, child: Instance | TextInstance, ): void { - let parentNode: Document | Element; - switch (container.nodeType) { - case COMMENT_NODE: { - parentNode = (container.parentNode: any); - if (supportsMoveBefore) { - // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. - parentNode.moveBefore(child, container); - } else { - parentNode.insertBefore(child, container); - } - return; - } - case DOCUMENT_NODE: { - parentNode = (container: any).body; - break; - } - default: { - if (container.nodeName === 'HTML') { - parentNode = (container.ownerDocument.body: any); - } else { - parentNode = (container: any); - } + let parentNode: DocumentFragment | Element; + if (container.nodeType === DOCUMENT_NODE) { + parentNode = (container: any).body; + } else if ( + !disableCommentsAsDOMContainers && + container.nodeType === COMMENT_NODE + ) { + parentNode = (container.parentNode: any); + if (supportsMoveBefore) { + // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. + parentNode.moveBefore(child, container); + } else { + parentNode.insertBefore(child, container); } + return; + } else if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); } if (supportsMoveBefore) { // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. @@ -869,24 +866,18 @@ export function insertInContainerBefore( child: Instance | TextInstance, beforeChild: Instance | TextInstance | SuspenseInstance, ): void { - let parentNode: Document | Element; - switch (container.nodeType) { - case COMMENT_NODE: { - parentNode = (container.parentNode: any); - break; - } - case DOCUMENT_NODE: { - const ownerDocument: Document = (container: any); - parentNode = (ownerDocument.body: any); - break; - } - default: { - if (container.nodeName === 'HTML') { - parentNode = (container.ownerDocument.body: any); - } else { - parentNode = (container: any); - } - } + let parentNode: DocumentFragment | Element; + if (container.nodeType === DOCUMENT_NODE) { + parentNode = (container: any).body; + } else if ( + !disableCommentsAsDOMContainers && + container.nodeType === COMMENT_NODE + ) { + parentNode = (container.parentNode: any); + } else if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); } if (supportsMoveBefore) { // $FlowFixMe[prop-missing]: We've checked this with supportsMoveBefore. @@ -943,20 +934,18 @@ export function removeChildFromContainer( container: Container, child: Instance | TextInstance | SuspenseInstance, ): void { - let parentNode: Document | Element; - switch (container.nodeType) { - case COMMENT_NODE: - parentNode = (container.parentNode: any); - break; - case DOCUMENT_NODE: - parentNode = (container: any).body; - break; - default: - if (container.nodeName === 'HTML') { - parentNode = (container.ownerDocument.body: any); - } else { - parentNode = (container: any); - } + let parentNode: DocumentFragment | Element; + if (container.nodeType === DOCUMENT_NODE) { + parentNode = (container: any).body; + } else if ( + !disableCommentsAsDOMContainers && + container.nodeType === COMMENT_NODE + ) { + parentNode = (container.parentNode: any); + } else if (container.nodeName === 'HTML') { + parentNode = (container.ownerDocument.body: any); + } else { + parentNode = (container: any); } parentNode.removeChild(child); } @@ -1037,18 +1026,20 @@ export function clearSuspenseBoundaryFromContainer( container: Container, suspenseInstance: SuspenseInstance, ): void { - if (container.nodeType === COMMENT_NODE) { - clearSuspenseBoundary((container.parentNode: any), suspenseInstance); - } else if (container.nodeType === DOCUMENT_NODE) { - clearSuspenseBoundary((container: any).body, suspenseInstance); + let parentNode: DocumentFragment | Element; + if (container.nodeType === DOCUMENT_NODE) { + parentNode = (container: any).body; + } else if ( + !disableCommentsAsDOMContainers && + container.nodeType === COMMENT_NODE + ) { + parentNode = (container.parentNode: any); } else if (container.nodeName === 'HTML') { - clearSuspenseBoundary( - (container.ownerDocument.body: any), - suspenseInstance, - ); + parentNode = (container.ownerDocument.body: any); } else { - clearSuspenseBoundary((container: any), suspenseInstance); + parentNode = (container: any); } + clearSuspenseBoundary(parentNode, suspenseInstance); // Retry if any event replaying was blocked on this. retryIfBlockedOn(container); } @@ -1990,7 +1981,7 @@ export function getNextHydratableSiblingAfterSingleton( export function describeHydratableInstanceForDevWarnings( instance: HydratableInstance, ): string | {type: string, props: $ReadOnly} { - // Reverse engineer a pseudo react-element from hydratable instnace + // Reverse engineer a pseudo react-element from hydratable instance if (instance.nodeType === ELEMENT_NODE) { // Reverse engineer a set of props that can print for dev warnings return { diff --git a/packages/react-dom-bindings/src/events/DOMPluginEventSystem.js b/packages/react-dom-bindings/src/events/DOMPluginEventSystem.js index 5c3c9bffa1295..c99115df90f56 100644 --- a/packages/react-dom-bindings/src/events/DOMPluginEventSystem.js +++ b/packages/react-dom-bindings/src/events/DOMPluginEventSystem.js @@ -53,6 +53,7 @@ import { enableCreateEventHandleAPI, enableScopeAPI, enableOwnerStacks, + disableCommentsAsDOMContainers, } from 'shared/ReactFeatureFlags'; import {createEventListenerWrapperWithPriority} from './ReactDOMEventListener'; import { @@ -558,7 +559,8 @@ function isMatchingRootContainer( ): boolean { return ( grandContainer === targetContainer || - (grandContainer.nodeType === COMMENT_NODE && + (!disableCommentsAsDOMContainers && + grandContainer.nodeType === COMMENT_NODE && grandContainer.parentNode === targetContainer) ); } diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js index 5cef2d28437b0..37e994cc7bb71 100644 --- a/packages/react-dom/src/client/ReactDOMRoot.js +++ b/packages/react-dom/src/client/ReactDOMRoot.js @@ -16,6 +16,7 @@ import type { import {isValidContainer} from 'react-dom-bindings/src/client/ReactDOMContainer'; import {queueExplicitHydrationTarget} from 'react-dom-bindings/src/events/ReactDOMEventReplaying'; import {REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; +import {disableCommentsAsDOMContainers} from 'shared/ReactFeatureFlags'; export type RootType = { render(children: ReactNodeList): void, @@ -236,7 +237,7 @@ export function createRoot( markContainerAsRoot(root.current, container); const rootContainerElement: Document | Element | DocumentFragment = - container.nodeType === COMMENT_NODE + !disableCommentsAsDOMContainers && container.nodeType === COMMENT_NODE ? (container.parentNode: any) : container; listenToAllSupportedEvents(rootContainerElement); diff --git a/packages/react-dom/src/client/ReactDOMRootFB.js b/packages/react-dom/src/client/ReactDOMRootFB.js index 1e5609726688f..f2368c8cc4fa6 100644 --- a/packages/react-dom/src/client/ReactDOMRootFB.js +++ b/packages/react-dom/src/client/ReactDOMRootFB.js @@ -27,7 +27,10 @@ import { hydrateRoot as hydrateRootImpl, } from './ReactDOMRoot'; -import {disableLegacyMode} from 'shared/ReactFeatureFlags'; +import { + disableLegacyMode, + disableCommentsAsDOMContainers, +} from 'shared/ReactFeatureFlags'; import {clearContainer} from 'react-dom-bindings/src/client/ReactFiberConfigDOM'; import { getInstanceFromNode, @@ -36,7 +39,7 @@ import { unmarkContainerAsRoot, } from 'react-dom-bindings/src/client/ReactDOMComponentTree'; import {listenToAllSupportedEvents} from 'react-dom-bindings/src/events/DOMPluginEventSystem'; -import {isValidContainerLegacy} from 'react-dom-bindings/src/client/ReactDOMContainer'; +import {isValidContainer} from 'react-dom-bindings/src/client/ReactDOMContainer'; import { DOCUMENT_NODE, ELEMENT_NODE, @@ -244,7 +247,9 @@ function legacyCreateRootFromDOMContainer( markContainerAsRoot(root.current, container); const rootContainerElement = - container.nodeType === COMMENT_NODE ? container.parentNode : container; + !disableCommentsAsDOMContainers && container.nodeType === COMMENT_NODE + ? container.parentNode + : container; // $FlowFixMe[incompatible-call] listenToAllSupportedEvents(rootContainerElement); @@ -278,7 +283,9 @@ function legacyCreateRootFromDOMContainer( markContainerAsRoot(root.current, container); const rootContainerElement = - container.nodeType === COMMENT_NODE ? container.parentNode : container; + !disableCommentsAsDOMContainers && container.nodeType === COMMENT_NODE + ? container.parentNode + : container; // $FlowFixMe[incompatible-call] listenToAllSupportedEvents(rootContainerElement); @@ -394,7 +401,7 @@ export function render( ); } - if (!isValidContainerLegacy(container)) { + if (!isValidContainer(container)) { throw new Error('Target container is not a DOM element.'); } @@ -428,7 +435,7 @@ export function unmountComponentAtNode(container: Container): boolean { } throw new Error('ReactDOM: Unsupported Legacy Mode API.'); } - if (!isValidContainerLegacy(container)) { + if (!isValidContainer(container)) { throw new Error('Target container is not a DOM element.'); } @@ -472,7 +479,7 @@ export function unmountComponentAtNode(container: Container): boolean { // Check if the container itself is a React root node. const isContainerReactRoot = container.nodeType === ELEMENT_NODE && - isValidContainerLegacy(container.parentNode) && + isValidContainer(container.parentNode) && // $FlowFixMe[prop-missing] // $FlowFixMe[incompatible-use] !!container.parentNode._reactRootContainer;