From 7d1fed5128cadbf30dd2ca822b67753f8474209f Mon Sep 17 00:00:00 2001
From: Brian Vaughn <bvaughn@fb.com>
Date: Mon, 27 Jan 2020 11:49:10 -0800
Subject: [PATCH] Removed Root API callback params and added warnings

---
 .../src/__tests__/ReactDOMRoot-test.js        | 29 +++++++++++
 .../react-dom/src/client/ReactDOMLegacy.js    | 19 +++++--
 packages/react-dom/src/client/ReactDOMRoot.js | 50 +++++++------------
 3 files changed, 61 insertions(+), 37 deletions(-)

diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js
index 99743902a75e0..a6bd6c3a330d8 100644
--- a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js
@@ -40,6 +40,35 @@ describe('ReactDOMRoot', () => {
     expect(container.textContent).toEqual('Hi');
   });
 
+  it('warns if a callback parameter is provided to render', () => {
+    const callback = jest.fn();
+    const root = ReactDOM.createRoot(container);
+    expect(() =>
+      root.render(<div>Hi</div>, callback),
+    ).toErrorDev(
+      'render(...): does not support the second callback argument. ' +
+        'To execute a side effect after rendering, declare it in a component body with useEffect().',
+      {withoutStack: true},
+    );
+    Scheduler.unstable_flushAll();
+    expect(callback).not.toHaveBeenCalled();
+  });
+
+  it('warns if a callback parameter is provided to unmount', () => {
+    const callback = jest.fn();
+    const root = ReactDOM.createRoot(container);
+    root.render(<div>Hi</div>);
+    expect(() =>
+      root.unmount(callback),
+    ).toErrorDev(
+      'unmount(...): does not support a callback argument. ' +
+        'To execute a side effect after rendering, declare it in a component body with useEffect().',
+      {withoutStack: true},
+    );
+    Scheduler.unstable_flushAll();
+    expect(callback).not.toHaveBeenCalled();
+  });
+
   it('unmounts children', () => {
     const root = ReactDOM.createRoot(container);
     root.render(<div>Hi</div>);
diff --git a/packages/react-dom/src/client/ReactDOMLegacy.js b/packages/react-dom/src/client/ReactDOMLegacy.js
index 3da5e468c2679..519a0be3bb551 100644
--- a/packages/react-dom/src/client/ReactDOMLegacy.js
+++ b/packages/react-dom/src/client/ReactDOMLegacy.js
@@ -16,11 +16,7 @@ import {
   isContainerMarkedAsRoot,
   unmarkContainerAsRoot,
 } from './ReactDOMComponentTree';
-import {
-  createLegacyRoot,
-  isValidContainer,
-  warnOnInvalidCallback,
-} from './ReactDOMRoot';
+import {createLegacyRoot, isValidContainer} from './ReactDOMRoot';
 import {ROOT_ATTRIBUTE_NAME} from '../shared/DOMProperty';
 import {
   DOCUMENT_NODE,
@@ -163,6 +159,19 @@ function legacyCreateRootFromDOMContainer(
   );
 }
 
+function warnOnInvalidCallback(callback: mixed, callerName: string): void {
+  if (__DEV__) {
+    if (callback !== null && typeof callback !== 'function') {
+      console.error(
+        '%s(...): Expected the last optional `callback` argument to be a ' +
+          'function. Instead received: %s.',
+        callerName,
+        callback,
+      );
+    }
+  }
+}
+
 function legacyRenderSubtreeIntoContainer(
   parentComponent: ?React$Component<any, any>,
   children: ReactNodeList,
diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js
index b11430b8b3c0f..de38259d1428c 100644
--- a/packages/react-dom/src/client/ReactDOMRoot.js
+++ b/packages/react-dom/src/client/ReactDOMRoot.js
@@ -15,8 +15,8 @@ import type {ReactNodeList} from 'shared/ReactTypes';
 import type {FiberRoot} from 'react-reconciler/src/ReactFiberRoot';
 
 export type RootType = {
-  render(children: ReactNodeList, callback: ?() => mixed): void,
-  unmount(callback: ?() => mixed): void,
+  render(children: ReactNodeList): void,
+  unmount(): void,
   _internalRoot: FiberRoot,
   ...
 };
@@ -62,30 +62,32 @@ function ReactDOMBlockingRoot(
 
 ReactDOMRoot.prototype.render = ReactDOMBlockingRoot.prototype.render = function(
   children: ReactNodeList,
-  callback: ?() => mixed,
 ): void {
-  const root = this._internalRoot;
-  const cb = callback === undefined ? null : callback;
   if (__DEV__) {
-    warnOnInvalidCallback(cb, 'render');
+    if (arguments.length > 1 && typeof arguments[1] === 'function') {
+      console.error(
+        'render(...): does not support the second callback argument. ' +
+          'To execute a side effect after rendering, declare it in a component body with useEffect().',
+      );
+    }
   }
-  updateContainer(children, root, null, cb);
+  const root = this._internalRoot;
+  updateContainer(children, root, null, null);
 };
 
-ReactDOMRoot.prototype.unmount = ReactDOMBlockingRoot.prototype.unmount = function(
-  callback: ?() => mixed,
-): void {
-  const root = this._internalRoot;
-  const cb = callback === undefined ? null : callback;
+ReactDOMRoot.prototype.unmount = ReactDOMBlockingRoot.prototype.unmount = function(): void {
   if (__DEV__) {
-    warnOnInvalidCallback(cb, 'render');
+    if (arguments.length > 0 && typeof arguments[0] === 'function') {
+      console.error(
+        'unmount(...): does not support a callback argument. ' +
+          'To execute a side effect after rendering, declare it in a component body with useEffect().',
+      );
+    }
   }
+  const root = this._internalRoot;
   const container = root.containerInfo;
   updateContainer(null, root, null, () => {
     unmarkContainerAsRoot(container);
-    if (cb !== null) {
-      cb();
-    }
   });
 };
 
@@ -152,22 +154,6 @@ export function isValidContainer(node: mixed): boolean {
   );
 }
 
-export function warnOnInvalidCallback(
-  callback: mixed,
-  callerName: string,
-): void {
-  if (__DEV__) {
-    if (callback !== null && typeof callback !== 'function') {
-      console.error(
-        '%s(...): Expected the last optional `callback` argument to be a ' +
-          'function. Instead received: %s.',
-        callerName,
-        callback,
-      );
-    }
-  }
-}
-
 function warnIfReactDOMContainerInDEV(container) {
   if (__DEV__) {
     if (isContainerMarkedAsRoot(container)) {