Skip to content

Commit d3e0869

Browse files
acdliteacdlite
authored
Make root.unmount() synchronous (#22444)
* Move flushSync warning to React DOM When you call in `flushSync` from an effect, React fires a warning. I've moved the implementation of this warning out of the reconciler and into React DOM. `flushSync` is a renderer API, not an isomorphic API, because it has behavior that was designed specifically for the constraints of React DOM. The equivalent API in a different renderer may not be the same. For example, React Native has a different threading model than the browser, so it might not make sense to expose a `flushSync` API to the JavaScript thread. * 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 2cc6d79 commit d3e0869

File tree

13 files changed

+177
-79
lines changed

13 files changed

+177
-79
lines changed

‎packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -246,9 +246,9 @@ describe('StoreStressConcurrent', () => {
246246
// 1. Capture the expected render result.
247247
const snapshots = [];
248248
let container = document.createElement('div');
249-
// $FlowFixMe
250-
let root = ReactDOM.createRoot(container);
251249
for (let i = 0; i < steps.length; i++) {
250+
// $FlowFixMe
251+
const root = ReactDOM.createRoot(container);
252252
act(() => root.render(<Root>{steps[i]}</Root>));
253253
// We snapshot each step once so it doesn't regress.
254254
snapshots.push(print(store));
@@ -320,7 +320,7 @@ describe('StoreStressConcurrent', () => {
320320
for (let j = 0; j < steps.length; j++) {
321321
container = document.createElement('div');
322322
// $FlowFixMe
323-
root = ReactDOM.createRoot(container);
323+
const root = ReactDOM.createRoot(container);
324324
act(() => root.render(<Root>{steps[i]}</Root>));
325325
expect(print(store)).toMatch(snapshots[i]);
326326
act(() => root.render(<Root>{steps[j]}</Root>));
@@ -337,7 +337,7 @@ describe('StoreStressConcurrent', () => {
337337
for (let j = 0; j < steps.length; j++) {
338338
container = document.createElement('div');
339339
// $FlowFixMe
340-
root = ReactDOM.createRoot(container);
340+
const root = ReactDOM.createRoot(container);
341341
act(() =>
342342
root.render(
343343
<Root>
@@ -408,9 +408,9 @@ describe('StoreStressConcurrent', () => {
408408
// This is the only step where we use Jest snapshots.
409409
const snapshots = [];
410410
let container = document.createElement('div');
411-
// $FlowFixMe
412-
let root = ReactDOM.createRoot(container);
413411
for (let i = 0; i < steps.length; i++) {
412+
// $FlowFixMe
413+
const root = ReactDOM.createRoot(container);
414414
act(() =>
415415
root.render(
416416
<Root>
@@ -512,6 +512,8 @@ describe('StoreStressConcurrent', () => {
512512

513513
// 2. Verify check Suspense can render same steps as initial fallback content.
514514
for (let i = 0; i < steps.length; i++) {
515+
// $FlowFixMe
516+
const root = ReactDOM.createRoot(container);
515517
act(() =>
516518
root.render(
517519
<Root>
@@ -536,7 +538,7 @@ describe('StoreStressConcurrent', () => {
536538
// Always start with a fresh container and steps[i].
537539
container = document.createElement('div');
538540
// $FlowFixMe
539-
root = ReactDOM.createRoot(container);
541+
const root = ReactDOM.createRoot(container);
540542
act(() =>
541543
root.render(
542544
<Root>
@@ -582,7 +584,7 @@ describe('StoreStressConcurrent', () => {
582584
// Always start with a fresh container and steps[i].
583585
container = document.createElement('div');
584586
// $FlowFixMe
585-
root = ReactDOM.createRoot(container);
587+
const root = ReactDOM.createRoot(container);
586588
act(() =>
587589
root.render(
588590
<Root>
@@ -640,7 +642,7 @@ describe('StoreStressConcurrent', () => {
640642
// Always start with a fresh container and steps[i].
641643
container = document.createElement('div');
642644
// $FlowFixMe
643-
root = ReactDOM.createRoot(container);
645+
const root = ReactDOM.createRoot(container);
644646
act(() =>
645647
root.render(
646648
<Root>
@@ -690,7 +692,7 @@ describe('StoreStressConcurrent', () => {
690692
// Always start with a fresh container and steps[i].
691693
container = document.createElement('div');
692694
// $FlowFixMe
693-
root = ReactDOM.createRoot(container);
695+
const root = ReactDOM.createRoot(container);
694696
act(() =>
695697
root.render(
696698
<Root>
@@ -744,7 +746,7 @@ describe('StoreStressConcurrent', () => {
744746
// Always start with a fresh container and steps[i].
745747
container = document.createElement('div');
746748
// $FlowFixMe
747-
root = ReactDOM.createRoot(container);
749+
const root = ReactDOM.createRoot(container);
748750
act(() =>
749751
root.render(
750752
<Root>
@@ -897,9 +899,9 @@ describe('StoreStressConcurrent', () => {
897899
// This is the only step where we use Jest snapshots.
898900
const snapshots = [];
899901
let container = document.createElement('div');
900-
// $FlowFixMe
901-
let root = ReactDOM.createRoot(container);
902902
for (let i = 0; i < steps.length; i++) {
903+
// $FlowFixMe
904+
const root = ReactDOM.createRoot(container);
903905
act(() =>
904906
root.render(
905907
<Root>
@@ -922,6 +924,8 @@ describe('StoreStressConcurrent', () => {
922924
// which is different from the snapshots above. So we take more snapshots.
923925
const fallbackSnapshots = [];
924926
for (let i = 0; i < steps.length; i++) {
927+
// $FlowFixMe
928+
const root = ReactDOM.createRoot(container);
925929
act(() =>
926930
root.render(
927931
<Root>
@@ -1055,7 +1059,7 @@ describe('StoreStressConcurrent', () => {
10551059
// Always start with a fresh container and steps[i].
10561060
container = document.createElement('div');
10571061
// $FlowFixMe
1058-
root = ReactDOM.createRoot(container);
1062+
const root = ReactDOM.createRoot(container);
10591063
act(() =>
10601064
root.render(
10611065
<Root>
@@ -1107,7 +1111,7 @@ describe('StoreStressConcurrent', () => {
11071111
// Always start with a fresh container and steps[i].
11081112
container = document.createElement('div');
11091113
// $FlowFixMe
1110-
root = ReactDOM.createRoot(container);
1114+
const root = ReactDOM.createRoot(container);
11111115
act(() =>
11121116
root.render(
11131117
<Root>
@@ -1174,7 +1178,7 @@ describe('StoreStressConcurrent', () => {
11741178
// Always start with a fresh container and steps[i].
11751179
container = document.createElement('div');
11761180
// $FlowFixMe
1177-
root = ReactDOM.createRoot(container);
1181+
const root = ReactDOM.createRoot(container);
11781182
act(() =>
11791183
root.render(
11801184
<Root>
@@ -1226,7 +1230,7 @@ describe('StoreStressConcurrent', () => {
12261230
// Always start with a fresh container and steps[i].
12271231
container = document.createElement('div');
12281232
// $FlowFixMe
1229-
root = ReactDOM.createRoot(container);
1233+
const root = ReactDOM.createRoot(container);
12301234
act(() =>
12311235
root.render(
12321236
<Root>
@@ -1278,7 +1282,7 @@ describe('StoreStressConcurrent', () => {
12781282
// Always start with a fresh container and steps[i].
12791283
container = document.createElement('div');
12801284
// $FlowFixMe
1281-
root = ReactDOM.createRoot(container);
1285+
const root = ReactDOM.createRoot(container);
12821286
act(() =>
12831287
root.render(
12841288
<Root>

‎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/ReactDOM.js‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
2323
import {
2424
batchedUpdates,
2525
discreteUpdates,
26-
flushSync,
27-
flushSyncWithoutWarningIfAlreadyRendering,
26+
flushSync as flushSyncWithoutWarningIfAlreadyRendering,
27+
isAlreadyRendering,
2828
flushControlled,
2929
injectIntoDevTools,
3030
attemptSynchronousHydration,
@@ -163,6 +163,25 @@ const Internals = {
163163
],
164164
};
165165

166+
// Overload the definition to the two valid signatures.
167+
// Warning, this opts-out of checking the function body.
168+
declare function flushSync<R>(fn: () => R): R;
169+
// eslint-disable-next-line no-redeclare
170+
declare function flushSync(): void;
171+
// eslint-disable-next-line no-redeclare
172+
function flushSync(fn) {
173+
if (__DEV__) {
174+
if (isAlreadyRendering()) {
175+
console.error(
176+
'flushSync was called from inside a lifecycle method. React cannot ' +
177+
'flush when React is already rendering. Consider moving this call to ' +
178+
'a scheduler task or micro task.',
179+
);
180+
}
181+
}
182+
return flushSyncWithoutWarningIfAlreadyRendering(fn);
183+
}
184+
166185
export {
167186
createPortal,
168187
batchedUpdates as unstable_batchedUpdates,

‎packages/react-dom/src/client/ReactDOMLegacy.js‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import {
2929
createContainer,
3030
findHostInstanceWithNoPortals,
3131
updateContainer,
32-
flushSyncWithoutWarningIfAlreadyRendering,
32+
flushSync,
3333
getPublicRootInstance,
3434
findHostInstance,
3535
findHostInstanceWithWarning,
@@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
174174
};
175175
}
176176
// Initial mount should not be batched.
177-
flushSyncWithoutWarningIfAlreadyRendering(() => {
177+
flushSync(() => {
178178
updateContainer(children, fiberRoot, parentComponent, callback);
179179
});
180180
} else {
@@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) {
357357
}
358358

359359
// Unmount should not be batched.
360-
flushSyncWithoutWarningIfAlreadyRendering(() => {
360+
flushSync(() => {
361361
legacyRenderSubtreeIntoContainer(null, null, container, false, () => {
362362
// $FlowFixMe This should probably use `delete container._reactRootContainer`
363363
container._reactRootContainer = null;

‎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(

‎packages/react-noop-renderer/src/createReactNoop.js‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -921,6 +921,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
921921
return children;
922922
}
923923

924+
function flushSync<R>(fn: () => R): R {
925+
if (__DEV__) {
926+
if (NoopRenderer.isAlreadyRendering()) {
927+
console.error(
928+
'flushSync was called from inside a lifecycle method. React cannot ' +
929+
'flush when React is already rendering. Consider moving this call to ' +
930+
'a scheduler task or micro task.',
931+
);
932+
}
933+
}
934+
return NoopRenderer.flushSync(fn);
935+
}
936+
924937
let idCounter = 0;
925938

926939
const ReactNoop = {
@@ -1136,7 +1149,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
11361149
}
11371150
},
11381151

1139-
flushSync: NoopRenderer.flushSync,
1152+
flushSync,
11401153
flushPassiveEffects: NoopRenderer.flushPassiveEffects,
11411154

11421155
// Logs the current state of the tree.

0 commit comments

Comments
 (0)