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.

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

Root API should clear non-empty roots before mounting #18730

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions packages/react-art/src/ReactARTHostConfig.js
Expand Up @@ -427,6 +427,10 @@ export function unhideTextInstance(textInstance, text): void {
// Noop
}

export function clearContainer(container) {
// TODO Implement this
}

export function DEPRECATED_mountResponderInstance(
responder: ReactEventResponder<any, any>,
responderInstance: ReactEventResponderInstance<any, any>,
Expand Down
36 changes: 32 additions & 4 deletions packages/react-dom/src/__tests__/ReactDOMRoot-test.js
Expand Up @@ -113,7 +113,28 @@ describe('ReactDOMRoot', () => {
expect(() => Scheduler.unstable_flushAll()).toErrorDev('Extra attributes');
});

it('does not clear existing children', async () => {
it('clears existing children with legacy API', async () => {
container.innerHTML = '<div>a</div><div>b</div>';
ReactDOM.render(
<div>
<span>c</span>
<span>d</span>
</div>,
container,
);
expect(container.textContent).toEqual('cd');
ReactDOM.render(
<div>
<span>d</span>
<span>c</span>
</div>,
container,
);
Scheduler.unstable_flushAll();
expect(container.textContent).toEqual('dc');
});

it('clears existing children', async () => {
container.innerHTML = '<div>a</div><div>b</div>';
const root = ReactDOM.createRoot(container);
root.render(
Expand All @@ -123,15 +144,15 @@ describe('ReactDOMRoot', () => {
</div>,
);
Scheduler.unstable_flushAll();
expect(container.textContent).toEqual('abcd');
expect(container.textContent).toEqual('cd');
root.render(
<div>
<span>d</span>
<span>c</span>
</div>,
);
Scheduler.unstable_flushAll();
expect(container.textContent).toEqual('abdc');
expect(container.textContent).toEqual('dc');
});

it('throws a good message on invalid containers', () => {
Expand Down Expand Up @@ -220,7 +241,14 @@ describe('ReactDOMRoot', () => {
let unmounted = false;
expect(() => {
unmounted = ReactDOM.unmountComponentAtNode(container);
}).toErrorDev('Did you mean to call root.unmount()?', {withoutStack: true});
}).toErrorDev(
[
'Did you mean to call root.unmount()?',
// This is more of a symptom but restructuring the code to avoid it isn't worth it:
"The node you're attempting to unmount was rendered by React and is not a top-level container.",
],
{withoutStack: true},
);
expect(unmounted).toBe(false);
Scheduler.unstable_flushAll();
expect(container.textContent).toEqual('Hi');
Expand Down
11 changes: 11 additions & 0 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Expand Up @@ -634,6 +634,17 @@ export function unhideTextInstance(
textInstance.nodeValue = text;
}

export function clearContainer(container: Container): void {
if (container.nodeType === ELEMENT_NODE) {
((container: any): Element).textContent = '';
} else if (container.nodeType === DOCUMENT_NODE) {
const body = ((container: any): Document).body;
if (body != null) {
body.textContent = '';
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

// -------------------
// Hydration
// -------------------
Expand Down
5 changes: 5 additions & 0 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
Expand Up @@ -482,6 +482,11 @@ export function unhideInstance(instance: Instance, props: Props): void {
);
}

export function clearContainer(container: Container): void {
// TODO Implement this for React Native
// UIManager does not expose a "remove all" type method.
}

export function unhideTextInstance(
textInstance: TextInstance,
text: string,
Expand Down
6 changes: 6 additions & 0 deletions packages/react-noop-renderer/src/createReactNoop.js
Expand Up @@ -155,6 +155,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
insertInContainerOrInstanceBefore(parentInstance, child, beforeChild);
}

function clearContainer(container: Container): void {
container.children.splice(0);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}

function removeChildFromContainerOrInstance(
parentInstance: Container | Instance,
child: Instance | TextInstance,
Expand Down Expand Up @@ -502,6 +506,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
insertInContainerBefore,
removeChild,
removeChildFromContainer,
clearContainer,

hideInstance(instance: Instance): void {
instance.hidden = true;
Expand Down Expand Up @@ -531,6 +536,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
supportsPersistence: true,

cloneInstance,
clearContainer,

createContainerChildSet(
container: Container,
Expand Down
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.new.js
Expand Up @@ -113,6 +113,7 @@ import {
commitHydratedContainer,
commitHydratedSuspenseInstance,
beforeRemoveInstance,
clearContainer,
} from './ReactFiberHostConfig';
import {
captureCommitPhaseError,
Expand Down Expand Up @@ -295,7 +296,15 @@ function commitBeforeMutationLifeCycles(
}
return;
}
case HostRoot:
case HostRoot: {
if (supportsMutation) {
if (finishedWork.effectTag & Snapshot) {
const root = finishedWork.stateNode;
clearContainer(root.containerInfo);
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}
}
return;
}
case HostComponent:
case HostText:
case HostPortal:
Expand Down
11 changes: 10 additions & 1 deletion packages/react-reconciler/src/ReactFiberCommitWork.old.js
Expand Up @@ -111,6 +111,7 @@ import {
commitHydratedContainer,
commitHydratedSuspenseInstance,
beforeRemoveInstance,
clearContainer,
} from './ReactFiberHostConfig';
import {
captureCommitPhaseError,
Expand Down Expand Up @@ -293,7 +294,15 @@ function commitBeforeMutationLifeCycles(
}
return;
}
case HostRoot:
case HostRoot: {
if (supportsMutation) {
if (finishedWork.effectTag & Snapshot) {
const root = finishedWork.stateNode;
clearContainer(root.containerInfo);
}
}
return;
}
case HostComponent:
case HostText:
case HostPortal:
Expand Down
16 changes: 15 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.new.js
Expand Up @@ -58,7 +58,13 @@ import {
OffscreenComponent,
} from './ReactWorkTags';
import {NoMode, BlockingMode} from './ReactTypeOfMode';
import {Ref, Update, NoEffect, DidCapture} from './ReactSideEffectTags';
import {
Ref,
Update,
NoEffect,
DidCapture,
Snapshot,
} from './ReactSideEffectTags';
import invariant from 'shared/invariant';

import {
Expand Down Expand Up @@ -675,6 +681,14 @@ function completeWork(
// If we hydrated, then we'll need to schedule an update for
// the commit side-effects on the root.
markUpdate(workInProgress);
} else if (!fiberRoot.hydrate) {
// Schedule an effect to clear this container at the start of the next commit.
// This handles the case of React rendering into a container with previous children.
// It's also safe to do for updates too, because current.child would only be null
// if the previous render was null (so the the container would already be empty).
//
// The additional root.hydrate check is required for hydration in legacy mode with no fallback.
workInProgress.effectTag |= Snapshot;
}
}
updateHostContainer(workInProgress);
Expand Down
9 changes: 9 additions & 0 deletions packages/react-reconciler/src/ReactFiberCompleteWork.old.js
Expand Up @@ -61,6 +61,7 @@ import {
NoEffect,
DidCapture,
Deletion,
Snapshot,
} from './ReactSideEffectTags';
import invariant from 'shared/invariant';

Expand Down Expand Up @@ -678,6 +679,14 @@ function completeWork(
// If we hydrated, then we'll need to schedule an update for
// the commit side-effects on the root.
markUpdate(workInProgress);
} else if (!fiberRoot.hydrate) {
// Schedule an effect to clear this container at the start of the next commit.
// This handles the case of React rendering into a container with previous children.
// It's also safe to do for updates too, because current.child would only be null
// if the previous render was null (so the the container would already be empty).
//
// The additional root.hydrate check is required for hydration in legacy mode with no fallback.
workInProgress.effectTag |= Snapshot;
}
}
updateHostContainer(workInProgress);
Expand Down
Expand Up @@ -37,3 +37,4 @@ export const hideInstance = shim;
export const hideTextInstance = shim;
export const unhideInstance = shim;
export const unhideTextInstance = shim;
export const clearContainer = shim;
Expand Up @@ -53,6 +53,7 @@ describe('ReactFiberHostContext', () => {
appendChildToContainer: function() {
return null;
},
clearContainer: function() {},
supportsMutation: true,
});

Expand Down Expand Up @@ -107,6 +108,7 @@ describe('ReactFiberHostContext', () => {
appendChildToContainer: function() {
return null;
},
clearContainer: function() {},
supportsMutation: true,
});

Expand Down
Expand Up @@ -107,6 +107,7 @@ export const updateFundamentalComponent =
$$$hostConfig.updateFundamentalComponent;
export const unmountFundamentalComponent =
$$$hostConfig.unmountFundamentalComponent;
export const clearContainer = $$$hostConfig.clearContainer;

// -------------------
// Persistence
Expand Down
4 changes: 4 additions & 0 deletions packages/react-test-renderer/src/ReactTestHostConfig.js
Expand Up @@ -126,6 +126,10 @@ export function removeChild(
parentInstance.children.splice(index, 1);
}

export function clearContainer(container: Container): void {
container.children.splice(0);
}

export function getRootHostContext(
rootContainerInstance: Container,
): HostContext {
Expand Down