Skip to content

Commit

Permalink
Warn when mixing createRoot() and old APIs (facebook#14615)
Browse files Browse the repository at this point in the history 
* Warn when mixing createRoot() and old APIs

* Move container checks to entry points

This way further warning check doesn't crash on bad inputs.

* Fix Flow

* Rename flag to be clearer

* managed by -> passed to

* Revert accidental change

* Fix Fire shim to match
  • Loading branch information
@gaearon @jetoneza
gaearon authored and jetoneza committed Jan 23, 2019
1 parent 469f9d4 commit 6f0ac82
Show file tree
Hide file tree
Showing 3 changed files with 202 additions and 16 deletions.
100 changes: 100 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,4 +374,104 @@ describe('ReactDOMRoot', () => {
'unstable_createRoot(...): Target container is not a DOM element.',
);
});

it('warns when rendering with legacy API into createRoot() container', () => {
const root = ReactDOM.unstable_createRoot(container);
root.render(<div>Hi</div>);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
expect(() => {
ReactDOM.render(<div>Bye</div>, container);
}).toWarnDev(
[
// We care about this warning:
'You are calling ReactDOM.render() on a container that was previously ' +
'passed to ReactDOM.unstable_createRoot(). This is not supported. ' +
'Did you mean to call root.render(element)?',
// This is more of a symptom but restructuring the code to avoid it isn't worth it:
'Replacing React-rendered children with a new root component.',
],
{withoutStack: true},
);
jest.runAllTimers();
// This works now but we could disallow it:
expect(container.textContent).toEqual('Bye');
});

it('warns when hydrating with legacy API into createRoot() container', () => {
const root = ReactDOM.unstable_createRoot(container);
root.render(<div>Hi</div>);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
expect(() => {
ReactDOM.hydrate(<div>Hi</div>, container);
}).toWarnDev(
[
// We care about this warning:
'You are calling ReactDOM.hydrate() on a container that was previously ' +
'passed to ReactDOM.unstable_createRoot(). This is not supported. ' +
'Did you mean to call root.render(element, {hydrate: true})?',
// This is more of a symptom but restructuring the code to avoid it isn't worth it:
'Replacing React-rendered children with a new root component.',
],
{withoutStack: true},
);
});

it('warns when unmounting with legacy API (no previous content)', () => {
const root = ReactDOM.unstable_createRoot(container);
root.render(<div>Hi</div>);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
let unmounted = false;
expect(() => {
unmounted = ReactDOM.unmountComponentAtNode(container);
}).toWarnDev(
[
// We care about this warning:
'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
'passed to ReactDOM.unstable_createRoot(). This is not supported. Did you mean to call root.unmount()?',
// This is more of a symptom but restructuring the code to avoid it isn't worth it:
"The node you're attempting to unmount was rendered by React and is not a top-level container.",
],
{withoutStack: true},
);
expect(unmounted).toBe(false);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
root.unmount();
jest.runAllTimers();
expect(container.textContent).toEqual('');
});

it('warns when unmounting with legacy API (has previous content)', () => {
// Currently createRoot().render() doesn't clear this.
container.appendChild(document.createElement('div'));
// The rest is the same as test above.
const root = ReactDOM.unstable_createRoot(container);
root.render(<div>Hi</div>);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
let unmounted = false;
expect(() => {
unmounted = ReactDOM.unmountComponentAtNode(container);
}).toWarnDev('Did you mean to call root.unmount()?', {withoutStack: true});
expect(unmounted).toBe(false);
jest.runAllTimers();
expect(container.textContent).toEqual('Hi');
root.unmount();
jest.runAllTimers();
expect(container.textContent).toEqual('');
});

it('warns when passing legacy container to createRoot()', () => {
ReactDOM.render(<div>Hi</div>, container);
expect(() => {
ReactDOM.unstable_createRoot(container);
}).toWarnDev(
'You are calling ReactDOM.unstable_createRoot() on a container that was previously ' +
'passed to ReactDOM.render(). This is not supported.',
{withoutStack: true},
);
});
});
59 changes: 51 additions & 8 deletions packages/react-dom/src/client/ReactDOM.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import type {
FiberRoot,
Batch as FiberRootBatch,
} from 'react-reconciler/src/ReactFiberRoot';
import type {Container} from './ReactDOMHostConfig';

import '../shared/checkReact';
import './ReactDOMClientInjection';
Expand Down Expand Up @@ -160,9 +159,11 @@ setRestoreImplementation(restoreControlledState);
export type DOMContainer =
| (Element & {
_reactRootContainer: ?Root,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
})
| (Document & {
_reactRootContainer: ?Root,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
});

