Skip to content

Commit e5dd82a

Browse files
authored
Warn for using a React owned node as a Container if it also has text content (facebook#32774)
The problem with setting both `children` or `dangerouslySetInnerHTML` and also using a ref on a DOM node to either manually append children or using it as a Container for `createRoot` or `createPortal` is that it's ambiguous which children should win. Ideally you use one of the four options to control children. Meaning that ideally you always use a leaf container for refs like this. Unfortunately it's very common to use a React owned thing with children as a Container of a Portal. For example `document.body` can have both regular React children and be used as a Portal container. This isn't really fully supported and has some undefined behavior like relative order isn't guaranteed but still very common. It is extra bad if the children are a `string`/`number` or if `dangerouslySetInnerHTML` is set. Because then when ever that reactively updates it'll clear out any manually added DOM nodes. When this happens isn't guaranteed. It's always happening as far as the reactivity is concerned. See facebook#31600 Therefore, we should warn for this specific pattern. This still allows non-text children as a compromise even though that behavior is also somewhat undefined.
1 parent 731ae3e commit e5dd82a

File tree

2 files changed

+48
-1
lines changed

2 files changed

+48
-1
lines changed

packages/react-dom-bindings/src/client/ReactDOMComponentTree.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,7 @@ export function getNodeFromInstance(inst: Fiber): Instance | TextInstance {
211211
}
212212

213213
export function getFiberCurrentPropsFromNode(
214-
node: Instance | TextInstance | SuspenseInstance,
214+
node: Container | Instance | TextInstance | SuspenseInstance,
215215
): Props {
216216
return (node: any)[internalPropsKey] || null;
217217
}

packages/react-dom-bindings/src/client/ReactFiberConfigDOM.js

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import type {TransitionTypes} from 'react/src/ReactTransitionType';
3030
import {NotPending} from '../shared/ReactDOMFormActions';
3131

3232
import {getCurrentRootHostContainer} from 'react-reconciler/src/ReactFiberHostContext';
33+
import {runWithFiberInDEV} from 'react-reconciler/src/ReactCurrentFiber';
3334

3435
import hasOwnProperty from 'shared/hasOwnProperty';
3536
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
@@ -43,6 +44,8 @@ export {
4344
import {
4445
precacheFiberNode,
4546
updateFiberProps,
47+
getFiberCurrentPropsFromNode,
48+
getInstanceFromNode,
4649
getClosestInstanceFromNode,
4750
getFiberFromScopeInstance,
4851
getInstanceFromNode as getInstanceFromNodeDOMTree,
@@ -821,10 +824,51 @@ export function appendChild(
821824
}
822825
}
823826

827+
function warnForReactChildrenConflict(container: Container): void {
828+
if (__DEV__) {
829+
if ((container: any).__reactWarnedAboutChildrenConflict) {
830+
return;
831+
}
832+
const props = getFiberCurrentPropsFromNode(container);
833+
if (props !== null) {
834+
const fiber = getInstanceFromNode(container);
835+
if (fiber !== null) {
836+
if (
837+
typeof props.children === 'string' ||
838+
typeof props.children === 'number'
839+
) {
840+
(container: any).__reactWarnedAboutChildrenConflict = true;
841+
// Run the warning with the Fiber of the container for context of where the children are specified.
842+
// We could also maybe use the Portal. The current execution context is the child being added.
843+
runWithFiberInDEV(fiber, () => {
844+
console.error(
845+
'Cannot use a ref on a React element as a container to `createRoot` or `createPortal` ' +
846+
'if that element also sets "children" text content using React. It should be a leaf with no children. ' +
847+
"Otherwise it's ambiguous which children should be used.",
848+
);
849+
});
850+
} else if (props.dangerouslySetInnerHTML != null) {
851+
(container: any).__reactWarnedAboutChildrenConflict = true;
852+
runWithFiberInDEV(fiber, () => {
853+
console.error(
854+
'Cannot use a ref on a React element as a container to `createRoot` or `createPortal` ' +
855+
'if that element also sets "dangerouslySetInnerHTML" using React. It should be a leaf with no children. ' +
856+
"Otherwise it's ambiguous which children should be used.",
857+
);
858+
});
859+
}
860+
}
861+
}
862+
}
863+
}
864+
824865
export function appendChildToContainer(
825866
container: Container,
826867
child: Instance | TextInstance,
827868
): void {
869+
if (__DEV__) {
870+
warnForReactChildrenConflict(container);
871+
}
828872
let parentNode: DocumentFragment | Element;
829873
if (container.nodeType === DOCUMENT_NODE) {
830874
parentNode = (container: any).body;
@@ -888,6 +932,9 @@ export function insertInContainerBefore(
888932
child: Instance | TextInstance,
889933
beforeChild: Instance | TextInstance | SuspenseInstance,
890934
): void {
935+
if (__DEV__) {
936+
warnForReactChildrenConflict(container);
937+
}
891938
let parentNode: DocumentFragment | Element;
892939
if (container.nodeType === DOCUMENT_NODE) {
893940
parentNode = (container: any).body;

0 commit comments

Comments
 (0)