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

Removed Root API callback params and added warnings #17916

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 29 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
Expand Up @@ -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>);
Expand Down
19 changes: 14 additions & 5 deletions packages/react-dom/src/client/ReactDOMLegacy.js
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
50 changes: 18 additions & 32 deletions packages/react-dom/src/client/ReactDOMRoot.js
Expand Up @@ -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,
...
};
Expand Down Expand Up @@ -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 (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 (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();
}
});
};

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