[Fiber] Disable comments as containers in OSS (#32250)

gnoff · web-flow · commit 0605cd9f38f8 · 2025-02-04T12:39:52.000-08:00
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.
diff --git 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
@@ -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);
 }
@@ -1039,18 +1028,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);
 }
@@ -1992,7 +1983,7 @@ export function getNextHydratableSiblingAfterSingleton(
 export function describeHydratableInstanceForDevWarnings(
   instance: HydratableInstance,
 ): string | {type: string, props: $ReadOnly<Props>} {
-  // 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
@@ -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
@@ -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
@@ -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;
