Skip to content

Commit 13cb70c

Browse files
committed
Make root.unmount() synchronous
When you unmount a root, the internal state that React stores on the DOM node is immediately cleared. So, we should also synchronously delete the React tree. You should be able to create a new root using the same container.
1 parent aa17cc2 commit 13cb70c

File tree

3 files changed

+86
-6
lines changed

3 files changed

+86
-6
lines changed

packages/react-dom/src/__tests__/ReactDOMRoot-test.js

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ let ReactDOM = require('react-dom');
1414
let ReactDOMServer = require('react-dom/server');
1515
let Scheduler = require('scheduler');
1616
let act;
17+
let useEffect;
1718

1819
describe('ReactDOMRoot', () => {
1920
let container;
@@ -26,6 +27,7 @@ describe('ReactDOMRoot', () => {
2627
ReactDOMServer = require('react-dom/server');
2728
Scheduler = require('scheduler');
2829
act = require('jest-react').act;
30+
useEffect = React.useEffect;
2931
});
3032

3133
it('renders children', () => {
@@ -342,4 +344,62 @@ describe('ReactDOMRoot', () => {
342344
});
343345
expect(container.textContent).toEqual('b');
344346
});
347+
348+
it('unmount is synchronous', async () => {
349+
const root = ReactDOM.createRoot(container);
350+
await act(async () => {
351+
root.render('Hi');
352+
});
353+
expect(container.textContent).toEqual('Hi');
354+
355+
await act(async () => {
356+
root.unmount();
357+
// Should have already unmounted
358+
expect(container.textContent).toEqual('');
359+
});
360+
});
361+
362+
it('throws if an unmounted root is updated', async () => {
363+
const root = ReactDOM.createRoot(container);
364+
await act(async () => {
365+
root.render('Hi');
366+
});
367+
expect(container.textContent).toEqual('Hi');
368+
369+
root.unmount();
370+
371+
expect(() => root.render("I'm back")).toThrow(
372+
'Cannot update an unmounted root.',
373+
);
374+
});
375+
376+
it('warns if root is unmounted inside an effect', async () => {
377+
const container1 = document.createElement('div');
378+
const root1 = ReactDOM.createRoot(container1);
379+
const container2 = document.createElement('div');
380+
const root2 = ReactDOM.createRoot(container2);
381+
382+
function App({step}) {
383+
useEffect(() => {
384+
if (step === 2) {
385+
root2.unmount();
386+
}
387+
}, [step]);
388+
return 'Hi';
389+
}
390+
391+
await act(async () => {
392+
root1.render(<App step={1} />);
393+
});
394+
expect(container1.textContent).toEqual('Hi');
395+
396+
expect(() => {
397+
ReactDOM.flushSync(() => {
398+
root1.render(<App step={2} />);
399+
});
400+
}).toErrorDev(
401+
'Attempted to synchronously unmount a root while React was ' +
402+
'already rendering.',
403+
);
404+
});
345405
});

packages/react-dom/src/client/ReactDOMRoot.js

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import type {FiberRoot} from 'react-reconciler/src/ReactInternalTypes';
1414
export type RootType = {
1515
render(children: ReactNodeList): void,
1616
unmount(): void,
17-
_internalRoot: FiberRoot,
17+
_internalRoot: FiberRoot | null,
1818
...
1919
};
2020

@@ -62,17 +62,23 @@ import {
6262
updateContainer,
6363
findHostInstanceWithNoPortals,
6464
registerMutableSourceForHydration,
65+
flushSync,
66+
isAlreadyRendering,
6567
} from 'react-reconciler/src/ReactFiberReconciler';
6668
import invariant from 'shared/invariant';
6769
import {ConcurrentRoot} from 'react-reconciler/src/ReactRootTags';
6870
import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags';
6971

70-
function ReactDOMRoot(internalRoot) {
72+
function ReactDOMRoot(internalRoot: FiberRoot) {
7173
this._internalRoot = internalRoot;
7274
}
7375

7476
ReactDOMRoot.prototype.render = function(children: ReactNodeList): void {
7577
const root = this._internalRoot;
78+
if (root === null) {
79+
invariant(false, 'Cannot update an unmounted root.');
80+
}
81+
7682
if (__DEV__) {
7783
if (typeof arguments[1] === 'function') {
7884
console.error(
@@ -109,10 +115,23 @@ ReactDOMRoot.prototype.unmount = function(): void {
109115
}
110116
}
111117
const root = this._internalRoot;
112-
const container = root.containerInfo;
113-
updateContainer(null, root, null, () => {
118+
if (root !== null) {
119+
this._internalRoot = null;
120+
const container = root.containerInfo;
121+
if (__DEV__) {
122+
if (isAlreadyRendering()) {
123+
console.error(
124+
'Attempted to synchronously unmount a root while React was already ' +
125+
'rendering. React cannot finish unmounting the root until the ' +
126+
'current render has completed, which may lead to a race condition.',
127+
);
128+
}
129+
}
130+
flushSync(() => {
131+
updateContainer(null, root, null, null);
132+
});
114133
unmarkContainerAsRoot(container);
115-
});
134+
}
116135
};
117136

118137
export function createRoot(

scripts/error-codes/codes.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -396,5 +396,6 @@
396396
"405": "hydrateRoot(...): Target container is not a DOM element.",
397397
"406": "act(...) is not supported in production builds of React.",
398398
"407": "Missing getServerSnapshot, which is required for server-rendered content. Will revert to client rendering.",
399-
"408": "Missing getServerSnapshot, which is required for server-rendered content."
399+
"408": "Missing getServerSnapshot, which is required for server-rendered content.",
400+
"409": "Cannot update an unmounted root."
400401
}

0 commit comments

Comments
 (0)