type Batch = FiberRootBatch & {
Expand Down Expand Up @@ -362,7 +363,7 @@ ReactWork.prototype._onCommit = function(): void {
};

function ReactRoot(
container: Container,
container: DOMContainer,
isConcurrent: boolean,
hydrate: boolean,
) {
Expand Down Expand Up @@ -543,12 +544,6 @@ function legacyRenderSubtreeIntoContainer(
forceHydrate: boolean,
callback: ?Function,
) {
// TODO: Ensure all entry points contain this check
invariant(
isValidContainer(container),
'Target container is not a DOM element.',
);

if (__DEV__) {
topLevelUpdateWarnings(container);
}
Expand Down Expand Up @@ -652,6 +647,19 @@ const ReactDOM: Object = {
},

hydrate(element: React$Node, container: DOMContainer, callback: ?Function) {
invariant(
isValidContainer(container),
'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.%s(). This is not supported. ' +
'Did you mean to call root.render(element, {hydrate: true})?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}
// TODO: throw or warn if we couldn't hydrate?
return legacyRenderSubtreeIntoContainer(
null,
Expand All @@ -667,6 +675,19 @@ const ReactDOM: Object = {
container: DOMContainer,
callback: ?Function,
) {
invariant(
isValidContainer(container),
'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.%s(). This is not supported. ' +
'Did you mean to call root.render(element)?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}
return legacyRenderSubtreeIntoContainer(
null,
element,
Expand All @@ -682,6 +703,10 @@ const ReactDOM: Object = {
containerNode: DOMContainer,
callback: ?Function,
) {
invariant(
isValidContainer(containerNode),
'Target container is not a DOM element.',
);
invariant(
parentComponent != null && hasInstance(parentComponent),
'parentComponent must be a valid React Component',
Expand All @@ -701,6 +726,15 @@ const ReactDOM: Object = {
'unmountComponentAtNode(...): Target container is not a DOM element.',
);

if (__DEV__) {
warningWithoutStack(
!container._reactHasBeenPassedToCreateRootDEV,
'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
'passed to ReactDOM.%s(). This is not supported. Did you mean to call root.unmount()?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}

if (container._reactRootContainer) {
if (__DEV__) {
const rootEl = getReactRootElementInContainer(container);
Expand Down Expand Up @@ -805,6 +839,15 @@ function createRoot(container: DOMContainer, options?: RootOptions): ReactRoot {
'%s(...): Target container is not a DOM element.',
functionName,
);
if (__DEV__) {
warningWithoutStack(
!container._reactRootContainer,
'You are calling ReactDOM.%s() on a container that was previously ' +
'passed to ReactDOM.render(). This is not supported.',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
container._reactHasBeenPassedToCreateRootDEV = true;
}
const hydrate = options != null && options.hydrate === true;
return new ReactRoot(container, true, hydrate);
}
Expand Down
59 changes: 51 additions & 8 deletions packages/react-dom/src/fire/ReactFire.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import type {
FiberRoot,
Batch as FiberRootBatch,
} from 'react-reconciler/src/ReactFiberRoot';
import type {Container} from '../client/ReactDOMHostConfig';

import '../shared/checkReact';
import '../client/ReactDOMClientInjection';
Expand Down Expand Up @@ -165,9 +164,11 @@ setRestoreImplementation(restoreControlledState);
export type DOMContainer =
| (Element & {
_reactRootContainer: ?Root,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
})
| (Document & {
_reactRootContainer: ?Root,
_reactHasBeenPassedToCreateRootDEV: ?boolean,
});

type Batch = FiberRootBatch & {
Expand Down Expand Up @@ -367,7 +368,7 @@ ReactWork.prototype._onCommit = function(): void {
};

function ReactRoot(
container: Container,
container: DOMContainer,
isConcurrent: boolean,
hydrate: boolean,
) {
Expand Down Expand Up @@ -548,12 +549,6 @@ function legacyRenderSubtreeIntoContainer(
forceHydrate: boolean,
callback: ?Function,
) {
// TODO: Ensure all entry points contain this check
invariant(
isValidContainer(container),
'Target container is not a DOM element.',
);

if (__DEV__) {
topLevelUpdateWarnings(container);
}
Expand Down Expand Up @@ -657,6 +652,19 @@ const ReactDOM: Object = {
},

hydrate(element: React$Node, container: DOMContainer, callback: ?Function) {
invariant(
isValidContainer(container),
'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.%s(). This is not supported. ' +
'Did you mean to call root.render(element, {hydrate: true})?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}
// TODO: throw or warn if we couldn't hydrate?
return legacyRenderSubtreeIntoContainer(
null,
Expand All @@ -672,6 +680,19 @@ const ReactDOM: Object = {
container: DOMContainer,
callback: ?Function,
) {
invariant(
isValidContainer(container),
'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.%s(). This is not supported. ' +
'Did you mean to call root.render(element)?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}
return legacyRenderSubtreeIntoContainer(
null,
element,
Expand All @@ -687,6 +708,10 @@ const ReactDOM: Object = {
containerNode: DOMContainer,
callback: ?Function,
) {
invariant(
isValidContainer(containerNode),
'Target container is not a DOM element.',
);
invariant(
parentComponent != null && hasInstance(parentComponent),
'parentComponent must be a valid React Component',
Expand All @@ -706,6 +731,15 @@ const ReactDOM: Object = {
'unmountComponentAtNode(...): Target container is not a DOM element.',
);

if (__DEV__) {
warningWithoutStack(
!container._reactHasBeenPassedToCreateRootDEV,
'You are calling ReactDOM.unmountComponentAtNode() on a container that was previously ' +
'passed to ReactDOM.%s(). This is not supported. Did you mean to call root.unmount()?',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
}

if (container._reactRootContainer) {
if (__DEV__) {
const rootEl = getReactRootElementInContainer(container);
Expand Down Expand Up @@ -810,6 +844,15 @@ function createRoot(container: DOMContainer, options?: RootOptions): ReactRoot {
'%s(...): Target container is not a DOM element.',
functionName,
);
if (__DEV__) {
warningWithoutStack(
!container._reactRootContainer,
'You are calling ReactDOM.%s() on a container that was previously ' +
'passed to ReactDOM.render(). This is not supported.',
enableStableConcurrentModeAPIs ? 'createRoot' : 'unstable_createRoot',
);
container._reactHasBeenPassedToCreateRootDEV = true;
}
const hydrate = options != null && options.hydrate === true;
return new ReactRoot(container, true, hydrate);
}
Expand Down

0 comments on commit 6f0ac82

Please sign in to comment.