@samitouri / QOS-React-2 / commits / d3e0869324

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.

Andrew Clark committed Sep 27, 2021 at 17:04 UTC d3e0869324267dc62b50ee02f747f5f0a5f5c656
13 files changed +177 -79
packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js
+22 -18
@@ -246,9 +246,9 @@ describe('StoreStressConcurrent', () => {
246 // 1. Capture the expected render result.
247 const snapshots = [];
248 let container = document.createElement('div');
249 - // $FlowFixMe
250 - let root = ReactDOM.createRoot(container);
249 for (let i = 0; i < steps.length; i++) {
250 + // $FlowFixMe
251 + const root = ReactDOM.createRoot(container);
252 act(() => root.render(<Root>{steps[i]}</Root>));
253 // We snapshot each step once so it doesn't regress.
254 snapshots.push(print(store));
@@ -320,7 +320,7 @@ describe('StoreStressConcurrent', () => {
320 for (let j = 0; j < steps.length; j++) {
321 container = document.createElement('div');
322 // $FlowFixMe
323 - root = ReactDOM.createRoot(container);
323 + const root = ReactDOM.createRoot(container);
324 act(() => root.render(<Root>{steps[i]}</Root>));
325 expect(print(store)).toMatch(snapshots[i]);
326 act(() => root.render(<Root>{steps[j]}</Root>));
@@ -337,7 +337,7 @@ describe('StoreStressConcurrent', () => {
337 for (let j = 0; j < steps.length; j++) {
338 container = document.createElement('div');
339 // $FlowFixMe
340 - root = ReactDOM.createRoot(container);
340 + const root = ReactDOM.createRoot(container);
341 act(() =>
342 root.render(
343 <Root>
@@ -408,9 +408,9 @@ describe('StoreStressConcurrent', () => {
408 // This is the only step where we use Jest snapshots.
409 const snapshots = [];
410 let container = document.createElement('div');
411 - // $FlowFixMe
412 - let root = ReactDOM.createRoot(container);
411 for (let i = 0; i < steps.length; i++) {
412 + // $FlowFixMe
413 + const root = ReactDOM.createRoot(container);
414 act(() =>
415 root.render(
416 <Root>
@@ -512,6 +512,8 @@ describe('StoreStressConcurrent', () => {
512
513 // 2. Verify check Suspense can render same steps as initial fallback content.
514 for (let i = 0; i < steps.length; i++) {
515 + // $FlowFixMe
516 + const root = ReactDOM.createRoot(container);
517 act(() =>
518 root.render(
519 <Root>
@@ -536,7 +538,7 @@ describe('StoreStressConcurrent', () => {
538 // Always start with a fresh container and steps[i].
539 container = document.createElement('div');
540 // $FlowFixMe
539 - root = ReactDOM.createRoot(container);
541 + const root = ReactDOM.createRoot(container);
542 act(() =>
543 root.render(
544 <Root>
@@ -582,7 +584,7 @@ describe('StoreStressConcurrent', () => {
584 // Always start with a fresh container and steps[i].
585 container = document.createElement('div');
586 // $FlowFixMe
585 - root = ReactDOM.createRoot(container);
587 + const root = ReactDOM.createRoot(container);
588 act(() =>
589 root.render(
590 <Root>
@@ -640,7 +642,7 @@ describe('StoreStressConcurrent', () => {
642 // Always start with a fresh container and steps[i].
643 container = document.createElement('div');
644 // $FlowFixMe
643 - root = ReactDOM.createRoot(container);
645 + const root = ReactDOM.createRoot(container);
646 act(() =>
647 root.render(
648 <Root>
@@ -690,7 +692,7 @@ describe('StoreStressConcurrent', () => {
692 // Always start with a fresh container and steps[i].
693 container = document.createElement('div');
694 // $FlowFixMe
693 - root = ReactDOM.createRoot(container);
695 + const root = ReactDOM.createRoot(container);
696 act(() =>
697 root.render(
698 <Root>
@@ -744,7 +746,7 @@ describe('StoreStressConcurrent', () => {
746 // Always start with a fresh container and steps[i].
747 container = document.createElement('div');
748 // $FlowFixMe
747 - root = ReactDOM.createRoot(container);
749 + const root = ReactDOM.createRoot(container);
750 act(() =>
751 root.render(
752 <Root>
@@ -897,9 +899,9 @@ describe('StoreStressConcurrent', () => {
899 // This is the only step where we use Jest snapshots.
900 const snapshots = [];
901 let container = document.createElement('div');
900 - // $FlowFixMe
901 - let root = ReactDOM.createRoot(container);
902 for (let i = 0; i < steps.length; i++) {
903 + // $FlowFixMe
904 + const root = ReactDOM.createRoot(container);
905 act(() =>
906 root.render(
907 <Root>
@@ -922,6 +924,8 @@ describe('StoreStressConcurrent', () => {
924 // which is different from the snapshots above. So we take more snapshots.
925 const fallbackSnapshots = [];
926 for (let i = 0; i < steps.length; i++) {
927 + // $FlowFixMe
928 + const root = ReactDOM.createRoot(container);
929 act(() =>
930 root.render(
931 <Root>
@@ -1055,7 +1059,7 @@ describe('StoreStressConcurrent', () => {
1059 // Always start with a fresh container and steps[i].
1060 container = document.createElement('div');
1061 // $FlowFixMe
1058 - root = ReactDOM.createRoot(container);
1062 + const root = ReactDOM.createRoot(container);
1063 act(() =>
1064 root.render(
1065 <Root>
@@ -1107,7 +1111,7 @@ describe('StoreStressConcurrent', () => {
1111 // Always start with a fresh container and steps[i].
1112 container = document.createElement('div');
1113 // $FlowFixMe
1110 - root = ReactDOM.createRoot(container);
1114 + const root = ReactDOM.createRoot(container);
1115 act(() =>
1116 root.render(
1117 <Root>
@@ -1174,7 +1178,7 @@ describe('StoreStressConcurrent', () => {
1178 // Always start with a fresh container and steps[i].
1179 container = document.createElement('div');
1180 // $FlowFixMe
1177 - root = ReactDOM.createRoot(container);
1181 + const root = ReactDOM.createRoot(container);
1182 act(() =>
1183 root.render(
1184 <Root>
@@ -1226,7 +1230,7 @@ describe('StoreStressConcurrent', () => {
1230 // Always start with a fresh container and steps[i].
1231 container = document.createElement('div');
1232 // $FlowFixMe
1229 - root = ReactDOM.createRoot(container);
1233 + const root = ReactDOM.createRoot(container);
1234 act(() =>
1235 root.render(
1236 <Root>
@@ -1278,7 +1282,7 @@ describe('StoreStressConcurrent', () => {
1282 // Always start with a fresh container and steps[i].
1283 container = document.createElement('div');
1284 // $FlowFixMe
1281 - root = ReactDOM.createRoot(container);
1285 + const root = ReactDOM.createRoot(container);
1286 act(() =>
1287 root.render(
1288 <Root>
packages/react-dom/src/__tests__/ReactDOMRoot-test.js
+60
@@ -14,6 +14,7 @@ let ReactDOM = require('react-dom');
14 let ReactDOMServer = require('react-dom/server');
15 let Scheduler = require('scheduler');
16 let act;
17 +let useEffect;
18
19 describe('ReactDOMRoot', () => {
20 let container;
@@ -26,6 +27,7 @@ describe('ReactDOMRoot', () => {
27 ReactDOMServer = require('react-dom/server');
28 Scheduler = require('scheduler');
29 act = require('jest-react').act;
30 + useEffect = React.useEffect;
31 });
32
33 it('renders children', () => {
@@ -342,4 +344,62 @@ describe('ReactDOMRoot', () => {
344 });
345 expect(container.textContent).toEqual('b');
346 });
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 + });
405 });
packages/react-dom/src/client/ReactDOM.js
+21 -2
@@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
23 import {
24 batchedUpdates,
25 discreteUpdates,
26 - flushSync,
27 - flushSyncWithoutWarningIfAlreadyRendering,
26 + flushSync as flushSyncWithoutWarningIfAlreadyRendering,
27 + isAlreadyRendering,
28 flushControlled,
29 injectIntoDevTools,
30 attemptSynchronousHydration,
@@ -163,6 +163,25 @@ const Internals = {
163 ],
164 };
165
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 +
185 export {
186 createPortal,
187 batchedUpdates as unstable_batchedUpdates,
packages/react-dom/src/client/ReactDOMLegacy.js
+3 -3
@@ -29,7 +29,7 @@ import {
29 createContainer,
30 findHostInstanceWithNoPortals,
31 updateContainer,
32 - flushSyncWithoutWarningIfAlreadyRendering,
32 + flushSync,
33 getPublicRootInstance,
34 findHostInstance,
35 findHostInstanceWithWarning,
@@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
174 };
175 }
176 // Initial mount should not be batched.
177 - flushSyncWithoutWarningIfAlreadyRendering(() => {
177 + flushSync(() => {
178 updateContainer(children, fiberRoot, parentComponent, callback);
179 });
180 } else {
@@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) {
357 }
358
359 // Unmount should not be batched.
360 - flushSyncWithoutWarningIfAlreadyRendering(() => {
360 + flushSync(() => {
361 legacyRenderSubtreeIntoContainer(null, null, container, false, () => {
362 // $FlowFixMe This should probably use `delete container._reactRootContainer`
363 container._reactRootContainer = null;
packages/react-dom/src/client/ReactDOMRoot.js
+24 -5
@@ -14,7 +14,7 @@ import type {FiberRoot} from 'react-reconciler/src/ReactInternalTypes';
14 export type RootType = {
15 render(children: ReactNodeList): void,
16 unmount(): void,
17 - _internalRoot: FiberRoot,
17 + _internalRoot: FiberRoot | null,
18 ...
19 };
20
@@ -62,17 +62,23 @@ import {
62 updateContainer,
63 findHostInstanceWithNoPortals,
64 registerMutableSourceForHydration,
65 + flushSync,
66 + isAlreadyRendering,
67 } from 'react-reconciler/src/ReactFiberReconciler';
68 import invariant from 'shared/invariant';
69 import {ConcurrentRoot} from 'react-reconciler/src/ReactRootTags';
70 import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags';
71
70 -function ReactDOMRoot(internalRoot) {
72 +function ReactDOMRoot(internalRoot: FiberRoot) {
73 this._internalRoot = internalRoot;
74 }
75
76 ReactDOMRoot.prototype.render = function(children: ReactNodeList): void {
77 const root = this._internalRoot;
78 + if (root === null) {
79 + invariant(false, 'Cannot update an unmounted root.');
80 + }
81 +
82 if (__DEV__) {
83 if (typeof arguments[1] === 'function') {
84 console.error(
@@ -109,10 +115,23 @@ ReactDOMRoot.prototype.unmount = function(): void {
115 }
116 }
117 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 + });
133 unmarkContainerAsRoot(container);
115 - });
134 + }
135 };
136
137 export function createRoot(
packages/react-noop-renderer/src/createReactNoop.js
+14 -1
@@ -921,6 +921,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
921 return children;
922 }
923
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 +
937 let idCounter = 0;
938
939 const ReactNoop = {
@@ -1136,7 +1149,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
1149 }
1150 },
1151
1139 - flushSync: NoopRenderer.flushSync,
1152 + flushSync,
1153 flushPassiveEffects: NoopRenderer.flushPassiveEffects,
1154
1155 // Logs the current state of the tree.
packages/react-reconciler/src/ReactFiberReconciler.js
+5 -5
@@ -22,7 +22,7 @@ import {
22 discreteUpdates as discreteUpdates_old,
23 flushControlled as flushControlled_old,
24 flushSync as flushSync_old,
25 - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_old,
25 + isAlreadyRendering as isAlreadyRendering_old,
26 flushPassiveEffects as flushPassiveEffects_old,
27 getPublicRootInstance as getPublicRootInstance_old,
28 attemptSynchronousHydration as attemptSynchronousHydration_old,
@@ -59,7 +59,7 @@ import {
59 discreteUpdates as discreteUpdates_new,
60 flushControlled as flushControlled_new,
61 flushSync as flushSync_new,
62 - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_new,
62 + isAlreadyRendering as isAlreadyRendering_new,
63 flushPassiveEffects as flushPassiveEffects_new,
64 getPublicRootInstance as getPublicRootInstance_new,
65 attemptSynchronousHydration as attemptSynchronousHydration_new,
@@ -107,9 +107,9 @@ export const flushControlled = enableNewReconciler
107 ? flushControlled_new
108 : flushControlled_old;
109 export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old;
110 -export const flushSyncWithoutWarningIfAlreadyRendering = enableNewReconciler
111 - ? flushSyncWithoutWarningIfAlreadyRendering_new
112 - : flushSyncWithoutWarningIfAlreadyRendering_old;
110 +export const isAlreadyRendering = enableNewReconciler
111 + ? isAlreadyRendering_new
112 + : isAlreadyRendering_old;
113 export const flushPassiveEffects = enableNewReconciler
114 ? flushPassiveEffects_new
115 : flushPassiveEffects_old;
packages/react-reconciler/src/ReactFiberReconciler.new.js
+2 -2
@@ -53,10 +53,10 @@ import {
53 flushRoot,
54 batchedUpdates,
55 flushSync,
56 + isAlreadyRendering,
57 flushControlled,
58 deferredUpdates,
59 discreteUpdates,
59 - flushSyncWithoutWarningIfAlreadyRendering,
60 flushPassiveEffects,
61 } from './ReactFiberWorkLoop.new';
62 import {
@@ -330,7 +330,7 @@ export {
330 discreteUpdates,
331 flushControlled,
332 flushSync,
333 - flushSyncWithoutWarningIfAlreadyRendering,
333 + isAlreadyRendering,
334 flushPassiveEffects,
335 };
336
packages/react-reconciler/src/ReactFiberReconciler.old.js
+2 -2
@@ -53,10 +53,10 @@ import {
53 flushRoot,
54 batchedUpdates,
55 flushSync,
56 + isAlreadyRendering,
57 flushControlled,
58 deferredUpdates,
59 discreteUpdates,
59 - flushSyncWithoutWarningIfAlreadyRendering,
60 flushPassiveEffects,
61 } from './ReactFiberWorkLoop.old';
62 import {
@@ -330,7 +330,7 @@ export {
330 discreteUpdates,
331 flushControlled,
332 flushSync,
333 - flushSyncWithoutWarningIfAlreadyRendering,
333 + isAlreadyRendering,
334 flushPassiveEffects,
335 };
336
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+10 -20
@@ -1191,11 +1191,11 @@ export function discreteUpdates<A, B, C, D, R>(
1191
1192 // Overload the definition to the two valid signatures.
1193 // Warning, this opts-out of checking the function body.
1194 -declare function flushSyncWithoutWarningIfAlreadyRendering<R>(fn: () => R): R;
1194 +declare function flushSync<R>(fn: () => R): R;
1195 // eslint-disable-next-line no-redeclare
1196 -declare function flushSyncWithoutWarningIfAlreadyRendering(): void;
1196 +declare function flushSync(): void;
1197 // eslint-disable-next-line no-redeclare
1198 -export function flushSyncWithoutWarningIfAlreadyRendering(fn) {
1198 +export function flushSync(fn) {
1199 // In legacy mode, we flush pending passive effects at the beginning of the
1200 // next event, not at the end of the previous one.
1201 if (
@@ -1232,23 +1232,13 @@ export function flushSyncWithoutWarningIfAlreadyRendering(fn) {
1232 }
1233 }
1234
1235 -// Overload the definition to the two valid signatures.
1236 -// Warning, this opts-out of checking the function body.
1237 -declare function flushSync<R>(fn: () => R): R;
1238 -// eslint-disable-next-line no-redeclare
1239 -declare function flushSync(): void;
1240 -// eslint-disable-next-line no-redeclare
1241 -export function flushSync(fn) {
1242 - if (__DEV__) {
1243 - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
1244 - console.error(
1245 - 'flushSync was called from inside a lifecycle method. React cannot ' +
1246 - 'flush when React is already rendering. Consider moving this call to ' +
1247 - 'a scheduler task or micro task.',
1248 - );
1249 - }
1250 - }
1251 - return flushSyncWithoutWarningIfAlreadyRendering(fn);
1235 +export function isAlreadyRendering() {
1236 + // Used by the renderer to print a warning if certain APIs are called from
1237 + // the wrong context.
1238 + return (
1239 + __DEV__ &&
1240 + (executionContext & (RenderContext | CommitContext)) !== NoContext
1241 + );
1242 }
1243
1244 export function flushControlled(fn: () => mixed): void {
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+10 -20
@@ -1191,11 +1191,11 @@ export function discreteUpdates<A, B, C, D, R>(
1191
1192 // Overload the definition to the two valid signatures.
1193 // Warning, this opts-out of checking the function body.
1194 -declare function flushSyncWithoutWarningIfAlreadyRendering<R>(fn: () => R): R;
1194 +declare function flushSync<R>(fn: () => R): R;
1195 // eslint-disable-next-line no-redeclare
1196 -declare function flushSyncWithoutWarningIfAlreadyRendering(): void;
1196 +declare function flushSync(): void;
1197 // eslint-disable-next-line no-redeclare
1198 -export function flushSyncWithoutWarningIfAlreadyRendering(fn) {
1198 +export function flushSync(fn) {
1199 // In legacy mode, we flush pending passive effects at the beginning of the
1200 // next event, not at the end of the previous one.
1201 if (
@@ -1232,23 +1232,13 @@ export function flushSyncWithoutWarningIfAlreadyRendering(fn) {
1232 }
1233 }
1234
1235 -// Overload the definition to the two valid signatures.
1236 -// Warning, this opts-out of checking the function body.
1237 -declare function flushSync<R>(fn: () => R): R;
1238 -// eslint-disable-next-line no-redeclare
1239 -declare function flushSync(): void;
1240 -// eslint-disable-next-line no-redeclare
1241 -export function flushSync(fn) {
1242 - if (__DEV__) {
1243 - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) {
1244 - console.error(
1245 - 'flushSync was called from inside a lifecycle method. React cannot ' +
1246 - 'flush when React is already rendering. Consider moving this call to ' +
1247 - 'a scheduler task or micro task.',
1248 - );
1249 - }
1250 - }
1251 - return flushSyncWithoutWarningIfAlreadyRendering(fn);
1235 +export function isAlreadyRendering() {
1236 + // Used by the renderer to print a warning if certain APIs are called from
1237 + // the wrong context.
1238 + return (
1239 + __DEV__ &&
1240 + (executionContext & (RenderContext | CommitContext)) !== NoContext
1241 + );
1242 }
1243
1244 export function flushControlled(fn: () => mixed): void {
packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
+2
@@ -6,6 +6,8 @@ let useState;
6 let useEffect;
7 let startTransition;
8
9 +// TODO: Migrate tests to React DOM instead of React Noop
10 +
11 describe('ReactFlushSync', () => {
12 beforeEach(() => {
13 jest.resetModules();
scripts/error-codes/codes.json
+2 -1
@@ -396,5 +396,6 @@
396 "405": "hydrateRoot(...): Target container is not a DOM element.",
397 "406": "act(...) is not supported in production builds of React.",
398 "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."
401 }