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 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/react-art/src/ReactARTHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,14 @@ export function unhideTextInstance(textInstance, text): void {
// Noop
}

export function clearContainer(container: Container): void {
// TODO This doesn't work for anything other than SVG.
// Is that okay?
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
while (container.lastChild != null) {
container.lastChild.eject();
}
}

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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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).innerHTML = '';
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
} else if (container.nodeType === DOCUMENT_NODE) {
const documentElement = ((container: any): Document).documentElement;
if (documentElement != null) {
documentElement.innerHTML = '';
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

// -------------------
// Hydration
// -------------------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ describe('when Trusted Types are available in global object', () => {
container,
);
expect(container.innerHTML).toBe('<div><b>Hi</b></div>');
expect(innerHTMLCalls.length).toBe(1);
// Second call to innerHTML is the clearContainer check.
expect(innerHTMLCalls.length).toBe(2);
// Ensure it didn't get stringified when passed to a DOM sink:
expect(innerHTMLCalls[0]).toBe(ttObject1);

Expand Down
5 changes: 5 additions & 0 deletions packages/react-native-renderer/src/ReactNativeHostConfig.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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;
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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