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.

Sign up for GitHub

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

Jump to bottom

Make root.unmount() synchronous #22444

Merged
merged 2 commits into from Sep 27, 2021
Merged

Make root.unmount() synchronous #22444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa17cc2
Move flushSync warning to React DOM
acdlite Sep 27, 2021
8cb21aa
Make root.unmount() synchronous
acdlite Sep 27, 2021
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
40 changes: 22 additions & 18 deletions packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js
View file
Open in desktop
Expand Up @@ -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(<Root>{steps[i]}</Root>));
// We snapshot each step once so it doesn't regress.
snapshots.push(print(store));
Expand Down Expand Up @@ -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(<Root>{steps[i]}</Root>));
expect(print(store)).toMatch(snapshots[i]);
act(() => root.render(<Root>{steps[j]}</Root>));
Expand All @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand All @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand All @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down Expand Up @@ -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(
<Root>
Expand Down
60 changes: 60 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
View file
Open in desktop
Expand Up @@ -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;
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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(<App step={1} />);
});
expect(container1.textContent).toEqual('Hi');

expect(() => {
ReactDOM.flushSync(() => {
root1.render(<App step={2} />);
});
}).toErrorDev(
'Attempted to synchronously unmount a root while React was ' +
'already rendering.',
);
});
});
23 changes: 21 additions & 2 deletions packages/react-dom/src/client/ReactDOM.js
View file
Open in desktop
Expand Up @@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
import {
batchedUpdates,
discreteUpdates,
flushSync,
flushSyncWithoutWarningIfAlreadyRendering,
flushSync as flushSyncWithoutWarningIfAlreadyRendering,
isAlreadyRendering,
flushControlled,
injectIntoDevTools,
attemptSynchronousHydration,
Expand Down Expand Up @@ -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<R>(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,
Expand Down
6 changes: 3 additions & 3 deletions packages/react-dom/src/client/ReactDOMLegacy.js
View file
Open in desktop
Expand Up @@ -29,7 +29,7 @@ import {
createContainer,
findHostInstanceWithNoPortals,
updateContainer,
flushSyncWithoutWarningIfAlreadyRendering,
flushSync,
getPublicRootInstance,
findHostInstance,
findHostInstanceWithWarning,
Expand Down Expand Up @@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
};
}
// Initial mount should not be batched.
flushSyncWithoutWarningIfAlreadyRendering(() => {
flushSync(() => {
updateContainer(children, fiberRoot, parentComponent, callback);
});
} else {
Expand Down Expand Up @@ -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;
Expand Down
29 changes: 24 additions & 5 deletions packages/react-dom/src/client/ReactDOMRoot.js
View file
Open in desktop
Expand Up @@ -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,
...
};

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
15 changes: 14 additions & 1 deletion packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Expand Up @@ -921,6 +921,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
return children;
}

function flushSync<R>(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 = {
Expand Down Expand Up @@ -1136,7 +1149,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}
},

flushSync: NoopRenderer.flushSync,
flushSync,
flushPassiveEffects: NoopRenderer.flushPassiveEffects,

// Logs the current state of the tree.
Expand Down