From d3e0869324267dc62b50ee02f747f5f0a5f5c656 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 27 Sep 2021 17:04:39 -0400 Subject: [PATCH] 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. --- .../storeStressTestConcurrent-test.js | 40 +++++++------ .../src/__tests__/ReactDOMRoot-test.js | 60 +++++++++++++++++++ packages/react-dom/src/client/ReactDOM.js | 23 ++++++- .../react-dom/src/client/ReactDOMLegacy.js | 6 +- packages/react-dom/src/client/ReactDOMRoot.js | 29 +++++++-- .../src/createReactNoop.js | 15 ++++- .../src/ReactFiberReconciler.js | 10 ++-- .../src/ReactFiberReconciler.new.js | 4 +- .../src/ReactFiberReconciler.old.js | 4 +- .../src/ReactFiberWorkLoop.new.js | 30 ++++------ .../src/ReactFiberWorkLoop.old.js | 30 ++++------ .../src/__tests__/ReactFlushSync-test.js | 2 + scripts/error-codes/codes.json | 3 +- 13 files changed, 177 insertions(+), 79 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js index 826a72552a..0f4a373e16 100644 --- a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js @@ -246,9 +246,9 @@ describe('StoreStressConcurrent', () => { // 1. Capture the expected render result. const snapshots = []; let container = document.createElement('div'); - // $FlowFixMe - let root = ReactDOM.createRoot(container); for (let i = 0; i < steps.length; i++) { + // $FlowFixMe + const root = ReactDOM.createRoot(container); act(() => root.render({steps[i]})); // We snapshot each step once so it doesn't regress. snapshots.push(print(store)); @@ -320,7 +320,7 @@ describe('StoreStressConcurrent', () => { for (let j = 0; j < steps.length; j++) { container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render({steps[i]})); expect(print(store)).toMatch(snapshots[i]); act(() => root.render({steps[j]})); @@ -337,7 +337,7 @@ describe('StoreStressConcurrent', () => { for (let j = 0; j < steps.length; j++) { container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -408,9 +408,9 @@ describe('StoreStressConcurrent', () => { // This is the only step where we use Jest snapshots. const snapshots = []; let container = document.createElement('div'); - // $FlowFixMe - let root = ReactDOM.createRoot(container); for (let i = 0; i < steps.length; i++) { + // $FlowFixMe + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -512,6 +512,8 @@ describe('StoreStressConcurrent', () => { // 2. Verify check Suspense can render same steps as initial fallback content. for (let i = 0; i < steps.length; i++) { + // $FlowFixMe + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -536,7 +538,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -582,7 +584,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -640,7 +642,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -690,7 +692,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -744,7 +746,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -897,9 +899,9 @@ describe('StoreStressConcurrent', () => { // This is the only step where we use Jest snapshots. const snapshots = []; let container = document.createElement('div'); - // $FlowFixMe - let root = ReactDOM.createRoot(container); for (let i = 0; i < steps.length; i++) { + // $FlowFixMe + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -922,6 +924,8 @@ describe('StoreStressConcurrent', () => { // which is different from the snapshots above. So we take more snapshots. const fallbackSnapshots = []; for (let i = 0; i < steps.length; i++) { + // $FlowFixMe + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -1055,7 +1059,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -1107,7 +1111,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -1174,7 +1178,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -1226,7 +1230,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( @@ -1278,7 +1282,7 @@ describe('StoreStressConcurrent', () => { // Always start with a fresh container and steps[i]. container = document.createElement('div'); // $FlowFixMe - root = ReactDOM.createRoot(container); + const root = ReactDOM.createRoot(container); act(() => root.render( diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js index 0c26424097..c13c2805a5 100644 --- a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js @@ -14,6 +14,7 @@ let ReactDOM = require('react-dom'); let ReactDOMServer = require('react-dom/server'); let Scheduler = require('scheduler'); let act; +let useEffect; describe('ReactDOMRoot', () => { let container; @@ -26,6 +27,7 @@ describe('ReactDOMRoot', () => { ReactDOMServer = require('react-dom/server'); Scheduler = require('scheduler'); act = require('jest-react').act; + useEffect = React.useEffect; }); it('renders children', () => { @@ -342,4 +344,62 @@ describe('ReactDOMRoot', () => { }); expect(container.textContent).toEqual('b'); }); + + it('unmount is synchronous', async () => { + const root = ReactDOM.createRoot(container); + await act(async () => { + root.render('Hi'); + }); + expect(container.textContent).toEqual('Hi'); + + await act(async () => { + root.unmount(); + // Should have already unmounted + expect(container.textContent).toEqual(''); + }); + }); + + it('throws if an unmounted root is updated', async () => { + const root = ReactDOM.createRoot(container); + await act(async () => { + root.render('Hi'); + }); + expect(container.textContent).toEqual('Hi'); + + root.unmount(); + + expect(() => root.render("I'm back")).toThrow( + 'Cannot update an unmounted root.', + ); + }); + + it('warns if root is unmounted inside an effect', async () => { + const container1 = document.createElement('div'); + const root1 = ReactDOM.createRoot(container1); + const container2 = document.createElement('div'); + const root2 = ReactDOM.createRoot(container2); + + function App({step}) { + useEffect(() => { + if (step === 2) { + root2.unmount(); + } + }, [step]); + return 'Hi'; + } + + await act(async () => { + root1.render(); + }); + expect(container1.textContent).toEqual('Hi'); + + expect(() => { + ReactDOM.flushSync(() => { + root1.render(); + }); + }).toErrorDev( + 'Attempted to synchronously unmount a root while React was ' + + 'already rendering.', + ); + }); }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 80fc3d1e4a..a1bb39c929 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle'; import { batchedUpdates, discreteUpdates, - flushSync, - flushSyncWithoutWarningIfAlreadyRendering, + flushSync as flushSyncWithoutWarningIfAlreadyRendering, + isAlreadyRendering, flushControlled, injectIntoDevTools, attemptSynchronousHydration, @@ -163,6 +163,25 @@ const Internals = { ], }; +// Overload the definition to the two valid signatures. +// Warning, this opts-out of checking the function body. +declare function flushSync(fn: () => R): R; +// eslint-disable-next-line no-redeclare +declare function flushSync(): void; +// eslint-disable-next-line no-redeclare +function flushSync(fn) { + if (__DEV__) { + if (isAlreadyRendering()) { + console.error( + 'flushSync was called from inside a lifecycle method. React cannot ' + + 'flush when React is already rendering. Consider moving this call to ' + + 'a scheduler task or micro task.', + ); + } + } + return flushSyncWithoutWarningIfAlreadyRendering(fn); +} + export { createPortal, batchedUpdates as unstable_batchedUpdates, diff --git a/packages/react-dom/src/client/ReactDOMLegacy.js b/packages/react-dom/src/client/ReactDOMLegacy.js index f03350cb29..70dcddc502 100644 --- a/packages/react-dom/src/client/ReactDOMLegacy.js +++ b/packages/react-dom/src/client/ReactDOMLegacy.js @@ -29,7 +29,7 @@ import { createContainer, findHostInstanceWithNoPortals, updateContainer, - flushSyncWithoutWarningIfAlreadyRendering, + flushSync, getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, @@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer( }; } // Initial mount should not be batched. - flushSyncWithoutWarningIfAlreadyRendering(() => { + flushSync(() => { updateContainer(children, fiberRoot, parentComponent, callback); }); } else { @@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) { } // Unmount should not be batched. - flushSyncWithoutWarningIfAlreadyRendering(() => { + flushSync(() => { legacyRenderSubtreeIntoContainer(null, null, container, false, () => { // $FlowFixMe This should probably use `delete container._reactRootContainer` container._reactRootContainer = null; diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js index ba062fc3f7..1a027d9a1e 100644 --- a/packages/react-dom/src/client/ReactDOMRoot.js +++ b/packages/react-dom/src/client/ReactDOMRoot.js @@ -14,7 +14,7 @@ import type {FiberRoot} from 'react-reconciler/src/ReactInternalTypes'; export type RootType = { render(children: ReactNodeList): void, unmount(): void, - _internalRoot: FiberRoot, + _internalRoot: FiberRoot | null, ... }; @@ -62,17 +62,23 @@ import { updateContainer, findHostInstanceWithNoPortals, registerMutableSourceForHydration, + flushSync, + isAlreadyRendering, } from 'react-reconciler/src/ReactFiberReconciler'; import invariant from 'shared/invariant'; import {ConcurrentRoot} from 'react-reconciler/src/ReactRootTags'; import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags'; -function ReactDOMRoot(internalRoot) { +function ReactDOMRoot(internalRoot: FiberRoot) { this._internalRoot = internalRoot; } ReactDOMRoot.prototype.render = function(children: ReactNodeList): void { const root = this._internalRoot; + if (root === null) { + invariant(false, 'Cannot update an unmounted root.'); + } + if (__DEV__) { if (typeof arguments[1] === 'function') { console.error( @@ -109,10 +115,23 @@ ReactDOMRoot.prototype.unmount = function(): void { } } const root = this._internalRoot; - const container = root.containerInfo; - updateContainer(null, root, null, () => { + if (root !== null) { + this._internalRoot = null; + const container = root.containerInfo; + if (__DEV__) { + if (isAlreadyRendering()) { + console.error( + 'Attempted to synchronously unmount a root while React was already ' + + 'rendering. React cannot finish unmounting the root until the ' + + 'current render has completed, which may lead to a race condition.', + ); + } + } + flushSync(() => { + updateContainer(null, root, null, null); + }); unmarkContainerAsRoot(container); - }); + } }; export function createRoot( diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 2f0d70a602..c5021d9689 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -921,6 +921,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { return children; } + function flushSync(fn: () => R): R { + if (__DEV__) { + if (NoopRenderer.isAlreadyRendering()) { + console.error( + 'flushSync was called from inside a lifecycle method. React cannot ' + + 'flush when React is already rendering. Consider moving this call to ' + + 'a scheduler task or micro task.', + ); + } + } + return NoopRenderer.flushSync(fn); + } + let idCounter = 0; const ReactNoop = { @@ -1136,7 +1149,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { } }, - flushSync: NoopRenderer.flushSync, + flushSync, flushPassiveEffects: NoopRenderer.flushPassiveEffects, // Logs the current state of the tree. diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index d25783164e..6e992ddd7d 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -22,7 +22,7 @@ import { discreteUpdates as discreteUpdates_old, flushControlled as flushControlled_old, flushSync as flushSync_old, - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_old, + isAlreadyRendering as isAlreadyRendering_old, flushPassiveEffects as flushPassiveEffects_old, getPublicRootInstance as getPublicRootInstance_old, attemptSynchronousHydration as attemptSynchronousHydration_old, @@ -59,7 +59,7 @@ import { discreteUpdates as discreteUpdates_new, flushControlled as flushControlled_new, flushSync as flushSync_new, - flushSyncWithoutWarningIfAlreadyRendering as flushSyncWithoutWarningIfAlreadyRendering_new, + isAlreadyRendering as isAlreadyRendering_new, flushPassiveEffects as flushPassiveEffects_new, getPublicRootInstance as getPublicRootInstance_new, attemptSynchronousHydration as attemptSynchronousHydration_new, @@ -107,9 +107,9 @@ export const flushControlled = enableNewReconciler ? flushControlled_new : flushControlled_old; export const flushSync = enableNewReconciler ? flushSync_new : flushSync_old; -export const flushSyncWithoutWarningIfAlreadyRendering = enableNewReconciler - ? flushSyncWithoutWarningIfAlreadyRendering_new - : flushSyncWithoutWarningIfAlreadyRendering_old; +export const isAlreadyRendering = enableNewReconciler + ? isAlreadyRendering_new + : isAlreadyRendering_old; export const flushPassiveEffects = enableNewReconciler ? flushPassiveEffects_new : flushPassiveEffects_old; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 38f9955657..95844fcbf2 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -53,10 +53,10 @@ import { flushRoot, batchedUpdates, flushSync, + isAlreadyRendering, flushControlled, deferredUpdates, discreteUpdates, - flushSyncWithoutWarningIfAlreadyRendering, flushPassiveEffects, } from './ReactFiberWorkLoop.new'; import { @@ -330,7 +330,7 @@ export { discreteUpdates, flushControlled, flushSync, - flushSyncWithoutWarningIfAlreadyRendering, + isAlreadyRendering, flushPassiveEffects, }; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 3c0ae63752..802b1bc971 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -53,10 +53,10 @@ import { flushRoot, batchedUpdates, flushSync, + isAlreadyRendering, flushControlled, deferredUpdates, discreteUpdates, - flushSyncWithoutWarningIfAlreadyRendering, flushPassiveEffects, } from './ReactFiberWorkLoop.old'; import { @@ -330,7 +330,7 @@ export { discreteUpdates, flushControlled, flushSync, - flushSyncWithoutWarningIfAlreadyRendering, + isAlreadyRendering, flushPassiveEffects, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 0ed934f00e..830ae65006 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1191,11 +1191,11 @@ export function discreteUpdates( // Overload the definition to the two valid signatures. // Warning, this opts-out of checking the function body. -declare function flushSyncWithoutWarningIfAlreadyRendering(fn: () => R): R; +declare function flushSync(fn: () => R): R; // eslint-disable-next-line no-redeclare -declare function flushSyncWithoutWarningIfAlreadyRendering(): void; +declare function flushSync(): void; // eslint-disable-next-line no-redeclare -export function flushSyncWithoutWarningIfAlreadyRendering(fn) { +export function flushSync(fn) { // In legacy mode, we flush pending passive effects at the beginning of the // next event, not at the end of the previous one. if ( @@ -1232,23 +1232,13 @@ export function flushSyncWithoutWarningIfAlreadyRendering(fn) { } } -// Overload the definition to the two valid signatures. -// Warning, this opts-out of checking the function body. -declare function flushSync(fn: () => R): R; -// eslint-disable-next-line no-redeclare -declare function flushSync(): void; -// eslint-disable-next-line no-redeclare -export function flushSync(fn) { - if (__DEV__) { - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { - console.error( - 'flushSync was called from inside a lifecycle method. React cannot ' + - 'flush when React is already rendering. Consider moving this call to ' + - 'a scheduler task or micro task.', - ); - } - } - return flushSyncWithoutWarningIfAlreadyRendering(fn); +export function isAlreadyRendering() { + // Used by the renderer to print a warning if certain APIs are called from + // the wrong context. + return ( + __DEV__ && + (executionContext & (RenderContext | CommitContext)) !== NoContext + ); } export function flushControlled(fn: () => mixed): void { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 438dce8b13..9cfea1563a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1191,11 +1191,11 @@ export function discreteUpdates( // Overload the definition to the two valid signatures. // Warning, this opts-out of checking the function body. -declare function flushSyncWithoutWarningIfAlreadyRendering(fn: () => R): R; +declare function flushSync(fn: () => R): R; // eslint-disable-next-line no-redeclare -declare function flushSyncWithoutWarningIfAlreadyRendering(): void; +declare function flushSync(): void; // eslint-disable-next-line no-redeclare -export function flushSyncWithoutWarningIfAlreadyRendering(fn) { +export function flushSync(fn) { // In legacy mode, we flush pending passive effects at the beginning of the // next event, not at the end of the previous one. if ( @@ -1232,23 +1232,13 @@ export function flushSyncWithoutWarningIfAlreadyRendering(fn) { } } -// Overload the definition to the two valid signatures. -// Warning, this opts-out of checking the function body. -declare function flushSync(fn: () => R): R; -// eslint-disable-next-line no-redeclare -declare function flushSync(): void; -// eslint-disable-next-line no-redeclare -export function flushSync(fn) { - if (__DEV__) { - if ((executionContext & (RenderContext | CommitContext)) !== NoContext) { - console.error( - 'flushSync was called from inside a lifecycle method. React cannot ' + - 'flush when React is already rendering. Consider moving this call to ' + - 'a scheduler task or micro task.', - ); - } - } - return flushSyncWithoutWarningIfAlreadyRendering(fn); +export function isAlreadyRendering() { + // Used by the renderer to print a warning if certain APIs are called from + // the wrong context. + return ( + __DEV__ && + (executionContext & (RenderContext | CommitContext)) !== NoContext + ); } export function flushControlled(fn: () => mixed): void { diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index 20d846f64b..07cc3437a5 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -6,6 +6,8 @@ let useState; let useEffect; let startTransition; +// TODO: Migrate tests to React DOM instead of React Noop + describe('ReactFlushSync', () => { beforeEach(() => { jest.resetModules(); diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json index 1a6afe0233..3206efa413 100644 --- a/scripts/error-codes/codes.json +++ b/scripts/error-codes/codes.json @@ -396,5 +396,6 @@ "405": "hydrateRoot(...): Target container is not a DOM element.", "406": "act(...) is not supported in production builds of React.", "407": "Missing getServerSnapshot, which is required for server-rendered content. Will revert to client rendering.", - "408": "Missing getServerSnapshot, which is required for server-rendered content." + "408": "Missing getServerSnapshot, which is required for server-rendered content.", + "409": "Cannot update an unmounted root." }