Skip to content

Commit

Permalink
[Fiber] Disable comments as containers in OSS
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@gnoff
gnoff committed Feb 1, 2025
1 parent 6ca6e31 commit 68cea76
Show file tree
Hide file tree
Showing 5 changed files with 75 additions and 87 deletions.
13 changes: 0 additions & 13 deletions packages/react-dom-bindings/src/client/ReactDOMContainer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 '))
);
}
121 changes: 56 additions & 65 deletions packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ import {
enableTrustedTypesIntegration,
disableLegacyMode,
enableMoveBefore,
disableCommentsAsDOMContainers,
} from 'shared/ReactFeatureFlags';
import {
HostComponent,
Expand Down Expand Up @@ -258,7 +259,7 @@ export function getRootHostContext(
}
default: {
const container: any =
nodeType === COMMENT_NODE
!disableCommentsAsDOMContainers && nodeType === COMMENT_NODE
? rootContainerInstance.parentNode
: rootContainerInstance;
type = container.tagName;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -1990,7 +1981,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 {
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom-bindings/src/events/DOMPluginEventSystem.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
enableCreateEventHandleAPI,
enableScopeAPI,
enableOwnerStacks,
disableCommentsAsDOMContainers,
} from 'shared/ReactFeatureFlags';
import {createEventListenerWrapperWithPriority} from './ReactDOMEventListener';
import {
Expand Down Expand Up @@ -558,7 +559,8 @@ function isMatchingRootContainer(
): boolean {
return (
grandContainer === targetContainer ||
(grandContainer.nodeType === COMMENT_NODE &&
(!disableCommentsAsDOMContainers &&
grandContainer.nodeType === COMMENT_NODE &&
grandContainer.parentNode === targetContainer)
);
}
Expand Down
3 changes: 2 additions & 1 deletion packages/react-dom/src/client/ReactDOMRoot.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down
21 changes: 14 additions & 7 deletions packages/react-dom/src/client/ReactDOMRootFB.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -394,7 +401,7 @@ export function render(
);
}

if (!isValidContainerLegacy(container)) {
if (!isValidContainer(container)) {
throw new Error('Target container is not a DOM element.');
}

Expand Down Expand Up @@ -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.');
}

Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 68cea76

Please sign in to comment.