Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when doing createRoot twice on the same node (another approach) #17329

Merged
merged 5 commits into from
Nov 10, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,4 +212,22 @@ describe('ReactDOMRoot', () => {
{withoutStack: true},
);
});

it('warns when creating two roots managing the same container', () => {
ReactDOM.createRoot(container);
expect(() => {
ReactDOM.createRoot(container);
}).toWarnDev(
'You are calling ReactDOM.createRoot() on a container that ' +
'has already been passed to createRoot() before. Instead, call ' +
'root.render() on the existing root instead if you want to update it.',
{withoutStack: true},
);
});

it('does not warn when creating second root after first one is unmounted', () => {
const root = ReactDOM.createRoot(container);
root.unmount();
ReactDOM.createRoot(container); // No warning
});
});
74 changes: 49 additions & 25 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ import {
getNodeFromInstance,
getFiberCurrentPropsFromNode,
getClosestInstanceFromNode,
isContainerMarkedAsRoot,
markContainerAsRoot,
unmarkContainerAsRoot,
} from './ReactDOMComponentTree';
import {restoreControlledState} from './ReactDOMComponent';
import {dispatchEvent} from '../events/ReactDOMEventListener';
Expand Down Expand Up @@ -174,11 +176,9 @@ setRestoreImplementation(restoreControlledState);
export type DOMContainer =
| (Element & {
_reactRootContainer: ?_ReactRoot,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
})
| (Document & {
_reactRootContainer: ?_ReactRoot,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
});

type _ReactRoot = {
Expand Down Expand Up @@ -242,6 +242,7 @@ ReactRoot.prototype.unmount = ReactBlockingRoot.prototype.unmount = function(
warnOnInvalidCallback(callback, 'render');
}
updateContainer(null, root, null, callback);
unmarkContainerAsRoot(root.containerInfo);
};

/**
Expand Down Expand Up @@ -448,12 +449,16 @@ const ReactDOM: Object = {
'Target container is not a DOM element.',
);
if (__DEV__) {
warningWithoutStack(
!container._reactHasBeenPassedToCreateRootDEV,
'You are calling ReactDOM.hydrate() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. ' +
'Did you mean to call createRoot(container, {hydrate: true}).render(element)?',
);
const isModernRoot =
isContainerMarkedAsRoot(container) && !container._reactRootContainer;
if (isModernRoot) {
warningWithoutStack(
false,
'You are calling ReactDOM.hydrate() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. ' +
'Did you mean to call createRoot(container, {hydrate: true}).render(element)?',
);
}
}
// TODO: throw or warn if we couldn't hydrate?
return legacyRenderSubtreeIntoContainer(
Expand All @@ -475,12 +480,16 @@ const ReactDOM: Object = {
'Target container is not a DOM element.',
);
if (__DEV__) {
warningWithoutStack(
!container._reactHasBeenPassedToCreateRootDEV,
'You are calling ReactDOM.render() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. ' +
'Did you mean to call root.render(element)?',
);
const isModernRoot =
isContainerMarkedAsRoot(container) && !container._reactRootContainer;
if (isModernRoot) {
warningWithoutStack(
false,
'You are calling ReactDOM.render() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. ' +
'Did you mean to call root.render(element)?',
);
}
}
return legacyRenderSubtreeIntoContainer(
null,
Expand Down Expand Up @@ -521,11 +530,15 @@ const ReactDOM: Object = {
);

if (__DEV__) {
warningWithoutStack(
!container._reactHasBeenPassedToCreateRootDEV,
'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. Did you mean to call root.unmount()?',
);
const isModernRoot =
isContainerMarkedAsRoot(container) && !container._reactRootContainer;
if (isModernRoot) {
warningWithoutStack(
false,
'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
'passed to ReactDOM.createRoot(). This is not supported. Did you mean to call root.unmount()?',
);
}
}

if (container._reactRootContainer) {
Expand All @@ -543,6 +556,7 @@ const ReactDOM: Object = {
unbatchedUpdates(() => {
legacyRenderSubtreeIntoContainer(null, null, container, false, () => {
container._reactRootContainer = null;
unmarkContainerAsRoot(container);
});
});
// If you call unmountComponentAtNode twice in quick succession, you'll
Expand Down Expand Up @@ -650,12 +664,22 @@ function createBlockingRoot(

function warnIfReactDOMContainerInDEV(container) {
if (__DEV__) {
warningWithoutStack(
!container._reactRootContainer,
'You are calling ReactDOM.createRoot() on a container that was previously ' +
'passed to ReactDOM.render(). This is not supported.',
);
container._reactHasBeenPassedToCreateRootDEV = true;
if (isContainerMarkedAsRoot(container)) {
if (container._reactRootContainer) {
warningWithoutStack(
false,
'You are calling ReactDOM.createRoot() on a container that was previously ' +
'passed to ReactDOM.render(). This is not supported.',
);
} else {
warningWithoutStack(
false,
'You are calling ReactDOM.createRoot() on a container that ' +
'has already been passed to createRoot() before. Instead, call ' +
'root.render() on the existing root instead if you want to update it.',
);
}
}
}
}

Expand Down
8 changes: 8 additions & 0 deletions packages/react-dom/src/client/ReactDOMComponentTree.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ export function markContainerAsRoot(hostRoot, node) {
node[internalContainerInstanceKey] = hostRoot;
}

export function unmarkContainerAsRoot(node) {
node[internalContainerInstanceKey] = null;
}

export function isContainerMarkedAsRoot(node) {
return !!node[internalContainerInstanceKey];
}

// 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.
Expand Down